[Transition PyPIConGPU to pydantic] Proof-of-concept I: Remove _get_serialized
#5501
Conversation
chillenzer
added
refactoring
chillenzer
changed the title
_get_serialized_get_serialized
| try: | ||
| # First case: `to` is a class and can be constructed with a fully-qualified constructor call (pydantic.BaseModel). | ||
| return to(**assignments) | ||
| except Exception: |
There was a problem hiding this comment.
We could maybe also at least check if to is a subclass of pydantic.BaseModel, so that we are sure it is nothing with sth like def __init__(**kwargs): pass :D
There was a problem hiding this comment.
I don't think we should. There's no implication in either direction:
I feel that it's very reasonable to assume that MyClass(attribute1=1, attribute2=2) constructs an instance of MyClass that follows the semantics of holding/containing/... attribute1=1 and attribute2=2. It might well be that its implementation does not actually store them as member variables which is totally fine with me. But whatever it does internally, if it doesn't represent something like "filled with attribute1=1 and attribute2=2" after construction, the author of that class is clearly misusing the constructor.
The other way round, you can provide custom __init__ functions to pydantic.BaseModel children, so they could equally well mess with whatever's going on there.
There is actually one thing fishy about what you highlighted, though: The except Exception is clearly too general. In #5502, I actually had to change that to the more appropriate TypeError that's thrown if the constructor doesn't accept any or all of the arguments passed. I could rebase that change to this PR already.
There was a problem hiding this comment.
Alright, I see your point. I agree that the Exception is a bit too broad :D Rebase sounds good.
| with self.assertRaises(typeguard.TypeCheckError): | ||
| g.cell_size_si = (1.2, 2.3, "126") | ||
| with self.assertRaises(typeguard.TypeCheckError): | ||
| g.cell_cnt = (11.1, 7, 8) |
There was a problem hiding this comment.
sth like this should still fail I think? Or what do you think?
There was a problem hiding this comment.
Actually, that's true. I'll leave them in.
There was a problem hiding this comment.
Funnily enough, upon closer inspection the first of the highlighted test cases is actually obsolete now because cell_size_si can be any float and can be converted from string. However, the second test case had a sibling below which should both still apply but I got a type annotation wrong in grid.py, so they didn't fire. All fixed now!
| raise ValueError(f"Unknown serialization for {spec=} as a time step specifier (--period argument).") | ||
| class Spec(BaseModel): | ||
| start: Annotated[int | None, PlainSerializer(lambda x: x or 0)] | ||
| stop: Annotated[int | None, PlainSerializer(lambda x: x or -1)] |
There was a problem hiding this comment.
With stop=0 (which is weird, but I think should be valid?) this returns -1
Also with step=0 it returns 1. I feel like these cases should fail and not silently change. Maybe do not rely on None being falsly but explicitly check for is None. Or, maybe we do not allow these to be zero somewhere else? Anyway, I like is None more :)
There was a problem hiding this comment.
You are correct. It was infuriatingly verbose in the version before and I remembered I couldn't get it shorter back then. I already wondered why I managed to do so this time. Thanks for catching it!
|
@chillenzer, Just the broad Exception and the serializer in Spec are left. The rest looks good to me. |
|
Thanks! I'm on it! |
chillenzer
force-pushed
the
transition-pypic-to-pydantic-poc-pydantic
branch
from
4bde411 to
dde1b16
Compare
|
All requested changes are integrated. I've taken the liberty to squash them into the last commit under the grand theme of "fixing things". |
2 participants
This is the first of a series of PRs relating to #5500.
This introduces 3 new functionalities:
copy_attributeswill first attempt to use a constructor to instantiate a class with all the necessary information. This is necessary becausepydantic.BaseModels want to be fully and validly instantiated.get_rendering_contextwill fall back tomodel_dumpif it doesn't find_get_serialized.SelfRegisteringto work withpydantic.BaseModels. They sure love dark Python voodoo as much as we do. -.-Afterwards it includes two examples:
Grid3D: Straightforward class but includes some custom transformations and renamings. Medium-term strategy might involve adjusting (names in) the templates to reduce the necessity for such conversion logic but with the laid-out constraints in Transition PyPIConGPU to Pydantic #5500 this workaround is the best we can get.Autoplugin: This one was a little trickier. It includes a non-trivially typed member (TimeStepSpec) which had to be converted to Pydantic as well and also relies on the self-registering feature.Some tests were adjusted to reflect the new interface and more forgiving interface.
Each commit consists of one of the mentioned above and you might want to review each commit individually.