-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
eff98ab
to
6f038f4
Compare
Codecov ReportAttention:
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. |
6f038f4
to
cca5c5d
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.
# Docstring inherited. | ||
raise NotImplementedError() | ||
if predict or run: | ||
raise NotImplementedError("Predict mode is not supported by RemoteButler") |
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.
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 |
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.
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, |
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.
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.
cca5c5d
to
6dc29dc
Compare
The newly-released Documenteer 1.0 requires additional configuration and does not support the pipelines.lsst.io theme yet.
Checklist
doc/changes