Cache JSON Schema ref facts#3472
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changes$ref caching and ref-loading refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-3472.datamodel-code-generator.pages.dev |
Merging this PR will improve performance by 16.59%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.6 s | 3 s | +20.15% |
| ⚡ | WallTime | test_perf_complex_refs |
2 s | 1.7 s | +20.06% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.6 s | 3 s | +19.55% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,078.2 ms | 911.1 ms | +18.34% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.7 s | 2.2 s | +18.19% |
| ⚡ | WallTime | test_perf_deep_nested |
5.2 s | 4.4 s | +16.67% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
2 s | 1.7 s | +15.17% |
| ⚡ | WallTime | test_perf_all_options_enabled |
5.4 s | 4.7 s | +15.14% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
796.6 ms | 697.1 ms | +14.27% |
| ⚡ | WallTime | test_perf_openapi_large |
2.9 s | 2.6 s | +12.92% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.9 s | 1.7 s | +12.33% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing cache-ref-data-type-facts (27adfb4) with main (988da55)
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 #3472 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 150 150
Lines 32337 32375 +38
Branches 3774 3775 +1
=========================================
+ Hits 32337 32375 +38
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.
🧹 Nitpick comments (1)
tests/parser/test_jsonschema.py (1)
184-188: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake the cache-hit assertion non-vacuous.
This currently passes even if
Path.resolveis never called. Assert the first invocation actually exercised resolution before checking the second call is cached.Proposed test hardening
parser._resolve_local_ref_path(target, "schema.json") first_call_count = len(calls) + assert first_call_count > 0 parser._resolve_local_ref_path(target, "schema.json") assert len(calls) == first_call_count🤖 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/parser/test_jsonschema.py` around lines 184 - 188, The test currently checks that a second call to parser._resolve_local_ref_path does not increase the call count, but it fails to verify that the first invocation actually performed resolution and increased the call count. Add an assertion after the first parser._resolve_local_ref_path call to confirm that len(calls) is greater than first_call_count, then capture this new count and call parser._resolve_local_ref_path a second time to verify the cached result does not increase the call count further.
🤖 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/parser/test_jsonschema.py`:
- Around line 184-188: The test currently checks that a second call to
parser._resolve_local_ref_path does not increase the call count, but it fails to
verify that the first invocation actually performed resolution and increased the
call count. Add an assertion after the first parser._resolve_local_ref_path call
to confirm that len(calls) is greater than first_call_count, then capture this
new count and call parser._resolve_local_ref_path a second time to verify the
cached result does not increase the call count further.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 531be4e8-b483-49eb-8507-19eb6f192566
📒 Files selected for processing (2)
src/datamodel_code_generator/parser/jsonschema.pytests/parser/test_jsonschema.py
Breaking Change AnalysisResult: No breaking changes detected Reasoning: PR #3472 is a purely internal performance optimization that caches JSON Schema ref facts. All modified methods are private (underscore-prefixed): This analysis was performed by Claude Code Action |
|
🎉 Released in 0.65.1 This PR is now available in the latest release. See the release notes for details. |
1 participant
Summary by CodeRabbit
Performance
$refhandling by reusing cached type facts from previously validated reference targets.$refpaths, avoiding repeated path resolution work during parsing.Tests
$reffacts are reused when available, and that fallback loading still occurs when they are not.