-
Notifications
You must be signed in to change notification settings - Fork 14
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-39944: Replace Butler.registry with registry shim #864
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
========================================
Coverage 87.90% 87.90%
========================================
Files 270 273 +3
Lines 35633 35764 +131
Branches 7462 7474 +12
========================================
+ Hits 31324 31440 +116
- Misses 3151 3166 +15
Partials 1158 1158
☔ View full report in Codecov by Sentry. |
It looks like I missed a butler.datastore fix in the tests since we get 22 deprecation warnings. It's interesting that python 3.11 is now significantly faster than 3.10. |
😄 the warnings about |
I likely messed it up when rebased after your changes, will check everything again. |
This is a trivial change, internally Butler uses `_registry` but it is also exposed as `registry` property. In the next commits `registry` property will return a special shim object, this will allow us to transparently update registry implementation without breaking client-visible interfaces.
`Registry` ABC now contains methods used by regular registry clients, this interface is exposed via `Butler.registry` property. `ButlerRegistry` ABC extends `Registry` with methods that are used by Butler, this includes factory methods and a couple of internal methods. SQL and remote implementations now inherit `ButlerRegistry`, they are not changed otherwise. Also dropped `Registry.datasetIdFactory` attribute, we now have more transparent method for generating dataset IDs and this attribute is not used.
ce6e9c9
to
429e7af
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.
Looks good! This is all consistent with how I see things going, at least at first.
I think we're in agreement that the roles of some of the new classes will change significantly before this is all over, and they may well disappear. So how about we add a leading underscore to ButlerRegistry
and RegistryFactory
to make it extra clear that these are not symbols anyone should be depending on.
Did you see any usage of createFromConfig
in tests downstream of daf_butler, or was it just a lot of tests in daf_butler itself? That's currently at the top of my list of Registry
methods we should actually deprecate (in favor of ButlerRegistry
, for now), with transaction
and resetConnectionPool
close behind. There are of course a ton of other methods we want to move to Butler
, maybe adjust, and ultimately remove from Registry
in the distant future, but we should probably be more aggressive about removing those three and maybe a few others ASAP since we don't expect to even give them Butler
-method replacements.
(defaulting to ``SqlRegistry``), import that class, and call one of the | ||
above methods on the imported class. | ||
""" | ||
|
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.
Would it make sense to give this class a constructor that does the work of force_registry_config
and holds the config as an attribute, and then make the other methods regular instance methods (probably abstract ones)?
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.
Adding a constructor looks reasonable. If factory methods are abstract we'd need them implemented somewhere? Do you think that this should be another base class for ButlerRegistry subclasses?
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.
Ah, I think I got confused about the current state of things while reviewing commit-by-commit - I thought this still had some methods with docs that said they needed to be reimplemented by derived classes, and that made me think there were already subclass implementations and I didn't step back and notice that there weren't any.
So just ignore the bit about abstract methods; this is just a regular concrete (probably final) class, and that's fine.
This new class implements `Registry` interface and forwards all methods to other objects. Everything is forwarded to `Butler._registry` now, but this will change as we start refactoring ButlerRegistry implementation.
Trying to further separate concerns into independent classes. Re-introduced `Registry.createFromConfig` to avoid updating tests in many other packages.
429e7af
to
020fe5b
Compare
I renamed those two classes.
There are bout five other packages that use
I looked at |
Registry
ABC is split into three classes:Registry
is now only contains the methods that are used by regular clients viaButler.registry
attributeButlerRegistry
ABC extendsRegistry
with few methods used by Butler internally, SQL and remote implementations now inheritButlerRegistry
ButlerFactory
class has methods to create/instantiate registry from configuration. There is stillRegistry.createFromConfig
, this is only for the tests that currently use this method, I do not want to update all those as I'm not sure ifButlerFactory
survives all further updates (or this review).Butler.registry
now returnsButlerShim
instance which is a subclass of Registry. Shim for now just forwards all calls toButler._registry
, as we do updates to Butler/Registry implementations the shim will probably do more complicated things, and eventually it should disappear after all clients migrate to new Butler API.Checklist
doc/changes