Resolve model rebuild issue - #7519
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDynamic Pydantic model factories now pass copied configuration and qualified names to ChangesDynamic model generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiida/orm/pydantic.py (1)
72-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCopy
FieldInfobefore modifying nested-model metadata.
fieldreferencescls.model_fields[key]. The code changesfield.annotationandfield.default_factorybefore line 82 copies it. After minimal-model creation, non-minimal serialization can use the nested minimal model and omit fields markedmay_be_large.Proposed fix
- for key, field in cls.model_fields.items(): + for key, source_field in cls.model_fields.items(): + field = deepcopy(source_field) annotation = field.annotation if get_metadata(field, 'may_be_large'): continue if isinstance(annotation, type) and issubclass(annotation, OrmModel): sub_minimal_model = annotation._as_minimal_model() field.annotation = sub_minimal_model if any(f.is_required() for f in sub_minimal_model.model_fields.values()): field.default_factory = None - model_fields[key] = (field.annotation, deepcopy(field)) + model_fields[key] = (field.annotation, field)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/pydantic.py` around lines 72 - 82, Deep-copy each field’s FieldInfo before changing nested-model metadata in the minimal-model construction flow around _as_minimal_model. Apply annotation and default_factory updates only to the copy, then store that copied field in model_fields, leaving cls.model_fields unchanged so regular serialization retains the full nested model.
🧹 Nitpick comments (1)
src/aiida/orm/entities.py (1)
561-575: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
copy_decoratorand assign the exception message.Add Sphinx
:param:,:return:, and:raises:fields to the helper docstring. Assign the RuntimeError message tomsgbefore raising it.As per coding guidelines, “Use Sphinx-style docstrings (
:param:,:return:,:raises:)” and “Assign exception messages to a variable before raising.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/entities.py` around lines 561 - 575, Update copy_decorator’s docstring with Sphinx :param: entries for model_cls, name, and decorator, plus :return: and :raises: descriptions. In the missing-attribute branch, assign the RuntimeError message to a msg variable before raising it.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/aiida/orm/pydantic.py`:
- Around line 72-82: Deep-copy each field’s FieldInfo before changing
nested-model metadata in the minimal-model construction flow around
_as_minimal_model. Apply annotation and default_factory updates only to the
copy, then store that copied field in model_fields, leaving cls.model_fields
unchanged so regular serialization retains the full nested model.
---
Nitpick comments:
In `@src/aiida/orm/entities.py`:
- Around line 561-575: Update copy_decorator’s docstring with Sphinx :param:
entries for model_cls, name, and decorator, plus :return: and :raises:
descriptions. In the missing-attribute branch, assign the RuntimeError message
to a msg variable before raising it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 264d0491-791f-488a-b47b-01c98516e3d0
📒 Files selected for processing (4)
src/aiida/orm/entities.pysrc/aiida/orm/nodes/data/code/abstract.pysrc/aiida/orm/nodes/node.pysrc/aiida/orm/pydantic.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7519 +/- ##
==========================================
- Coverage 80.67% 80.65% -0.01%
==========================================
Files 581 581
Lines 46998 46989 -9
==========================================
- Hits 37909 37894 -15
- Misses 9089 9095 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@agoscinski most mypy complaints are due to the type signature of Pydantic's |
| type[OrmModel], | ||
| pdt.create_model( | ||
| 'CliModel', | ||
| __config__=deepcopy(cls.ConstructorArgsModel.model_config) | {'arbitrary_types_allowed': True}, |
There was a problem hiding this comment.
I think maybe the deepcopy here (and on __config__ in general) is not necessary 🤔 Will check
There was a problem hiding this comment.
Lets keep deepcopy to be safe. It seems to not hurt the performance significant. Can we take some reasonable default from a parent class for | {'arbitrary_types_allowed': True} ?. Having this default in AbstractCode looks like a hardcoded hack.
There was a problem hiding this comment.
I'm not sure what you mean by default here, but in any case, I cannot recall why I allowed arbitrary types on the code schema (tests pass without it). Looking into it. While I'm at it, I will revisit all config overrides (not many).
There was a problem hiding this comment.
I'm failing to reproduce any issue when leaving arbitarary_types_allowed=False 🤷🏻♂️
There was a problem hiding this comment.
So I am fine how it is now. With | {'arbitrary_types_allowed': True} you introduced a default value if the other one is None. I think it is okay to have a default value for this pydantic models but not in the abstract code. Now the default value is in src/aiida/orm/pydantic.py which is okay for me.
There was a problem hiding this comment.
Oh. That's not an or operator. It is a merge operation on dictionaries (| and & set operators). So the old config + {the mutation}. See here for another example.
There was a problem hiding this comment.
I can't find an issue without it though. I imagine at some point of my testing, I had an issue with this sort of serialization. My guess is the code changed and I hadn't updated this part yet. In addition, see comment in ae3fb70 regarding redundant __config__ redefinitions.
There was a problem hiding this comment.
Right I mixed it with or usage. Anyway, its not in abstract code anymore so I am fine with it.
The issue appears to be with the type of model_fields. Fixing... (update - this is now resolved) |
b77b3c4 to
feb39df
Compare
agoscinski
left a comment
agoscinski
left a comment
There was a problem hiding this comment.
I cannot really reason on this meta class logic changes especially in src/aiida/orm/pydantic.py. We already discussed that there will be a redesign. I hope the meta class logic will there be drastically reduced as it is impossible for me now to understand all the details about it. I checked that this does not introduce another performance regression.
Which part? |
|
Sorry for not being clear. I checked it (by running the production benchmarks), and it does not introduce another performance regression. Its all good. |
They are only needed if modifying (see minimal model) or if there are multiple inheritance (see write model). In all other cases, the default `__config__` is already set in the base class and does not need to be redefined.
To be clear, do we still have a performance regression w.r.t pre-#6990? You mentioned there was still something there. |
|
So it might be that if you completely remove the meta class initialization and inheritance from pydantic classes inside the orm classes then you gain another 10% but I did not verify this rigorously. Here the code changes agoscinski@e0f7e55#diff-859fbb51fc0e6832b53b0e85123464fd19c508c4778a73a7514c2f7ada8fa8ce Note that I also did not factor out variance in the benchmark. This effects the estimate of 10%. So far I only care about large shifts in the benchmarks. It is too much work at the moment to consider smaller shifts as well. Also completely separating the pydantic orm models from the python orm objects for v2.9.0 is an unreasonable workload. |
Thanks @agoscinski. I was asking as part of my assessment for the refactor work in v3. Just wanted to know if improvements are still warranted quantitatively. Cheers. |
|
In any case, even if there would be no performance benefit, I would decouple the orm pydantic models for the python objects since the models are not needed for running calculations and a coupling makes the maintenance of modules that do not require the pydantic serialization harder. I feel like the pydantic validation is not a strong enough reason to keep pydantic models for the construction of the python object, especially not when we have this complex metaclass logic. |
Yes. That is indeed the plan regardless. Just planning out my priorities across several projects 🙂 Thanks |
|
So one thing, because removing the model rebuild did not cause any failure in the aiida-core test suite but was clearly required for the aiida-restapi. Can we add tests for this? So basically the change in 987a56f would have failed with this additional test. |
| cls.WriteModel.model_json_schema()['$defs']['AttributesWriteModel']['title'] | ||
| == AttributesWriteModel.model_json_schema()['title'] | ||
| == f'{name}AttributesWriteModel' |
There was a problem hiding this comment.
To clarify, the issue was a wrong title in the $defs part of the JSON schema (tested in the REST API), but clearly not on the model itself, since the existing test here passed. However, if you simply add the $defs check after, it passes, because the check on the model triggers the deferred model build, making the $defs check miss the mark. Hence, we check equality simultaneously.
|
@agoscinski okay, test added. Good to merge? |
2 participants
Supersedes #7518