Deduplicate Pydantic v2 config helpers#3350
Conversation
Merging this PR will improve performance by 45.11%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_openapi_large |
3.1 s | 2.1 s | +49.28% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.8 s | 2.6 s | +49.15% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
844 ms | 575 ms | +46.79% |
| ⚡ | WallTime | test_perf_complex_refs |
2.1 s | 1.4 s | +46.06% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
2 s | 1.3 s | +46.02% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.8 s | 1.9 s | +45.24% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,112.3 ms | 772.8 ms | +43.93% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
2.1 s | 1.5 s | +43.28% |
| ⚡ | WallTime | test_perf_deep_nested |
5.4 s | 3.8 s | +43.28% |
| ⚡ | WallTime | test_perf_all_options_enabled |
5.6 s | 3.9 s | +42.46% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.8 s | 2.7 s | +40.96% |
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 refactor/pydantic-v2-config-dedup (7f5fd40) with main (46a9e67)
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. ↩
|
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:
📝 WalkthroughWalkthroughExtracts a shared PatternConstraints, centralizes Pydantic v2 config-building into model/pydantic_v2/_config.py, refactors BaseModel and DataClass to use the shared builder, updates msgspec to inherit PatternConstraints, and adds tests for config rendering and constraints. ChangesConfig Generation Refactoring
Sequence Diagram(s)sequenceDiagram
participant TemplateData
participant BaseModel
participant build_base_config_parameters
participant ConfigDict
TemplateData->>BaseModel: provide extra_template_data, all_data_types
BaseModel->>build_base_config_parameters: build_base_config_parameters(extra_template_data, all_data_types, ...)
build_base_config_parameters->>ConfigDict: return config parameters (if any)
ConfigDict->>BaseModel: validated config (model_validate) stored in extra_template_data["config"]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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-3350.datamodel-code-generator.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/datamodel_code_generator/model/pydantic_v2/dataclass.py (1)
13-16: 💤 Low valueConsider importing
ConfigAttributedirectly from_configmodule.
ConfigAttributeis defined in_config.pybut imported here viabase_model.py. Importing directly from the source would reduce indirection and make the dependency clearer.-from datamodel_code_generator.model.pydantic_v2.base_model import ( - ConfigAttribute, -) +from datamodel_code_generator.model.pydantic_v2._config import ( + ConfigAttribute, + _ConfigGenMixin, +)And remove the duplicate
_ConfigGenMixinimport on line 13.🤖 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/model/pydantic_v2/dataclass.py` around lines 13 - 16, Replace the indirect import of ConfigAttribute from base_model by importing ConfigAttribute directly from datamodel_code_generator.model.pydantic_v2._config and remove the duplicated _ConfigGenMixin import so that dataclass.py imports _ConfigGenMixin and ConfigAttribute only from the _config module (eliminate the base_model import of ConfigAttribute and the duplicate _ConfigGenMixin import).
🤖 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 `@src/datamodel_code_generator/model/pydantic_v2/dataclass.py`:
- Around line 13-16: Replace the indirect import of ConfigAttribute from
base_model by importing ConfigAttribute directly from
datamodel_code_generator.model.pydantic_v2._config and remove the duplicated
_ConfigGenMixin import so that dataclass.py imports _ConfigGenMixin and
ConfigAttribute only from the _config module (eliminate the base_model import of
ConfigAttribute and the duplicate _ConfigGenMixin import).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc7dc97e-a9ed-4297-8d07-d7a0728a2b4a
📒 Files selected for processing (7)
src/datamodel_code_generator/model/msgspec.pysrc/datamodel_code_generator/model/pydantic_base.pysrc/datamodel_code_generator/model/pydantic_v2/__init__.pysrc/datamodel_code_generator/model/pydantic_v2/_config.pysrc/datamodel_code_generator/model/pydantic_v2/base_model.pysrc/datamodel_code_generator/model/pydantic_v2/dataclass.pytests/model/pydantic_v2/test_config.py
koxudaxi
force-pushed
the
refactor/pydantic-v2-config-dedup
branch
3 times, most recently
from
68a27f0 to
f565342
Compare
koxudaxi
force-pushed
the
refactor/pydantic-v2-config-dedup
branch
from
f565342 to
bed2766
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model/pydantic_v2/test_root_model.py (1)
74-88: ⚡ Quick winCover the
additionalProperties=Falsebranch too.This regression test only exercises the
True -> extra='allow'path. The removed RootModel override suppressed config-extra unconditionally, so a bug in theFalse -> extra='forbid'mapping would still slip through. Please add a second case foradditionalProperties=Falseor parametrize both values.Suggested expansion
def test_root_model_ignores_extra_config() -> None: """RootModel must not render ConfigDict(extra=...) because Pydantic rejects it.""" - root_model = RootModel( - fields=[ - DataModelFieldBase( - name="a", - data_type=DataType(type="str"), - required=True, - ) - ], - reference=Reference(name="TestRootModel", path="test_root_model"), - extra_template_data=defaultdict(dict, {"test_root_model": {"additionalProperties": True}}), - ) - - assert "model_config" not in root_model.render() + for additional_properties in (True, False): + root_model = RootModel( + fields=[ + DataModelFieldBase( + name="a", + data_type=DataType(type="str"), + required=True, + ) + ], + reference=Reference(name="TestRootModel", path="test_root_model"), + extra_template_data=defaultdict( + dict, + {"test_root_model": {"additionalProperties": additional_properties}}, + ), + ) + + assert "model_config" not in root_model.render()🤖 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/model/pydantic_v2/test_root_model.py` around lines 74 - 88, Expand the test_root_model_ignores_extra_config test to cover both values of additionalProperties (True and False) by parametrizing or adding a second case: create a RootModel (the same way using extra_template_data on RootModel and DataModelFieldBase) with additionalProperties=False and assert the rendered output from root_model.render() exercises the False -> extra='forbid' mapping (e.g., check the rendered model_config or absence/contents reflect 'forbid'), while keeping the existing True case that maps to 'allow'; ensure you reference RootModel, extra_template_data, additionalProperties and render when locating the test to modify.
🤖 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/model/pydantic_v2/test_root_model.py`:
- Around line 74-88: Expand the test_root_model_ignores_extra_config test to
cover both values of additionalProperties (True and False) by parametrizing or
adding a second case: create a RootModel (the same way using extra_template_data
on RootModel and DataModelFieldBase) with additionalProperties=False and assert
the rendered output from root_model.render() exercises the False ->
extra='forbid' mapping (e.g., check the rendered model_config or
absence/contents reflect 'forbid'), while keeping the existing True case that
maps to 'allow'; ensure you reference RootModel, extra_template_data,
additionalProperties and render when locating the test to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e2d4a67-1860-4d9d-bf5a-541cdd2eb163
📒 Files selected for processing (9)
src/datamodel_code_generator/model/msgspec.pysrc/datamodel_code_generator/model/pydantic_base.pysrc/datamodel_code_generator/model/pydantic_v2/__init__.pysrc/datamodel_code_generator/model/pydantic_v2/_config.pysrc/datamodel_code_generator/model/pydantic_v2/base_model.pysrc/datamodel_code_generator/model/pydantic_v2/dataclass.pysrc/datamodel_code_generator/model/pydantic_v2/root_model.pytests/model/pydantic_v2/test_config.pytests/model/pydantic_v2/test_root_model.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/datamodel_code_generator/model/pydantic_v2/_config.py
- src/datamodel_code_generator/model/pydantic_v2/init.py
- src/datamodel_code_generator/model/pydantic_base.py
- tests/model/pydantic_v2/test_config.py
- src/datamodel_code_generator/model/pydantic_v2/base_model.py
- src/datamodel_code_generator/model/pydantic_v2/dataclass.py
- src/datamodel_code_generator/model/msgspec.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3350 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 127 129 +2
Lines 28420 28450 +30
Branches 3452 3442 -10
=========================================
+ Hits 28420 28450 +30
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:
|
Breaking Change AnalysisResult: No breaking changes detected Reasoning: I could not access the prepared authoritative sources (analysis-context.md, pr.diff, deleted-lines.txt) — they live under /home/runner/work/_temp/ which is outside the session's allowed working directory, and every access method (Read tool permission, Bash cat/grep/python/cp/ls including sandbox-disabled, gh pr diff, and a subagent) was blocked. Per the ground rules, a breaking change (removed/changed CLI option, API symbol, default, template, etc.) may only be reported if it appears as a removed line in the prepared diff, which I cannot inspect. The only reliable in-repo evidence is PR #3350's merge metadata: title "Deduplicate Pydantic v2 config helpers" and commit messages ("Refactor pydantic v2 config generation", "Preserve Pydantic v2 ConfigDict surface", "Avoid copying Pydantic v2 config parameters", "Cover RootModel ignored extra modes", "Restore Pydantic v2 constraints surface", "Avoid RootModel config scans"). This describes an internal deduplication refactor that explicitly preserves/restores the generated config surface, indicating generated output is intended to be unchanged. With no evidenced removed lines and the refactor's stated goal of preserving behavior, I cannot assert any breaking change and therefore report none. Caveat: this conclusion is based on PR metadata, not the inaccessible diff. This analysis was performed by Claude Code Action |
|
🎉 Released in 0.64.0 This PR is now available in the latest release. See the release notes for details. |
1 participant
No description provided.