DM-41165: Inconsistent handling of expanded datasetRefs

[kfindeisen]

This PR fixes a bug where some Registry objects require expanded datasetRefs for transfers and others do not, but refs would not be expanded when necessary because the transfer code tried to be clever and second-guess the spec. The formal spec is unchanged, but the code is now more robust about following it.

I have not tried to improve daf_butler's test coverage to catch this bug, as the intersecting variables (SqlRegistry vs. other implementations, ObsCore vs. non-ObsCore registries, expanded vs. non-expanded data IDs) are too complex for me to follow. Any pointers on where this should be covered would be appreciated.

Checklist

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

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (9d5d565) 87.60% compared to head (2eae16f) 87.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
- Coverage   87.60%   87.60%   -0.01%     
==========================================
  Files         272      272              
  Lines       36600    36600              
  Branches     7641     7641              
==========================================
- Hits        32064    32062       -2     
- Misses       3350     3351       +1     
- Partials     1186     1187       +1     

Files	Coverage Δ
python/lsst/daf/butler/_butler.py	88.72% <ø> (ø)
python/lsst/daf/butler/direct_butler.py	79.36% <100.00%> (ø)
python/lsst/daf/butler/registry/_registry.py	96.72% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TallJimbo]

Looks good. I fully agree with your diagnosis and fix.

As for testing, one thing you don't have to worry about is Registry implementations other than SqlRegistry; one of the decisions coming out of the middleware meeting a couple of weeks ago is to collapse those back into a single final/concrete Registry. But there are indeed lots of ways a registry (or a butler, for that matter) can be configured and testing all of the various permutations is unsurprisingly difficult.

But I don't see any tests right now that both set up a full Butler and enable the ObsCore stuff - test_obscore.py only makes a Registry and then operates directly on that. We're going to want to address that at some point, because we're also going to move in the direction of putting more public interfaces directly on Butler (rather than Butler.registry) and hence that's what we'll want to test in places like test_obscore.py. But doing that on this ticket is a heavy lift. I'm okay with punting test coverage for this bugfix to another ticket (e.g. one mostly about migrating registry-focused tests to butler-focused tests). @andy-slac, any thoughts?

[TallJimbo]

Thinking a bit more, I wonder if this says that something down in the ObsCore implementation should also be able to expand its own data IDs; if it's not too inefficient, that would make it easier to test and (more importantly) make overall Registry behavior much less dependent on whether ObsCore is enabled. Curious to hear @andy-slac 's thoughts on that, too.

[andy-slac]

Doing that at obscore plugin level will likely be inefficient, and probably non-trivial (obscore plugin would have to depend on other things at registry level). Still there will be an opportunity to revisit obscore implementation when we do big refactoring, we can think about it then. For now I think we can merge this ticket and remember that there may be potential optimizations.

[andy-slac]

Regarding test_obscore.py and testing with full butler - obscore is just one of the registry managers, so it should not depend on hoe butler is set up, it only cares about registry itself. Of course as we move things to Butler we'd have to think what happens with all managers too, but this is certainly not a concern for this ticket.

[kfindeisen]

Thanks for the tip about Registry/SqlRegistry, but the fundamental testing problem is still the proliferation of tagged classes and the way those tags (in this case, ObsCore-ness and expanded-ness) interact with each other. Perhaps we need a separate suite of unit tests that run high-level operations on the various permutations? From what I saw, test_obscore.py focused almost entirely on ObsCoreTableManager itself, and not on how other classes use it (i.e., we're missing tests of Butler, not tests of ObsCore).

From a contract point of view, ObsCore requiring expanded IDs in its API was not the problem -- the implementation of _importDatasets already assumed that its refs were expanded, and it was _importDatasets's contract that was violated. We just didn't notice until ObsCore came along.

[TallJimbo]

Perhaps we need a separate suite of unit tests that run high-level operations on the various permutations?

We definitely do. It's just difficult right now because the set of permutations we test now (registries with different database backends, butlers with different kinds of datastores) is incomplete, already quite large, and about to be quite obsolete (since we are collapsing registry the registry hierarchy, expanding polymorphism in the butler hierarchy, and dropping InMemoryDatastore). We have a lot of work to do on the tests.

[kfindeisen]

Should I open a ticket for that (to be blocked by the big redesigns)?

[TallJimbo]

Yes, I think that makes sense, especially since it seems there's a particular combination of operations and configurations that are quite important to Prompt Processing that got missed in our test matrix so far, and it'd be good to get your best guess at that combination in writing somewhere.

[kfindeisen]

Done as DM-41234, though I'm not sure I fully answered the question about "particular combination".