Skip to content
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-41235: Make SqlRegistry a concrete class #898

Merged
merged 5 commits into from Oct 26, 2023
Merged

Conversation

andy-slac
Copy link
Contributor

SqlRegistry un-inherits Registry and drops an intermediate _ButlerRegistry
interface. All clients of _ButlerRegistry now use SqlRegistry. Registry is
still an interface for client-facing Butler.registry, which is a shim for
SqlRegistry or other butler methods.

Checklist

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

Updated test_server.py to skip all tests as it depends on RemoteRegistry.
We will fix the tests later using new shiny client/server Butler.
SqlRegistry un-inherits Registry and drops an intermediate _ButlerRegistry
interface. All clients of _ButlerRegistry now use SqlRegistry. Registry is
still an interface for client-facing `Butler.registry`, which is a shim for
SqlRegistry or other butler methods.
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d9d7cda) 87.59% compared to head (a6005bf) 87.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #898      +/-   ##
==========================================
- Coverage   87.59%   87.23%   -0.37%     
==========================================
  Files         272      270       -2     
  Lines       36584    36345     -239     
  Branches     7637     7598      -39     
==========================================
- Hits        32046    31704     -342     
- Misses       3351     3484     +133     
+ Partials     1187     1157      -30     
Files Coverage Δ
python/lsst/daf/butler/direct_butler.py 79.39% <100.00%> (+0.02%) ⬆️
python/lsst/daf/butler/registry/__init__.py 100.00% <ø> (ø)
python/lsst/daf/butler/registry/_defaults.py 94.23% <100.00%> (ø)
python/lsst/daf/butler/registry/_registry.py 98.23% <100.00%> (+1.50%) ⬆️
python/lsst/daf/butler/registry/tests/_registry.py 98.22% <100.00%> (ø)
python/lsst/daf/butler/tests/_datasetsHelper.py 88.33% <100.00%> (ø)
python/lsst/daf/butler/transfers/_context.py 87.82% <100.00%> (-0.11%) ⬇️
python/lsst/daf/butler/transfers/_yaml.py 88.00% <100.00%> (+0.06%) ⬆️
tests/test_butler.py 97.94% <100.00%> (ø)
tests/test_cliCmdPruneDatasets.py 100.00% <100.00%> (ø)
... and 7 more

... and 10 files with indirect coverage changes

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

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay although I am now wondering what the purpose of Registry ABC is.

for filename in filenames:
with open(os.path.join(TESTDIR, "data", "registry", filename)) as stream:
# Go behind the back of the import code a bit to deal with
# the fact that this is just registry content with no actual
# files for the datastore.
backend = YamlRepoImportBackend(stream, butler.registry)
backend = YamlRepoImportBackend(stream, butler._registry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because butler.registry is no longer a registry but butler._registry is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly because I changed YamlRepoImportBackend to accept SqlRegistry instead of Registry. I think that Registry at this point exists only because we need backward-compatible interface for Butler.registry. When we move all functionality to Butler we will eventually remove Registry completely.

@andy-slac
Copy link
Contributor Author

Looks okay although I am now wondering what the purpose of Registry ABC is.

This is for clients that still use Butler.registry. It does not have to be an ABC, I just kept it with minimal changes. I could merge Registry and RegistryShim if you prefer.

@andy-slac
Copy link
Contributor Author

andy-slac commented Oct 26, 2023

I could merge Registry and RegistryShim if you prefer.

OTOH if we need registry interface for remote butler then its implementation will likely be different from RegistryShim. We probably should keep Registry ABC for now just in case.

@andy-slac andy-slac merged commit dd9cc5e into main Oct 26, 2023
15 of 16 checks passed
@andy-slac andy-slac deleted the tickets/DM-41235 branch October 26, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants