-
Notifications
You must be signed in to change notification settings - Fork 132
Fix distributed tests #1438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix distributed tests #1438
Conversation
Reviewer's GuideEnable distributed test support by centralizing job creation, standardizing environment variable handling, refining worker startup logic, updating path casting, simplifying test assertions, and skipping checkpoints tests under distributed mode. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider using the pytest monkeypatch fixture for all environment variable changes instead of modifying os.environ directly (e.g., in cloud_server_credentials) to ensure state is properly isolated and restored between tests.
- There’s a lot of repeated DATACHAIN_JOB_ID setup in multiple fixtures—factor that logic into a single helper fixture to reduce duplication and make future updates easier.
- The query string in _create_job looks like it’s missing a closing bracket/parenthesis around the call to save()—double-check that expression to avoid silent syntax errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using the pytest monkeypatch fixture for all environment variable changes instead of modifying os.environ directly (e.g., in cloud_server_credentials) to ensure state is properly isolated and restored between tests.
- There’s a lot of repeated DATACHAIN_JOB_ID setup in multiple fixtures—factor that logic into a single helper fixture to reduce duplication and make future updates easier.
- The query string in _create_job looks like it’s missing a closing bracket/parenthesis around the call to save()—double-check that expression to avoid silent syntax errors.
## Individual Comments
### Comment 1
<location> `tests/conftest.py:494-495` </location>
<code_context>
[email protected]
-def cloud_server_credentials(cloud_server, monkeypatch):
[email protected](scope="session")
+def cloud_server_credentials(cloud_server):
if cloud_server.kind == "s3":
cfg = cloud_server.src.fs.client_kwargs
</code_context>
<issue_to_address>
**question (testing):** Changed cloud_server_credentials fixture scope to session.
Ensure that no tests modify environment variables set by this fixture, as session scope will share state across all tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @pytest.mark.skipif( | ||
| "os.environ.get('DATACHAIN_DISTRIBUTED')", | ||
| reason="Checkpoints test skipped in distributed mode", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkpoints tests failed with distributed more enabled. Need to fix them later, not a subject for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors distributed testing configuration to improve test isolation and flexibility. The main changes standardize environment variable handling, add conditional test skipping for distributed mode, and modify exception handling expectations.
- Adds
monkeypatch.delenv("DATACHAIN_JOB_ID", raising=False)to job management tests to ensure clean test state - Introduces
@pytest.mark.skipifdecorators to skip checkpoint tests in distributed mode - Standardizes exception handling in UDF tests to expect
RuntimeErrorinstead of conditional logic - Refactors
cloud_server_credentialsfixture to session scope and use directos.environmanipulation - Updates
run_datachain_workerfixture to support dynamic queue names and disable distributed mode guard
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_job_management.py | Adds monkeypatch fixture and clears DATACHAIN_JOB_ID env var to ensure test isolation |
| tests/unit/test_catalog_loader.py | Adds environment variable cleanup for distributed mode testing |
| tests/unit/lib/test_checkpoints.py | Adds skip conditions for checkpoint tests in distributed mode |
| tests/func/test_checkpoints.py | Adds skip condition for checkpoint test in distributed mode |
| tests/func/test_udf.py | Simplifies exception handling by removing conditional logic for distributed vs local mode |
| tests/conftest.py | Refactors fixtures: extracts job creation helper, changes fixture scopes, adds dynamic queue generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1438 +/- ##
=======================================
Coverage 87.96% 87.96%
=======================================
Files 160 160
Lines 15377 15377
Branches 2224 2224
=======================================
Hits 13527 13527
Misses 1336 1336
Partials 514 514
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Deploying datachain-documentation with
|
| Latest commit: |
2b8278d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e5caa63f.datachain-documentation.pages.dev |
| Branch Preview URL: | https://metastore-update.datachain-documentation.pages.dev |
Co-authored-by: Copilot <[email protected]>
|
Closing this, see new version: #1451 |
Update fixtures and tests to enable distributed tests.
See related SaaS PR for more details.
Summary by Sourcery
Enable and streamline distributed test support by adjusting fixtures, environment variable handling, and test behaviors.
Enhancements:
_create_jobhelper and automatically set DATACHAIN_JOB_ID in metastore fixtures and datachain_job_id fixturedb_fileparametersTests: