Update node serialization/deserialization and other Pydantic issues #6990
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6990 +/- ##
==========================================
- Coverage 77.61% 77.54% -0.06%
==========================================
Files 566 566
Lines 43546 43722 +176
==========================================
+ Hits 33794 33901 +107
- Misses 9752 9821 +69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
edan-bainglass
force-pushed
the
fix-node-serialization
branch
4 times, most recently
from
9b7cfbd to
d83ced6
Compare
|
@khsrali regarding one of the failed tests... In this PR, I lift some validation to the top of the constructor (of stash data classes - see 91e4ad5), to avoid any operations if the object is bound to fail. However, this seems to introduce failures in testing them. It suggests that perhaps there is some order to the operations, though I don't see it. Can you please comment? |
It was checking for |
|
@GeigerJ2 okay, this is ready for others to inspect. I did my best to isolate the commits and provided comments on each. Happy to discuss further. Pinging also @agoscinski @superstar54 if interested. Pinging @sphuber for input/feedback, if he has time. |
|
It may be possible to rely on the post models of aiida-restapi as a reference for defining ORM constructor parameters, as the post models are intended to represent serialized objects passed to the REST API for object construction. Looking into this. |
edan-bainglass
force-pushed
the
fix-node-serialization
branch
3 times, most recently
from
c8ded01 to
476e144
Compare
edan-bainglass
force-pushed
the
fix-node-serialization
branch
from
476e144 to
7c191fa
Compare
|
@danielhollas what is this about? Nevermind. I see that importing |
edan-bainglass
force-pushed
the
fix-node-serialization
branch
from
516a6df to
2c3e5df
Compare
Nice, the system works. :-) Feel free to improve the error message here to make it more obvious that this is about the |
edan-bainglass
force-pushed
the
fix-node-serialization
branch
from
d6e44ff to
0b37ef6
Compare
|
|
@danielhollas what do you think about ignoring Ruff N806 - "~variable should be lowercase"? See case below. |
Do you mean ignoring locally (fine) or globally? |
Global. There are many cases when the variable is a class, not an instance. I've pushed this in my last commit just to verify that it works. Okay with removing it in favor of local handling, but would like to hear the reason against a global N806 rule. UpdateNice. Ignoring N806 globally raised a whole lot of RUF100 due to the codebase being littered with local N806 rules. I'd say that supports my case 🙂 |
|
@danielhollas done for tonight. Will revisit this in the morning 😴 |
Yeah, running Seems fine to remove it, or alternatively, use the e.g. something like this in pyproject.toml In any case please open a separate PR for that so we don't polute this one with bikeshedding discussion and unrelated changes. |
Thanks @danielhollas. Then for this one, since there are only a few cases in my PR, I will locally ignore them. Will open a PR for the pattern handling shortly after. |
edan-bainglass
force-pushed
the
fix-node-serialization
branch
4 times, most recently
from
5faafb0 to
a4a5b3e
Compare
edan-bainglass
force-pushed
the
fix-node-serialization
branch
from
13ca751 to
9bb780c
Compare
| class Model(NumericType.Model): | ||
| value: float = MetadataField( | ||
| title='Float value', | ||
| description='The value of the float', | ||
| ) |
There was a problem hiding this comment.
I override the models of BaseType sub-entities (NumericType is also a BaseType) to provide clear documentation of the inherited value field.
| def MetadataField( # noqa: N802 | ||
| default: t.Any = PydanticUndefined, | ||
| *, | ||
| priority: int = 0, | ||
| short_name: str | None = None, | ||
| option_cls: t.Any | None = None, | ||
| orm_class: type[Entity[t.Any, t.Any]] | str | None = None, | ||
| orm_to_model: t.Callable[[Entity[t.Any, t.Any], Path], t.Any] | None = None, | ||
| model_to_orm: t.Callable[['BaseModel'], t.Any] | None = None, | ||
| orm_to_model: t.Callable[[Entity[t.Any, t.Any], dict[str, t.Any]], t.Any] | None = None, | ||
| model_to_orm: t.Callable[[BaseModel], t.Any] | None = None, | ||
| exclude_to_orm: bool = False, | ||
| exclude_from_cli: bool = False, | ||
| is_attribute: bool = True, | ||
| is_subscriptable: bool = False, | ||
| **kwargs: t.Any, | ||
| ) -> t.Any: |
There was a problem hiding this comment.
@agoscinski I ended up dropping exclude_from_cli from the model system, as it is redundant. All proper exclude_from_cli are already marked with exclude_to_orm, which makes sense. You should not prompt in the CLI for a field that is to be excluded from the ORM constructor, as that is what you're trying to do - create an entity (computer/code/etc.). However, we still provide CLI arguments via MetadataField - priority (order), short_name (e.g., -Y for computer), option_cls (click.Option class).
We could think in the future if indeed a separate model is needed for the CLI. At the moment, I am not convinced. CLI and REST API are two means of interaction with the ORM layer (get/post actions), and as such both require an understanding of ORM Entity schemas.
Nevertheless, good to hear you thoughts on this (or link here where you record them if already present) 🙏
5 participants
Some parts of the ORM module do not pass a serialization round trip. This PR addresses this, but further discussion is needed regarding what should actually be (de)serialized. If interested, good to read aiidateam/AEP#40 and discussion on #6255. Some discussion regarding mismatch between an object's constructor args and its Model fields can be found here.
Good to also discuss if the use of pydantic should be extended to ORM instance creation in general, not only by way of
from_serialized. This is not implemented in this PR (and is out of scope) but can be addressed in a follow-up PR if deemed "correct".Open questions
repository_content. The current implementation usesbase64encoding/decoding, but this is clearly not ideal for large files. Some discussion was had w.r.t switching to links to some online storage of files. TBDUpdates
The PR now also introduces the following:
Dataplugins viaattributesattributesinDataplugins (not allowed in the backend node)InputModel- a derived view of the defined entityModelsuitable forEntitycreationNote for PR reviewers
There are a few changes that are likely out of scope. These will move to dedicated PRs prior to merge of this PR.