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
DM-34924: Check for dataset type compatibility when creating execution butler #251
Conversation
Codecov Report
@@ Coverage Diff @@
## main #251 +/- ##
==========================================
- Coverage 72.14% 71.95% -0.19%
==========================================
Files 60 60
Lines 6628 6651 +23
Branches 1250 1254 +4
==========================================
+ Hits 4782 4786 +4
- Misses 1621 1641 +20
+ Partials 225 224 -1
Continue to review full report at Codecov.
|
8ec15cf
to
f54b99b
Compare
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.
I'm assuming I shouldn't worry too much about Codecov having lots of comments to share? I have minor comments only, overall this looks good and fixes my problem, thank you!
# Use the first type encountered. | ||
return prevDsType | ||
|
||
raise ValueError( |
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.
Suggest putting this in an explicit else
clause for clarity
dsType = registryDsType | ||
else: | ||
# Not compatible to re-raise the original exception. | ||
raise |
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.
Please include a helpful error message as before, e.g., "Cannot insert {dsType} into the registry because it is incompatible with {registryDsType}"
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.
I am dreaming of python 3.11 exception notes. They would be perfect for this scenario. I think in this case though the exception that you are going to get is going to tell you "ConflictingDefinitionError(DatasetType A differs from Registry version B)" or something and there's not a lot I need to add.
A graph can contain dataset types that are shared with the dataset types that are automatically defined, but use different dataset types.
Checklist
doc/changes