-
Notifications
You must be signed in to change notification settings - Fork 13
Updates to handle bad dataset exceptions on the server side #674
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #674 +/- ##
==========================================
+ Coverage 97.19% 97.72% +0.52%
==========================================
Files 29 29
Lines 1958 2413 +455
==========================================
+ Hits 1903 2358 +455
Misses 55 55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 extends previous work to handle bad dataset exceptions from the ServiceX server by providing more detailed feedback to users when dataset lookups fail. The changes add support for a new "bad_dataset" status and integrate server-side dataset information to provide better error messages.
- Added support for handling "bad_dataset" status from ServiceX server
- Enhanced error reporting with detailed dataset lookup information
- Added comprehensive test coverage for new bad dataset scenarios
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 |
|---|---|
| servicex/models.py | Added bad_dataset status enum and did_id field to TransformStatus |
| servicex/query_core.py | Implemented bad dataset handling logic with detailed error messages |
| tests/test_dataset.py | Added test cases for bad dataset scenarios and zero file completions |
| tests/test_servicex_dataset.py | Updated test data to include did_id field |
| tests/conftest.py | Updated test fixtures to include did_id field |
| tests/app/test_transforms.py | Updated test fixture to include did_id field |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gordonwatts
left a comment
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.
This looks great - will be lots better. I'd rather see my code from the closed PR dump the dataset name, because that is what causes the error here, and that gives the user direct access to what is causing the problem.
* Updates to handle bad dataset exceptions on the server side; give DID in warning message
Extend @MattShirley 's #657 to handle the more detailed feedback on bad datasets from the server, as implemented by ssl-hep/ServiceX#1134 .