Retain model import caches#3600
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parser now distinguishes built-in and custom generation types, retaining import caches for built-in-only processing while conditionally invalidating caches after custom mutations. New fixtures and integration tests cover aliases, defaults, cross-module models, generic bases, and inherited enums. ChangesImport cache lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant _process_single_module
participant DataModel
Parser->>_process_single_module: determine cache retention for module
_process_single_module->>DataModel: apply model post-processing
_process_single_module->>DataModel: retain or clear import cache
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
📚 Docs Preview: https://pr-3600.datamodel-code-generator.pages.dev |
Merging this PR will degrade performance by 25.45%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | test_perf_large_models_pydantic_v2 |
2.6 s | 3.7 s | -30.43% |
| ❌ | WallTime | test_perf_complex_refs |
1.4 s | 2 s | -28.02% |
| ❌ | WallTime | test_perf_deep_nested |
3.7 s | 5.2 s | -27.84% |
| ❌ | WallTime | test_perf_duplicate_names |
757.7 ms | 1,037.3 ms | -26.96% |
| ❌ | WallTime | test_perf_all_options_enabled |
3.9 s | 5.3 s | -26.93% |
| ❌ | WallTime | test_perf_multiple_files_input |
2.5 s | 3.4 s | -26.19% |
| ❌ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
1.9 s | 2.6 s | -25.42% |
| ❌ | WallTime | test_perf_graphql_style_pydantic_v2 |
576.8 ms | 765.6 ms | -24.67% |
| ❌ | WallTime | test_perf_openapi_large |
2.2 s | 2.9 s | -24.42% |
| ❌ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.5 s | 1.9 s | -23.57% |
| ❌ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.4 s | 1.9 s | -23.13% |
| ❌ | WallTime | test_perf_large_models_pydantic_v2_builtin |
661.1 ms | 848.5 ms | -22.09% |
| ❌ | WallTime | test_perf_large_models_pydantic_v2_noformat |
634.9 ms | 799.3 ms | -20.56% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing agent/retain-import-caches (53e7434) with main (01303c3)
Footnotes
-
98 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3600 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 164 164
Lines 35325 35456 +131
Branches 4078 4091 +13
==========================================
+ Hits 35325 35456 +131
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 (2)
src/datamodel_code_generator/parser/base.py (2)
3843-3883: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReorder the final
validate_defaultpass before capturingmodel_imports.
src/datamodel_code_generator/parser/base.py:3847-3883— the last__set_validate_default_on_fields(...)runs aftermodel_importsis snapshotted, so any field that gainsvalidate_default=Truehere can still miss theField(...)import in the emitted module. Move that pass earlier, or rebuild the import merge afterward.🤖 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/datamodel_code_generator/parser/base.py` around lines 3843 - 3883, Move the final __set_validate_default_on_fields pass before model_imports is constructed in the module-finalization flow, preserving its existing can_retain_cache argument; then snapshot model_imports and perform import merging and cleanup afterward so newly required Field imports are included. Remove the now-duplicated later validation loop.
2236-2260: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the retained imports cache after converting unique lists to sets. Duplicate-name handling and
reuse_model/collapse_reuse_modelsboth touchmodel.imports/get_dedup_key()earlier in_build_module_structure, socan_retain_cache=Truecan carry a stale imports tuple into__change_from_import().🤖 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/datamodel_code_generator/parser/base.py` around lines 2236 - 2260, Ensure __replace_unique_list_to_set always invalidates the model imports cache after changing a field type, including when can_retain_cache=True. Update the changed/cache-clearing logic in __replace_unique_list_to_set so stale imports and get_dedup_key() data cannot reach __change_from_import().
🧹 Nitpick comments (2)
src/datamodel_code_generator/parser/base.py (1)
3179-3198: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAsymmetric cache clearing between the retain/no-retain branches.
In the
can_retain_cache=Truebranch, the model's cache is only cleared whenoriginal_importwas actually found and removed from_additional_imports, mirroring the mutation. In theelse(can_retain_cache=False) branch,_additional_importsis unconditionally reassigned (a no-op new list when nothing changes) butmodel.clear_imports_cache()is never called at all.In practice this is very likely a no-op: the same
can_retain_cachecondition here is generally consistent with the value computed for the samemodelslist inside_process_single_module, whose trailingif not can_retain_cache: _clear_model_imports_cache(models)(line 3831-3832) would already have cleared these caches before__apply_generic_base_classruns. Still, relying on that indirect coupling is fragile — a future refactor to either check could silently reintroduce staleness. Consider making theelsebranch explicit for defense-in-depth.♻️ Suggested defensive fix
else: - model._additional_imports = [ - i for i in additional_imports if i != original_import - ] + if original_import in additional_imports: + model._additional_imports = [ + i for i in additional_imports if i != original_import + ] + model.clear_imports_cache()🤖 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/datamodel_code_generator/parser/base.py` around lines 3179 - 3198, Ensure the can_retain_cache=False branch in the module/model processing loop explicitly clears each model’s imports cache after updating _additional_imports, matching the cache invalidation behavior of the can_retain_cache=True branch. Add model.clear_imports_cache() within that else path, using the surrounding model iteration and _can_retain_model_imports_cache logic as the change location.tests/main/test_main_general.py (1)
97-198: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftConsider adding coverage for the
reuse_model/collapse_reuse_models+ mutation-ordering interplay.The new tests thoroughly cover the builtin-retention and cross-module/enum-hook scenarios, but none exercise
reuse_model=True+collapse_reuse_models=True(or duplicate class names) combined with mutations that happen in__replace_unique_list_to_set/__override_required_fieldbefore.importsis first read in_process_single_module. GivenDataModel.get_dedup_key()reads.importsearlier in_build_module_structure, this combination is the scenario most likely to expose the cache-retention gaps flagged inbase.py. Adding a manifest-style test similar to the existing ones for this combination would help lock in correct behavior.Also applies to: 200-257, 259-311
🤖 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/main/test_main_general.py` around lines 97 - 198, Extend test_parser_retains_builtin_import_cache_and_invalidates_custom_cache with a manifest-style scenario using reuse_model=True and collapse_reuse_models=True, including duplicate class names and mutations from __replace_unique_list_to_set and __override_required_field before _process_single_module first reads imports. Assert the resulting generated output and cache-retention/invalidation manifest, covering the earlier .imports access through DataModel.get_dedup_key() in _build_module_structure.
🤖 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/datamodel_code_generator/parser/base.py`:
- Around line 3843-3883: Move the final __set_validate_default_on_fields pass
before model_imports is constructed in the module-finalization flow, preserving
its existing can_retain_cache argument; then snapshot model_imports and perform
import merging and cleanup afterward so newly required Field imports are
included. Remove the now-duplicated later validation loop.
- Around line 2236-2260: Ensure __replace_unique_list_to_set always invalidates
the model imports cache after changing a field type, including when
can_retain_cache=True. Update the changed/cache-clearing logic in
__replace_unique_list_to_set so stale imports and get_dedup_key() data cannot
reach __change_from_import().
---
Nitpick comments:
In `@src/datamodel_code_generator/parser/base.py`:
- Around line 3179-3198: Ensure the can_retain_cache=False branch in the
module/model processing loop explicitly clears each model’s imports cache after
updating _additional_imports, matching the cache invalidation behavior of the
can_retain_cache=True branch. Add model.clear_imports_cache() within that else
path, using the surrounding model iteration and _can_retain_model_imports_cache
logic as the change location.
In `@tests/main/test_main_general.py`:
- Around line 97-198: Extend
test_parser_retains_builtin_import_cache_and_invalidates_custom_cache with a
manifest-style scenario using reuse_model=True and collapse_reuse_models=True,
including duplicate class names and mutations from __replace_unique_list_to_set
and __override_required_field before _process_single_module first reads imports.
Assert the resulting generated output and cache-retention/invalidation manifest,
covering the earlier .imports access through DataModel.get_dedup_key() in
_build_module_structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5ed4c08-5f98-4412-b75c-c31035ad7af1
⛔ Files ignored due to path filters (5)
tests/data/expected/main/builtin_import_cache_retention.txtis excluded by!tests/data/**/*.txtand included by nonetests/data/expected/main/custom_import_cache_cross_module_history.txtis excluded by!tests/data/**/*.txtand included by nonetests/data/expected/main/custom_import_cache_cross_module_order.txtis excluded by!tests/data/**/*.txtand included by nonetests/data/expected/main/custom_import_cache_inherited_enum_history.txtis excluded by!tests/data/**/*.txtand included by nonetests/data/expected/main/custom_import_cache_inherited_enum_order.txtis excluded by!tests/data/**/*.txtand included by none
📒 Files selected for processing (21)
src/datamodel_code_generator/parser/base.pytests/data/expected/main/builtin_import_cache_retention.pytests/data/expected/main/custom_import_cache_alias_invalidation.pytests/data/expected/main/custom_import_cache_cross_module/__init__.pytests/data/expected/main/custom_import_cache_cross_module/inner_folder/__init__.pytests/data/expected/main/custom_import_cache_cross_module/inner_folder/artificial_folder/__init__.pytests/data/expected/main/custom_import_cache_cross_module/inner_folder/artificial_folder/type_1.pytests/data/expected/main/custom_import_cache_cross_module/inner_folder/schema.pytests/data/expected/main/custom_import_cache_cross_module/inner_folder/type_2.pytests/data/expected/main/custom_import_cache_cross_module/subfolder/__init__.pytests/data/expected/main/custom_import_cache_cross_module/subfolder/type_5.pytests/data/expected/main/custom_import_cache_cross_module/type_4.pytests/data/expected/main/custom_import_cache_generic_base.pytests/data/expected/main/custom_import_cache_inherited_enum/__init__.pytests/data/expected/main/custom_import_cache_inherited_enum/nested_state1.pytests/data/expected/main/custom_import_cache_inherited_enum/nested_state2.pytests/data/expected/main/custom_import_cache_inherited_enum/result1.pytests/data/expected/main/custom_import_cache_inherited_enum/result2.pytests/data/expected/main/custom_import_cache_inherited_enum/state.pytests/data/expected/main/custom_import_cache_invalidation.pytests/main/test_main_general.py
|
Addressed the review against the current code in
Validation: |
Breaking Change AnalysisResult: No breaking changes detected Reasoning: PR #3600 is an internal performance optimization that retains model import caches when possible. All modified functions are private (name-mangled This analysis was performed by Claude Code Action |
1 participant
Summary by CodeRabbit
Performance
Bug Fixes
Tests