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-42187: Add RemoteButler.getURIs #924

Merged
merged 3 commits into from Dec 19, 2023
Merged

DM-42187: Add RemoteButler.getURIs #924

merged 3 commits into from Dec 19, 2023

Conversation

dhirving
Copy link
Contributor

Checklist

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

@dhirving dhirving force-pushed the tickets/DM-42187 branch 2 times, most recently from eff98ab to 6f038f4 Compare December 15, 2023 17:32
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

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

Comparison is base (ee20ead) 87.82% compared to head (3325adb) 87.84%.

Files Patch % Lines
...on/lsst/daf/butler/remote_butler/_remote_butler.py 93.54% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #924      +/-   ##
==========================================
+ Coverage   87.82%   87.84%   +0.01%     
==========================================
  Files         295      295              
  Lines       38379    38429      +50     
  Branches     8115     8118       +3     
==========================================
+ Hits        33707    33757      +50     
  Misses       3476     3476              
  Partials     1196     1196              

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

@dhirving dhirving marked this pull request as ready for review December 15, 2023 17:47
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 good.

# Docstring inherited.
raise NotImplementedError()
if predict or run:
raise NotImplementedError("Predict mode is not supported by RemoteButler")
Copy link
Member

Choose a reason for hiding this comment

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

We need to discuss whether we can make this work. Obviously a signed URL to a resource that doesn't exist is not all that helpful but people like to know what the filename might look like. We can tell them that in theory -- maybe for predict mode we return the unsigned s3 URI? Can punt this to another ticket.

# Add a file with corrupted data for testing error conditions
cls.dataset_with_corrupted_data = _create_corrupted_dataset(cls.repo)
# All of the datasets that come with MetricTestRepo are disassembled
Copy link
Member

Choose a reason for hiding this comment

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

I had forgotten that the MetricTestRepo defaults to storage class StructuredCompositeReadComp. Easy to change the default if we wanted to but also nice to know that getURIs should work.

with self.assertRaises(RuntimeError):
self.butler.getURI(dataset_type, dataId=data_id, collections=collections)

# getURIs does NOT respect component overrides on the DatasetRef,
Copy link
Member

Choose a reason for hiding this comment

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

There is only one URI for any of the components if not disassembled. I imagine no-one has ever tried to request the URI of an individual component by passing in the ref but it seems like it should return the one URI of the component.

This re-uses the existing server endpoints for getting file info.
Moved the DirectButler implementation to the Butler base class, since the code is identical for RemoteButler.
The newly-released Documenteer 1.0 requires additional configuration and does not support the pipelines.lsst.io theme yet.
@dhirving dhirving merged commit 480261b into main Dec 19, 2023
18 checks passed
@dhirving dhirving deleted the tickets/DM-42187 branch December 19, 2023 19:43
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