DM-43526: Implement registry collection methods for RemoteButler #983
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
==========================================
+ Coverage 88.92% 88.94% +0.01%
==========================================
Files 329 329
Lines 42351 42442 +91
Branches 8704 8716 +12
==========================================
+ Hits 37660 37749 +89
Misses 3441 3441
- Partials 1250 1252 +2 ☔ View full report in Codecov by Sentry. |
dhirving
force-pushed
the
tickets/DM-43526
branch
from
efa42d4
to
d1cb5fd
Compare
dhirving
force-pushed
the
tickets/DM-43526
branch
from
85a3bbc
to
9aeab89
Compare
|
|
||
| def setCollectionChain(self, parent: str, children: Any, *, flatten: bool = False) -> None: | ||
| raise NotImplementedError() | ||
|
|
||
| def getCollectionParentChains(self, collection: str) -> set[str]: | ||
| raise NotImplementedError() | ||
| info = self._butler._get_collection_info(collection, include_parents=True) | ||
| assert info.parents is not None |
There was a problem hiding this comment.
assert does trigger a philosophical debate here as to what happens if it ever fails. The error message that you get is never helpful. Consider RuntimeError with an error message instead? (or at least add a message to the assert).
| @@ -271,7 +281,15 @@ def queryCollections( | |||
| flattenChains: bool = False, | |||
| includeChains: bool | None = None, | |||
| ) -> Sequence[str]: | |||
| raise NotImplementedError() | |||
| if includeChains is None: | |||
There was a problem hiding this comment.
@TallJimbo what's the plan for the datasetType parameter? It's not in this implementation and is ignored in sql registry, but should the client code forward it to the server on the assumption that at some point the direct butler might use it? Or are we okay acting like it doesn't exist (the documentation doesn't seem to say that it's ignored and sql_registry mentions DM-24939 which is Done but hasn't fixed the problem).
There was a problem hiding this comment.
If it gets implemented in DirectButler someday it can get added here at the same time. It sounds like we're leaning towards re-designing this interface entirely, so we can drop it from the new interface or make it actually work there.
I don't like having unused variables/features hanging around because their existence implies to future readers that they are used for something, which can end up wasting a lot of time.
There was a problem hiding this comment.
I think we drop it here, and RFC dropping it from DirectButler (and the CLI) when we RFC dropping regex support there.
SqlRegistry is no longer a public interface, and _get_collection_record is used outside SqlRegistry a few places, so removing the leading underscore for clarity.
Implement additional registry methods for RemoteButler
Implement registry.getCollectionDocumentation and registry.getCollectionParentChains for RemoteButler.
dhirving
force-pushed
the
tickets/DM-43526
branch
from
9aeab89
to
6d4ae7a
Compare
Checklist
doc/changesconfigs/old_dimensions