DM-33600: Improve error handling in registry query methods #651

andy-slac · 2022-02-25T16:30:29Z

Several related changes, some small and some big:

Fix handling of binds in analysis of user expression for governor values.
Check that governor values specified in user expression really exist, raise exception if they are not. This is to make it consistent with handling of keyword arguments.
Add explain_no_result to the result class returned from queryDimensionRecords.
Change types of exceptions generated by registry to use more structured exception types, to make it easier to handle these exceptions on client side.

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes

TallJimbo

Looks great!

My only quibble is with one of the exception additions: at least most (maybe all) of the direct raises of CollectionError (as opposed to one of its derived classes) seem like they might be better remaining as TypeError, or maybe a new exception like NoDefaultProvided that inherits from it - they're cases where an argument isn't being provided to a method, and no default was provided at Registry initialization. To me that's on the spectrum of function-signature-mismatch logic bugs (which are reliably TypeError in Python), and the kind of thing that a more strongly-typed implementation might even statically reject (e.g. if RegistryWithDefaults was not the same class as RegistryWithoutDefaults), and not terribly related to the CollectionError subclasses.

andy-slac · 2022-02-25T18:11:07Z

I think I'll add NoDefaultCollectionError for the case of missing default collection. I'd like to avoid using very common exception types because handling those reliably is not really possible. Getting exception hierarchy right is a tough problem, lake many other things related to error handling.

I also realized that I probably need to expose some exception classes at the butler module level, right now they are only visible in registry module. As Registry API is already more or less public, I think we should make documented exception public as well.

Handling of binds in InspectionVisitor was incomplete which caused issues with checking for governor constraints. This patch handles bind values as literals which fixes the issue. A simple unit test is added to reproduce the issue.

Check that literal values given for governor dimensions in WHERE expressions are actually know, raise a LookupError if values are not known.

Also changed annotated return type of Registry query methods to return query result instances instead of iterables. Added docstrings for raised exception to query methods.

This introduces exception class hierarchy for use in registry methods. It is limited to registry only and does not touch core module or datastore, though some changes in Butler class were needed to make it consistent with registry exceptions. Registry docstrings were updated to enumerate expected exception types. There is still no guarantee that raised exceptions will be limited to classes in this hierarchy, there is always a possibility that exceptions from other modules will leak. In particular there was no attempt in this commit to convert possible SQLAlchemy exceptions into registry-specific types.

codecov · 2022-02-25T18:57:47Z

Codecov Report

Merging #651 (aade7fb) into main (837304b) will increase coverage by 0.05%.
The diff coverage is 82.41%.

❗ Current head aade7fb differs from pull request most recent head e6d6b07. Consider uploading reports for the commit e6d6b07 to get more accurate results

@@            Coverage Diff             @@
##             main     #651      +/-   ##
==========================================
+ Coverage   83.60%   83.65%   +0.05%     
==========================================
  Files         239      239              
  Lines       30254    30367     +113     
  Branches     5066     5086      +20     
==========================================
+ Hits        25295    25405     +110     
- Misses       3806     3809       +3     
  Partials     1153     1153

Impacted Files	Coverage Δ
python/lsst/daf/butler/registries/remote.py	`0.00% <0.00%> (ø)`
...n/lsst/daf/butler/registry/interfaces/_datasets.py	`72.82% <ø> (ø)`
python/lsst/daf/butler/script/transferDatasets.py	`41.17% <0.00%> (ø)`
.../butler/registry/datasets/byDimensions/_storage.py	`83.55% <20.00%> (ø)`
python/lsst/daf/butler/registries/sql.py	`80.83% <40.62%> (-0.91%)`	⬇️
...t/daf/butler/registry/queries/expressions/check.py	`89.58% <72.22%> (-0.56%)`	⬇️
...ython/lsst/daf/butler/registry/queries/_structs.py	`90.43% <75.00%> (+0.04%)`	⬆️
...ython/lsst/daf/butler/registry/queries/_results.py	`83.09% <80.00%> (+0.67%)`	⬆️
python/lsst/daf/butler/registry/tests/_registry.py	`98.97% <98.38%> (-0.05%)`	⬇️
python/lsst/daf/butler/_butler.py	`80.75% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 837304b...e6d6b07. Read the comment docs.

andy-slac · 2022-02-25T23:28:22Z

@TallJimbo, could you check 12540ac, I have added a small paragraph documenting exceptions and explain_no_results?

TallJimbo

I've made added some patch recommendations with newlines between sentences and very small grammatical fixes.

doc/lsst.daf.butler/queries.rst

TallJimbo approved these changes Feb 25, 2022

View reviewed changes

andy-slac added 4 commits February 25, 2022 10:32

Check governor values provided in user expressions.

0282879

Check that literal values given for governor dimensions in WHERE expressions are actually know, raise a LookupError if values are not known.

Add explain_no_results method to DimensionRecordQueryResults.

fd49870

Also changed annotated return type of Registry query methods to return query result instances instead of iterables. Added docstrings for raised exception to query methods.

andy-slac force-pushed the tickets/DM-33600 branch from 6941eb3 to 59ef99b Compare February 25, 2022 18:38

TallJimbo approved these changes Feb 26, 2022

View reviewed changes

doc/lsst.daf.butler/queries.rst Outdated Show resolved Hide resolved

doc/lsst.daf.butler/queries.rst Outdated Show resolved Hide resolved

doc/lsst.daf.butler/queries.rst Outdated Show resolved Hide resolved

Add doc snippet; document exceptions and explain_no_results

e6d6b07

andy-slac force-pushed the tickets/DM-33600 branch from aade7fb to e6d6b07 Compare February 27, 2022 01:50

andy-slac merged commit e5b471c into main Feb 27, 2022

andy-slac deleted the tickets/DM-33600 branch February 27, 2022 02:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-33600: Improve error handling in registry query methods #651

DM-33600: Improve error handling in registry query methods #651

andy-slac commented Feb 25, 2022 •

edited

TallJimbo left a comment

andy-slac commented Feb 25, 2022

codecov bot commented Feb 25, 2022 •

edited

andy-slac commented Feb 25, 2022

TallJimbo left a comment

DM-33600: Improve error handling in registry query methods #651

DM-33600: Improve error handling in registry query methods #651

Conversation

andy-slac commented Feb 25, 2022 • edited

Checklist

TallJimbo left a comment

Choose a reason for hiding this comment

andy-slac commented Feb 25, 2022

codecov bot commented Feb 25, 2022 • edited

Codecov Report

andy-slac commented Feb 25, 2022

TallJimbo left a comment

Choose a reason for hiding this comment

andy-slac commented Feb 25, 2022 •

edited

codecov bot commented Feb 25, 2022 •

edited