Revert orm model changes since v2.8.0 - #7517
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7517 +/- ##
===========================================
- Coverage 80.67% 29.96% -50.71%
===========================================
Files 581 580 -1
Lines 46998 46303 -695
===========================================
- Hits 37909 13868 -24041
- Misses 9089 32435 +23346 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
885f2bd to
2a70de9
Compare
)" This reverts commit f47698f.
This is a lot! Can you please clarify once more, just to be super clear, as to why we are reverting these changes? 🙏 |
This reverts commit f456028. git rm aiida/orm/pydantic.py git checkout --theirs -- . git revert --continue
pytest --force-regen tests/orm/test_fields.py
They are required for all pyupgrade to allow forward declarations
git diff --diff-filter=d upstream/main HEAD --name-only -z -- '*.py' | xargs -0 uv run pre-commit run --files Run twice
git diff --diff-filter=d upstream/main HEAD --name-only -z -- '*.py' | xargs -0 uv run ruff check --fix --unsafe-fixes verified manually if they match upstream/main changes
From commit 12f88f4 reapply changes on src/aiida/orm/fields.py: `fields.py`: `extract_root_type` unwraps both `typing.Union` and the PEP 604 `types.UnionType`, so the rewrites do not degrade 207 ORM query fields across 40 classes to `QbAnyField` on Python 3.10-3.13.
Revert back to importlib_metadata
2a70de9 to
2680e3f
Compare
|
The pydantic orm model upgrade touches the public API a lot. It would be like 95% of our public API changes in the next release. You mentioned that by removing the automatic model rebuilds on initialization, the restapi test suite does not work properly. So the feature is not working properly as intended or introduces performance regression. That seems not something we want to ship. Also we soon will change how the ORM models work. so we completely break this API change again in 2-3 months. Even if is a valid break by semantic versioning, it seems unwise to ship something like this. Reverting the commits was for me the safest option to remove it from the public API since not much manual work has to be done that needs to be reviewed. I marked all commits that are not just running commands with |
Thanks for clarifying @agoscinski. No issue from my side. As discussed, we restore this on |
|
@agoscinski as a follow up, regarding the single failed test on the REST API, which fails due to an incorrect JSON schema title for the dynamic WriteModel = cast( # noqa: N806
type[OrmModel],
pdt.create_model(
name,
__config__=deepcopy(model_cls.model_config), # lifted into the creation step
__base__=tuple(bases),
__module__=model_cls.__module__,
__qualname__=model_cls.__qualname__.replace(suffix, 'WriteModel'), # lifted into the creation step
**model_fields,
),
)
# WriteModel.__qualname__ = model_cls.__qualname__.replace(suffix, 'WriteModel')
WriteModel.__pydantic_decorators__.field_serializers = serializers
WriteModel.__pydantic_decorators__.field_validators = validators
# WriteModel.model_config = deepcopy(model_cls.model_config)
`
By moving those two parts into the model creation, I no longer require a `model_rebuild`. |
2 participants
Because these changes touch the public API, I decided to go through the effort and remove them for the release. Each commit is just a revert of the marked commit. Then at the end i applied
pyupgrade --py310-pluson the files I have touched with the revert.