Revert "Revert "MetadataValue subclasses to DagsterModel (#21324)" (#… #21543
Conversation
There was a problem hiding this comment.
Outside the __init__s that coerce inputs like [1] most of them like [2] seem like they could be omitted. Am I missing something?
I think pretty much every object is either a [1] (coerces input but property type does not reflect) or [2] (maybe not needed __init__)
| return super(TextMetadataValue, cls).__new__( | ||
| cls, check.opt_str_param(text, "text", default="") | ||
| ) | ||
| text_inner: Optional[str] = Field(..., alias="text") |
There was a problem hiding this comment.
[1] this object property (and @propertys) could be str since we coerce None to "" in the __init__ that takes Optional[str]
| def __init__(self, text: Optional[str]): | ||
| super().__init__(text=text or "") |
There was a problem hiding this comment.
^ or we could just drop this custom __init__ and let the None flow through since thats how everything is typed
There was a problem hiding this comment.
I agree that this is weird - this was just me trying to minimize changes to the existing behavior.
There was a problem hiding this comment.
got it - consider dropping the unnecessary Optional in a follow up PR then I guess. Hard to argue that its that impactful of an improvement
| def __init__(self, path: Optional[Union[str, os.PathLike]]): | ||
| super().__init__(path=check.opt_path_param(path, "path", default="")) |
| def __init__(self, module: str, name: str): | ||
| super().__init__(module=module, name=name) |
They're needed to support positional arguments (for backcompat). The Pydantic-generated constructors only support keyword arguments. |
| def __init__(self, text: Optional[str]): | ||
| super().__init__(text=text or "") |
There was a problem hiding this comment.
got it - consider dropping the unnecessary Optional in a follow up PR then I guess. Hard to argue that its that impactful of an improvement
|
Some more discussion on the positional stuff here btw: #20461 |
fix-up side effects from the bug correction in #21543 to prevent non vanilla json objects from being part of JsonMetadatValue by adding special processing for the `data` field ## How I Tested These Changes added test
fix-up side effects from the bug correction in #21543 to prevent non vanilla json objects from being part of JsonMetadatValue by adding special processing for the `data` field ## How I Tested These Changes added test
fix-up side effects from the bug correction in dagster-io#21543 to prevent non vanilla json objects from being part of JsonMetadatValue by adding special processing for the `data` field ## How I Tested These Changes added test
…21462)"
This reverts commit 7b103db.
Summary & Motivation
This moves
MetadataValuesubclasses to also be subclasses ofDagsterModel, instead ofNamedTuples.This was originally reverted because it caused a test failure. This PR fixed the test failure: #21463. This PR further reduced the risk: #21531.
Original PR: #21324
How I Tested These Changes