Disable some worker pods for helm testing workflow #8671
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Worker
participant Export
participant Utils
participant Annotation
User->>Worker: Request for processing
Worker->>Worker: Set NUMPROCS to 2
Worker-->>User: Process completed
User->>Export: Request for export
Export->>Export: Set NUMPROCS to 2
Export-->>User: Export completed
User->>Utils: Request for utility
Utils-->>User: No active instances (replicas: 0)
User->>Annotation: Check annotations
Annotation-->>User: No active instances (replicas: 0)
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://avatars.githubusercontent.com/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 (2)
helm-chart/test.values.yaml (2)
25-29: Document the rationale for worker configuration.The export worker configuration mirrors the import worker settings. Consider adding a comment explaining why these specific values (
replicas: 1,NUMPROCS: '2') were chosen for the testing environment.export: + # Reduced replicas and increased NUMPROCS for testing environment + # to optimize resource usage while maintaining necessary capacity replicas: 1 additionalEnv: - name: NUMPROCS value: '2'
17-33: Update documentation and changelog.These configuration changes should be:
- Documented in the changelog
- Reflected in the testing documentation
- Added to the PR description with explanation of the testing strategy
This will help other developers understand the rationale behind these changes and their impact on the testing workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
helm-chart/test.values.yaml(1 hunks)
🔇 Additional comments (2)
helm-chart/test.values.yaml (2)
17-20: Verify testing workflow performance with reduced worker replicas.
Setting replicas: 1 with NUMPROCS: '2' means we'll have fewer worker pods but each worker will handle 2 processes. While this reduces resource usage, we should ensure it doesn't negatively impact testing performance.
Could you confirm if this configuration has been tested to ensure:
- Tests complete within acceptable time limits
- No resource bottlenecks occur during parallel test execution
31-33:
Verify test coverage with disabled utils and annotation pods.
Setting replicas: 0 for both utils and annotation pods completely disables these services. Please ensure:
- No test scenarios depend on these services
- This won't cause any test failures or unexpected behavior
✅ Verification successful
Let me gather more specific information about how these services are used in the tests.
Let me check one more aspect to understand if these services are used in test setup or teardown.
Based on the gathered information, I can now provide a final response:
Setting replicas to 0 for utils and annotation pods is safe for testing
The investigation shows that while there are many test files referencing "utils" and "annotation", these are primarily related to test cases for API endpoints and SDK functionality, not actual dependencies on the utils and annotation services. The evidence shows:
- No direct service dependencies found in test configurations
- The references in test files are about testing annotation-related APIs/features
- No test fixtures or setup/teardown procedures require these services
- These services are typically used in production (default 1 replica in values.yaml) but aren't critical for test execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that might depend on utils or annotation services
rg -l -g "test_*.py" "utils|annotation"
# Search for any direct references to these services in test configurations
rg -l "utils:|annotation:" tests/ helm-chart/
Length of output: 1686
Script:
#!/bin/bash
# Check content of test files for actual dependencies on utils/annotation services
rg -C2 "utils.*service|annotation.*service" tests/
# Check helm chart values to understand the default configuration
cat helm-chart/values.yaml | grep -A5 -B5 "utils:\|annotation:"
# Look for any test setup/configuration files
fd "conftest|pytest.*conf" tests/
# Check if there are any specific test markers or fixtures related to these services
rg -C2 "@pytest.(fixture|mark).*utils|@pytest.(fixture|mark).*annotation" tests/
Length of output: 5220
Script:
#!/bin/bash
# Check test setup/teardown in conftest files
cat tests/python/conftest.py
cat tests/python/sdk/conftest.py
cat tests/python/cli/conftest.py
# Check if there are any test dependencies defined in setup files
fd "setup.py|requirements.*" tests/
Length of output: 591
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8671 +/- ##
===========================================
- Coverage 74.25% 74.20% -0.05%
===========================================
Files 401 401
Lines 43465 43504 +39
Branches 3950 3950
===========================================
+ Hits 32273 32282 +9
- Misses 11192 11222 +30
|
helm-chart/test.values.yaml
Outdated
| replicas: 1 | ||
| additionalEnv: | ||
| - name: NUMPROCS | ||
| value: '2' |
There was a problem hiding this comment.
Do we even need 2? Why not make it 1?
| utils: | ||
| additionalEnv: | ||
| - name: DJANGO_SETTINGS_MODULE | ||
| value: cvat.settings.testing_rest |
There was a problem hiding this comment.
It doesn't seem right that you can just remove this. This settings module contains a setting specifically related to the cleaning queue, IMPORT_CACHE_CLEAN_DELAY; so presumably there's a test that depends on this. How can the test still work with this removed?
There was a problem hiding this comment.
How can the test still work with this removed?
If I remember correctly, these tests were skipped because they were unstable.
|
a couple of warkflow runs on this branch https://github.com/cvat-ai/cvat/actions/runs/11744289016/job/32791689399
Motivation and context
How has this been tested?
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
New Features
NUMPROCSfor improved performance in worker and export components.Bug Fixes