-
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-41326: remove DimensionGraph and related interfaces #1036
Conversation
53f1611
to
8a99594
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. Impressive amount of cleanup work.
@@ -748,8 +742,6 @@ def from_simple( | |||
match simple.dimensions: |
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.
Seems like match
is now overkill if this is now doing:
if simple.dimensions is None:
raise(...)
dimensions = universe.conform(simple.dimensions)
@@ -109,8 +109,6 @@ def direct( | |||
match dimensions: |
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.
serialized_dimensions = dimensions if dimensions else None
? Or are you worried that the type really might not be correct?
I think the type annotation for this method needs to remove the dict
now.
@@ -59,7 +59,7 @@ class SerializedDatasetType(BaseModel): | |||
|
|||
name: StrictStr | |||
storageClass: StrictStr | None = None | |||
dimensions: SerializedDimensionGraph | list[StrictStr] | None = None | |||
dimensions: list[StrictStr] | 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.
I see that this leads to any SerializedDatasetType JSON we have lying around being incompatible if it had the graph in it. People shouldn't have that anywhere (not even in older graphs?) but it probably means that we need to ensure the butler server deployments are updated so they don't send back the wrong form of SerializedDatasetType.
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.
That's right, but we've been writing this with list
for a while now, so butler server should be fine, and I figured any graphs old enough to suffer from this would probably be impossible to read already due to other incompatibilities (though I don't think I can prove that in general).
I see there is a failure:
which is great because Fabio reported that failure this morning when building the new weekly. This means there is a chance it can be debugged more easily. |
e36c185
to
43d8e5f
Compare
43d8e5f
to
73ae713
Compare
Checklist
doc/changes
configs/old_dimensions