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-39944: Update Execution butler builder to use Butler._registry #355

Merged
merged 4 commits into from Jul 14, 2023

Conversation

andy-slac
Copy link
Contributor

@andy-slac andy-slac commented Jul 12, 2023

Depends on lsst/daf_butler#864

Checklist

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

I hope execution butler will disappear sooner that we need to change
this to something else.
@@ -240,7 +240,7 @@ def _export(butler: Butler, collections: Iterable[str] | None, inserts: DataSetT
# export/import
BackendClass = get_class_of(butler._config["repo_transfer_formats", "yaml", "export"])
backend = BackendClass(yamlBuffer, universe=butler.dimensions)
exporter = RepoExportContext(butler.registry, butler._datastore, backend, directory=None, transfer=None)
exporter = RepoExportContext(butler._registry, butler._datastore, backend, directory=None, transfer=None)
Copy link
Member

Choose a reason for hiding this comment

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

It is looking suspiciously like RepoExportContext is not a public API...

Copy link
Contributor Author

@andy-slac andy-slac Jul 12, 2023

Choose a reason for hiding this comment

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

I think its instantiation is intended to happen inside Butler.export(), but execution butler does not like to use export() for some reason. I prefer to wait until we remove execution butler rather than fixing it.

Copy link
Member

Choose a reason for hiding this comment

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

I consider everything but construction of RepoExportContext to be public API, but construction definitely is not.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03 ⚠️

Comparison is base (1344297) 82.73% compared to head (3517803) 82.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   82.73%   82.70%   -0.03%     
==========================================
  Files          66       66              
  Lines        7268     7268              
  Branches     1418     1418              
==========================================
- Hits         6013     6011       -2     
- Misses       1007     1008       +1     
- Partials      248      249       +1     
Impacted Files Coverage Δ
python/lsst/pipe/base/tests/simpleQGraph.py 71.42% <ø> (ø)
python/lsst/pipe/base/connections.py 78.14% <100.00%> (ø)
python/lsst/pipe/base/executionButlerBuilder.py 66.21% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@timj
Copy link
Member

timj commented Jul 12, 2023

Sorry about the ruff problem. Not sure how I committed such a broken noqa statement last time.

@@ -240,7 +240,7 @@ def _export(butler: Butler, collections: Iterable[str] | None, inserts: DataSetT
# export/import
BackendClass = get_class_of(butler._config["repo_transfer_formats", "yaml", "export"])
backend = BackendClass(yamlBuffer, universe=butler.dimensions)
exporter = RepoExportContext(butler.registry, butler._datastore, backend, directory=None, transfer=None)
exporter = RepoExportContext(butler._registry, butler._datastore, backend, directory=None, transfer=None)
Copy link
Member

Choose a reason for hiding this comment

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

I consider everything but construction of RepoExportContext to be public API, but construction definitely is not.

@andy-slac andy-slac force-pushed the tickets/DM-39944 branch 2 times, most recently from daf4772 to 3517803 Compare July 14, 2023 16:20
@andy-slac andy-slac merged commit 1237108 into main Jul 14, 2023
13 of 14 checks passed
@andy-slac andy-slac deleted the tickets/DM-39944 branch July 14, 2023 16:25
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

3 participants