-
Notifications
You must be signed in to change notification settings - Fork 28
Add get_model_state to get validated doc.
#309
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
base: main
Are you sure you want to change the base?
Conversation
This adds `get_model_state` which returns the entire doc state as the pydantic model pass in as `Model=` during instantiation. It's effectively the other side of `apply_update`. If the doc has no Model defined it will raise a RuntimeError. If the doc is invalid it will raise a pydantic ValidationError. Also, update `apply_update` to use `model_validate` instead of Model(**value). This is more of a "style" thing. See pydantic/pydantic#9676 Also add ruff to the test dependencies and format.
| d = {k: twin_doc[k].to_py() for k in self._Model.model_fields} | ||
| try: | ||
| self._Model(**d) | ||
| self._Model.model_validate(d) |
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 the style change i refer to in the PR description.
|
@davidbrochart This actually doesn't work in it's current iteration because _roots gets updated lazily when calling get(). Have you had any thoughts about how _roots maybe could be kept in sync? I think either making _roots look at the rust state directly, or updating _roots when running Any thoughts? |
This iterates through the roots of the Doc, converting them into their native python types (using the underlying type's to_py() fn). tbd if this can be used to replace `_roots` as well?
5cf8926 to
1aa8dfc
Compare
|
Sorry @mnbbrown I'm a bit low on bandwidth currently, but excited to see what you're doing. |
No worries! I know how it is :) I think this is ready for a review now - I hope the description is sufficiently clear. Let me know if anything else needs explanation. |
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.
Thanks @mnbbrown, I left some minor changes and I have questions.
I'm wondering if get_model_state could be supported without enabling validation in apply_update, which is costly because it requires a "twin doc". Maybe a parameter to Doc like validate_updates=False. Maybe in the future a validate_changes=True parameter could validate changes before applying them to the document too.
Also, you may want to look at type annotations if you don't need validation, which works at static type analysis and at run-time.
| skip_gc: bool | None = None, | ||
| doc: _Doc | None = None, | ||
| Model=None, | ||
| Model: Any | 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.
| Model: Any | None = None, | |
| Model: Any = None, |
| def get_model_state(self) -> Any: | ||
| if self._Model is None: | ||
| raise RuntimeError( | ||
| "no Model defined for doc. Instantiate Doc with Doc(Model=PydanticModel)" |
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 Model defined for doc. Instantiate Doc with Doc(Model=PydanticModel)" | |
| "Document has no model" |
| ) | ||
|
|
||
|
|
||
| def test_model_no_model_defined(): |
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.
| def test_model_no_model_defined(): | |
| def test_model_not_defined(): |
| with pytest.raises(RuntimeError) as exc_info: | ||
| local_doc.get_model_state() | ||
|
|
||
| assert str(exc_info.value).startswith("no Model defined for doc") |
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.
| assert str(exc_info.value).startswith("no Model defined for doc") | |
| assert str(exc_info.value).startswith("Document has no model") |
| ) | ||
| with self.transaction() as txn: | ||
| assert txn._txn is not None | ||
| all_roots = self._doc.to_py(txn._txn) |
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'm wondering why you don't do the same as in Doc.apply_update?:
d = {k: self._doc[k].to_py() for k in self._Model.model_fields}
self._Model.model_validate(d)| result.into() | ||
| } | ||
|
|
||
| fn to_py(&self, py: Python<'_>, txn: &mut Transaction) -> PyResult<Py<PyAny>> { |
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.
Does this convert nested shared data to Python too?
| "mypy", | ||
| "coverage[toml] >=7", | ||
| "exceptiongroup; python_version<'3.11'", | ||
| "ruff>=0.13.3", |
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.
Since you're introducing ruff, maybe we should use pre-commit to check and lint?
Add get_model_state to Doc
This adds
get_model_statewhich returns the entire doc state as thepydantic model pass in as
Model=during instantiation.It's effectively the other side of
apply_update.If the doc has no Model defined it will raise a RuntimeError.
If the doc is invalid it will raise a pydantic ValidationError.
Add to_py to Doc
get_mode_state uses a new
to_pyforDoc(1aa8dfc)It iterates through the roots of the Doc, converting them into their
native python types (using the underlying type's to_py() fn).
additional changes
Also, update
apply_updateto usemodel_validateinstead ofModel(**value). This is more of a "style" thing.
See pydantic/pydantic#9676
Also add ruff to the test dependencies and format given it's config was already
defined in pyproject.toml.