Cache derived minimal model on specific class - #7539
Conversation
The previous mechanism caused subclasses to end up with the same cached minimal model as their parent class.
📝 WalkthroughWalkthrough
ChangesMinimal model cache isolation
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7539 +/- ##
==========================================
- Coverage 80.68% 80.67% -0.01%
==========================================
Files 581 581
Lines 47068 47068
==========================================
- Hits 37972 37967 -5
- Misses 9096 9101 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/orm/models/test_models.py (1)
521-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso verify child-model idempotency.
This test proves that
orm.Data.ReadModeldoes not reuseParentMinimalModel. It does not prove that repeated calls reuseChildMinimalModel. Add a second call and an identity assertion.Proposed test addition
assert ChildMinimalModel is not ParentMinimalModel + RepeatedChildMinimalModel = orm.Data.ReadModel._as_minimal_model() # noqa: N806 + assert RepeatedChildMinimalModel is ChildMinimalModel🤖 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 `@tests/orm/models/test_models.py` around lines 521 - 524, Extend test_minimal_model_idempotency_with_submodels by calling orm.Data.ReadModel._as_minimal_model() a second time and asserting the returned object is identical to ChildMinimalModel, while preserving the existing assertion that child and parent minimal models differ.
🤖 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.
Nitpick comments:
In `@tests/orm/models/test_models.py`:
- Around line 521-524: Extend test_minimal_model_idempotency_with_submodels by
calling orm.Data.ReadModel._as_minimal_model() a second time and asserting the
returned object is identical to ChildMinimalModel, while preserving the existing
assertion that child and parent minimal models differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fe1001f-6569-4c8a-a4b2-ad0a9708f109
📒 Files selected for processing (2)
src/aiida/orm/pydantic.pytests/orm/models/test_models.py
|
@GeigerJ2 here's an example of a suggestion that should not be accepted, as such a test already exists in isolation (directly above this new one). I'm wondering then if the rabbit only scans and comments on the new code. If so, we should be careful! |
The previous mechanism caused subclasses to end up with the same cached minimal model as their parent class.