fix nightly tests #8496
fix nightly tests #8496
Conversation
WalkthroughThe changes involve updates to the CI-nightly workflow configuration in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
.github/workflows/schedule.yml (5)
150-152: LGTM: Updated server image tagging, but note the TODOThe change to use the 'dev' tag for the server image is consistent with the UI image tagging and aligns with the PR objectives. The additional 'local' tag appears to be for compatibility purposes.
However, please note the TODO comment about fixing the tag in Dockerfile.ci. This should be addressed to ensure consistency across all configurations.
Consider creating a follow-up task to address the TODO comment and update Dockerfile.ci accordingly.
161-172: LGTM: Improved organization of SDK generation and dependency installationThe reorganization of the SDK generation and dependency installation steps is a good improvement. It enhances readability and potentially reduces the overall execution time of the workflow. This aligns well with the PR objective of improving the nightly tests.
Consider adding a comment explaining the purpose of the
--extra-index-urlflag in the pip install command. This would improve maintainability for future contributors.
Line range hint
174-184: LGTM: Added comprehensive REST API and SDK testsThe addition of multiple pytest commands with different configurations is an excellent improvement to the CI pipeline. This enhances test coverage and helps identify potential issues across various scenarios, directly contributing to the PR objective of fixing and improving nightly tests.
Consider extracting the pytest commands into a separate shell script to improve readability and maintainability of the workflow file. This would also make it easier to run these tests locally if needed.
Line range hint
254-258: LGTM: Improved image tagging and server waiting mechanismThe consistent use of 'dev' tags for both UI and server images is a good practice. The enhanced server waiting mechanism with additional logging is an excellent improvement for troubleshooting CI issues. These changes contribute to the overall reliability of the nightly tests.
Consider adding a timeout to the server waiting loop to prevent infinite waiting in case of unexpected issues. This could be implemented by checking if
max_trieshas reached zero and exiting with an error if so.Also applies to: 270-288
Line range hint
1-324: Overall assessment: Improvements to CI-nightly workflowThe changes made to this workflow file are well-aligned with the PR objectives of fixing and improving nightly tests. Key improvements include:
- Consistent use of 'dev' tags for images
- Reorganization and enhancement of test steps
- Improved logging and error handling
These modifications contribute to a more reliable and maintainable CI pipeline. The changes are generally well-implemented, with only minor suggestions for further improvements.
Consider creating a separate configuration file for test-specific settings (e.g., image tags, test commands) to make the workflow more flexible and easier to maintain in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/schedule.yml (4 hunks)
🔇 Additional comments (1)
.github/workflows/schedule.yml (1)
130-130: LGTM: Updated UI image tag to 'dev'Changing the tag from 'latest' to 'dev' is a good practice. It provides better control over the specific version used in the CI pipeline and aligns with the PR objective of fixing nightly tests.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8496 +/- ##
===========================================
- Coverage 74.34% 74.34% -0.01%
===========================================
Files 394 394
Lines 42216 42211 -5
Branches 3896 3896
===========================================
- Hits 31387 31383 -4
+ Misses 10829 10828 -1
|
| @@ -127,7 +127,7 @@ jobs: | |||
| context: . | |||
| file: ./Dockerfile.ui | |||
| cache-from: type=local,src=/tmp/cvat_cache_ui | |||
| tags: cvat/ui:latest | |||
| tags: cvat/ui:dev | |||
There was a problem hiding this comment.
Is this image actually used for anything? It seems like the UI doesn't actually get started at any point in this job.
And if it is needed, wouldn't it make more sense to pull the image that's already been built, instead of building it anew?
| @@ -147,9 +147,9 @@ jobs: | |||
| - name: Pull CVAT server image | |||
| run: | | |||
| docker pull ${{ steps.meta-server.outputs.tags }} | |||
| docker tag ${{ steps.meta-server.outputs.tags }} cvat/server:dev | |||
| #TODO fix tag in Dockerfile.ci | |||
There was a problem hiding this comment.
Could you elaborate? What do you want to fix?
Motivation and context
How has this been tested?
https://github.com/cvat-ai/cvat/actions/runs/11124390649
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit