Use dataclass instead of NamedTuple for serializing AssetCondition #21147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
That makes sense. If we move away from NamedTuple, should we consider adopting DagsterModel instead of dataclass? Some additional context here and here, when we did the change for AssetSelection. |
alangenfeld
approved these changes
Apr 11, 2024
|
Thanks for the info re: DagsterModel -- I'll do that in a followup PR today, just to decouple it from the release timeline, but seems like a good idea |
yuhan
pushed a commit
that referenced
this pull request
Apr 11, 2024
…21147) ## Summary & Motivation We're about to start serializing these things, so making a diving save to allow us to use the nicer dataclass API instead of NamedTuples. In the process of doing this, I realized that we had a weird setup with a `children` property on the base AssetCondition class that didn't work when using dataclasses. Previously, using NamedTuples meant that the NamedTuple property management stuff would handle the case where you added an explicit `children` field on the child class, but dataclasses don't work in the same way. Basically we want a few things: - To have a property on all AssetConditions that lists all sub-conditions (if any) - To not have to explicitly store a set of sub-conditions in serialized form if there aren't any (mostly a dev ergonomics thing, it's a pain to add a guaranteed-to-be-empty "children" property to all child classes) - To make it easy to set invariants that certain AssetConditions will only have a single child, etc. This new system makes it so that AssetConditions that *do* have sub-conditions just store them in a field with a different name (currently we're just doing boolean expressions, so I've used "operand" / "operands"), and then implement an explicit "children" property on top of that field. AssetConditions without sub-conditions just use the default "children" implementation, which returns an empty list. Overall, this setup works a lot nicer and simplifies the class definitions for all these subclasses. ## How I Tested These Changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
We're about to start serializing these things, so making a diving save to allow us to use the nicer dataclass API instead of NamedTuples.
In the process of doing this, I realized that we had a weird setup with a
childrenproperty on the base AssetCondition class that didn't work when using dataclasses. Previously, using NamedTuples meant that the NamedTuple property management stuff would handle the case where you added an explicitchildrenfield on the child class, but dataclasses don't work in the same way.Basically we want a few things:
This new system makes it so that AssetConditions that do have sub-conditions just store them in a field with a different name (currently we're just doing boolean expressions, so I've used "operand" / "operands"), and then implement an explicit "children" property on top of that field. AssetConditions without sub-conditions just use the default "children" implementation, which returns an empty list.
Overall, this setup works a lot nicer and simplifies the class definitions for all these subclasses.
How I Tested These Changes