-
Notifications
You must be signed in to change notification settings - Fork 5
fix premature model construction #138
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
fix premature model construction #138
Conversation
| result: GroupSpec | ||
| attributes = group.attrs.asdict() | ||
| members: dict[str, object] = {} | ||
| members: dict[str, dict[str, object]] = {} |
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.
Should this not be dict[str, GroupSpec | ArraySpec], and pydantic will handle the casting if it's given dict[str, object] as input?
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.
we are filling members with dicts produced by ArraySpec.model_dump() / GroupSpec.model_dump(). If we don't do that, then pydantic will reject models that declared specialized members, because pydantic uses nominal typing (isinstance checks) when the input is a basemodel
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.
Won't the specialised members be sub-classes of ArraySpec or GroupSpec though, and therefore would pass the isinstance check?
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.
this is what will happen:
from pydantic_zarr.experimental.v3 import ArraySpec, GroupSpec
import numpy as np
class MyArray(ArraySpec):
...
class MyGroup(GroupSpec):
members: dict[str, MyArray]
MyGroup(attributes={},members={'a': ArraySpec.from_array(np.arange(10))})
"""
pydantic_core._pydantic_core.ValidationError: 1 validation error for MyGroup
members.a
Input should be a valid dictionary or instance of MyArray [type=model_type, input_value=ArraySpec(zarr_format=3, ...), dimension_names=None), input_type=ArraySpec]
For further information visit https://errors.pydantic.dev/2.11/v/model_type
"""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.
isinstance(ArraySpec(...), SpecializedArraySpec) will fail, which is why we need to turn the arrayspec / groupspec members into dicts
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.
Okay - sorry, this is somewhat off-topic on this PR
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.
no worries!
… into fix/deserialize-in-one-place
…tests that rely on zarr indirectly
2 fixes in this PR:
from_zarrto work by constructing the final group from dicts instead of arrayspec / groupspec instances