-
Notifications
You must be signed in to change notification settings - Fork 14
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-35396: Add provenance hooks for datastore to use #1147
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1147 +/- ##
=======================================
Coverage 89.47% 89.47%
=======================================
Files 366 367 +1
Lines 49115 49185 +70
Branches 5955 5960 +5
=======================================
+ Hits 43944 44007 +63
- Misses 3753 3759 +6
- Partials 1418 1419 +1 ☔ View full report in Codecov by Sentry. |
a8247c8
to
880b214
Compare
880b214
to
ff6ea4a
Compare
0a3bb85
to
7ddf988
Compare
7ddf988
to
bcdbff4
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 great, a couple of questions.
@@ -453,11 +454,11 @@ def dimensions(self) -> DimensionUniverse: | |||
# Docstring inherited. | |||
return self._dimensions | |||
|
|||
def put(self, obj: Any, ref: DatasetRef, /) -> DatasetRef: | |||
def put(self, obj: Any, ref: DatasetRef, /, provenance: DatasetProvenance | None = None) -> 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.
def put(self, obj: Any, ref: DatasetRef, /, provenance: DatasetProvenance | None = None) -> DatasetRef: | |
def put(self, obj: Any, ref: DatasetRef, /, *, provenance: DatasetProvenance | None = None) -> DatasetRef: |
to make it consistent with LimitedButler?
def put(self, inMemoryDataset: Any, datasetRef: DatasetRef) -> None: | ||
def put( | ||
self, inMemoryDataset: Any, datasetRef: DatasetRef, provenance: DatasetProvenance | None = None | ||
) -> None: | ||
raise NotImplementedError("This is a no-op datastore that can not access a real datastore") | ||
|
||
def put_new(self, in_memory_dataset: Any, ref: DatasetRef) -> Mapping[str, 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.
Hmm, we should probably drop put_new
if we are not using it. It's not tested and is probably broken already. Not on this ticket, of course.
@@ -169,6 +174,7 @@ def __init__( | |||
ref: DatasetRef, | |||
write_parameters: Mapping[str, Any] | None = None, | |||
write_recipes: Mapping[str, Any] | None = None, | |||
provenance: DatasetProvenance | None = None, |
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.
Does it need to be a constructor argument, could you pass it to write
instead?
if dataset_id not in self.extras: | ||
self.extras[dataset_id] = {} | ||
self.extras[dataset_id].update(extra) |
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.
Maybe
self.extras.setdefault(dataset_id, {}).update(extra)
?
bcdbff4
to
37dd1a7
Compare
No-op in the base class but can be used by subclasses to add provenance to composites that are to be disassembled.
f6b4efc
to
afb16c5
Compare
Checklist
doc/changes
configs/old_dimensions