DM-41761: Add Butler.query property #914
Conversation
andy-slac
force-pushed
the
tickets/DM-41761
branch
from
1d1df20 to
fdcfcf4
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #914 +/- ##
==========================================
+ Coverage 87.24% 87.50% +0.25%
==========================================
Files 286 292 +6
Lines 37168 38067 +899
Branches 7833 8062 +229
==========================================
+ Hits 32428 33310 +882
- Misses 3539 3553 +14
- Partials 1201 1204 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
There are some potential (minor, probably preexisting) issues with which test lines are actually raising our expected exceptions, but otherwise I've got nothing much to say for the second of half of this review.
I've hit "Accepted" because I think everything I'd like to consider changing can be deferred to another ticket if we're marking the Butler.query result types as experimental somehow. But I would like to avoid committing long-term to supporting some aspects of the old result-type interface via Butler.query.
andy-slac
force-pushed
the
tickets/DM-41761
branch
3 times, most recently
from
0a1c9b9 to
f8954bf
Compare
There was a problem hiding this comment.
I would like to make Butler.query private (i.e. Butler._query) to avoid committing to the Query class APIs (and the APIs of its helper classes); I just think we probably want to keep all of these APIs private until we're moderately far along writing the RemoteButler versions, as I could see unexpected difficulties there shaking things up.
I think the Butler convenience methods are probably closest to being ready for public consumption - the fact that those do not return objects capable of follow-up queries or lazy iteration makes me pretty confident RemoteButler won't throw a wrench in their works, but it's still conceivable we'd want to make small adjustments in the future to make them more similar to corresponding advanced-interface methods that do need to be adjusted due to RemoteButler considerations.
But while I want to be very cautious about rolling this out because I think we might want to change thigns, I don't actually see anything here that differs enough from how I'd imagined doing it for me to have recommendations to change more now, so I think we put up that last "privacy wall" and proceed.
| """The components of the parent dataset type included in these results | ||
| (`~collections.abc.Sequence` [ `str` or `None` ]). | ||
| """ | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
I think we can drop this from the new API. If its equivalent in the old API wasn't deprecated, I think it should have been, as RFC-879 included deprecating all support for components in query APIs.
There was a problem hiding this comment.
Registry.queryDatasets's component parameter is documented as deprecated, I guess that makes that attribute deprecated too. I'll drop it in the new API and I can explicitly document it as deprecated in old API.
python/lsst/daf/butler/_butler.py
Outdated
| @@ -1357,3 +1363,275 @@ def registry(self) -> Registry: | |||
| will be replaced by equivalent `Butler` methods. | |||
| """ | |||
| raise NotImplementedError() | |||
|
|
|||
| @abstractmethod | |||
| def query(self) -> AbstractContextManager[Query]: | |||
There was a problem hiding this comment.
Can we make this private for now, too? While I'm not super worried about this method changing in backwards-compatible ways (it might get additional arguments, but they could be default), I do think we will want to tinker with the Query class APIs further as we actually implement more kinds of chaining, and it's easier to put the wall here than on every single method there.
There was a problem hiding this comment.
I guess I should also remove news fragment as we are hiding new API from users.
There was a problem hiding this comment.
Keep it in with the underscores and when I make the release notes I can decide what to do with it.
andy-slac
force-pushed
the
tickets/DM-41761
branch
3 times, most recently
from
034ae8d to
f90d27d
Compare
This new method defines an interface for making complex queries on repository database. This initial commit includes only three convenience methods that are a close match to the query methods that already exist in Registry. The implementation of that interface for DirectButler just forwards calls to SqlRegistry. RemoteButler does not implement those methods yet, we will need to define representation for queries and results that client exchanges server. This should become clearer as we add more methods to `Query`.
As we do not have non-UUID manager anymore, there is no point keeping two dataset.yaml files.
Many tests were just copied from `RegistryTests` to `ButlerQueryTests` replacing calls to registry methods with matching calls to `Butler.query` methods. Tests are implemented for DirectButler only, for both SQLite and Postgres backends.
andy-slac
force-pushed
the
tickets/DM-41761
branch
from
f90d27d to
0e06fc3
Compare
This new property defines an interface for making complex queries
on repository database. This initial commit includes only three
convenience methods that are a close match to the query methods that
already exist in Registry. The implementation of that interface
for DirectButler just forwards calls to SqlRegistry.
RemoteButler does not implement those methods yet, we will need
to define representation for queries and results that client exchanges
server. This should become clearer as we add more methods to
Query.Checklist
doc/changes