DM-38210: Make butler.get act like getDirect and deprecate getDirect

[timj]

We now want a resolved DatasetRef to work with butler.get without doing registry lookup. Also change the LimitedButler to add a .get(). .getDirect and deferred form are now deprecated.

Checklist

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

[timj]

@TallJimbo can you take a quick look at this to see if it matches your expectations? Obviously a lot of the checks for .id is not None will go away when the other half of the deprecation is done but it seems like getting this deprecation for getDirect out the way early would be a good idea. I need to fix the other lsst_distrib packages.

[codecov]

Codecov Report

Patch coverage: 83.33% and project coverage change: +0.04 🎉

Comparison is base (77f0486) 87.64% compared to head (30e075b) 87.69%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #797      +/-   ##
==========================================
+ Coverage   87.64%   87.69%   +0.04%     
==========================================
  Files         267      267              
  Lines       34706    34700       -6     
  Branches     7294     7299       +5     
==========================================
+ Hits        30417    30429      +12     
+ Misses       3136     3117      -19     
- Partials     1153     1154       +1     

Impacted Files	Coverage Δ
python/lsst/daf/butler/script/removeCollections.py	100.00% <ø> (ø)
python/lsst/daf/butler/script/removeRuns.py	100.00% <ø> (ø)
python/lsst/daf/butler/_butler.py	78.53% <61.90%> (+3.00%)	⬆️
python/lsst/daf/butler/_limited_butler.py	79.59% <68.75%> (-11.59%)	⬇️
python/lsst/daf/butler/_quantum_backed.py	89.07% <80.00%> (-0.71%)	⬇️
python/lsst/daf/butler/_deferredDatasetHandle.py	90.00% <100.00%> (ø)
tests/test_butler.py	97.62% <100.00%> (ø)
tests/test_logFormatter.py	100.00% <100.00%> (ø)
tests/test_quantumBackedButler.py	99.06% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TallJimbo]

Looks good in terms of behavior, but I'd like to handle the signatures in LimitedButler slightly differently.

datasetExistsDirect should probably also go away, and the getURI[s] are probably worth a look to see if they need modifications (my recollection is that they already have behavior consistent with the new get behavior, but I'm not sure).

[timj]

the getURI[s] are probably worth a look

getURI uses _findDatasetRef so will now work directly with a resolved ref.

datasetExistsDirect should probably also go away

This begs the question of whether we are "fixing" datasetExists such that it only returns a boolean rather than raising if registry doesn't know about the dataset. In DM-32940 you suggest we change that to return an enum that can evaluate as a boolean. Then datasetExistsDirect could go away. We have known to registry, known to datastore, actually exists on "disk" as options but only the last one is True.

I assume putDirect should disappear as well.

[TallJimbo]

👍 to removing putDirect.

What to do about datasetExists is trickier. I tried an enum on the DMTN-249 prototyping and while I think it might have worked fine for full Butler alone, it didn't make sense for LimitedButler where there's no concept of registry existence. And if you have something that checks for datastore existence only in LimitedButler, that doesn't make sense in full Butler, where Datastore-only existence checks are designed to almost never be necessary.

So I think our best bet is to give the LimitedButler datastore-only method a somewhat obscure, very precise name, and reserve exists or datastoreExists as a full-Butler method that returns an enum, but we might need to experiment on that a bit, or gather feedback from users.

[TallJimbo]

I'd rather not have a storageClass argument here, since the caller can override the registry storage class via ref.datasetType.storageClass already and I think that makes it more confusing than convenient (especially since right now LimitedButler mostly exists just for task execution and that doesn't use this parameter at all). I think that argument is even more applicable to Datastore.get, since that really isn't in need of convenience functionality since only butler should call it, but that may be out of scope for this ticket.

To be clear, I do think it's worth having a storageClass parameter to full Butler.get, since there it's very convenient when not passing a DatasetRef, and then since it'll present anyway it might as well further override what's in the ref when a DatasetRef is passed.

[timj]

My main argument for keeping it is that it's forwarded directly to datastore and datastore will use it and treat it as the priority. It seems odd to declare that the base class can't support the parameter even when it can.