Conversation
…s the correct way per Pydantic v2.11+.
|
this needs minimum pin in pyproject.toml to accompany this |
|
@skrawcz done. |
|
@vbhavh I think we want it for all pydantic dependencies in pyproject.toml. |
|
hey @skrawcz, is it solved now ? I have done it for all pydantic deps. |
|
Could we add a test that confirms this? |
i'll add one in test/integrations. |
|
@elijahbenizzy done. |
| def test_pydantic_version(): | ||
| """Ensure pydantic >= 2.11 is installed (required for class-level model_fields access).""" | ||
| from packaging.version import Version | ||
| assert float(float(".".join(pydantic.__version__.split(".")[:2]))) >= float("2.11"), ( | ||
| f"pydantic >= 2.11 required, got {pydantic.__version__}" | ||
| ) |
There was a problem hiding this comment.
@elijahbenizzy you didn't mean this right ?
What type of test were you looking for? I think the existing unit tests should have covered things?
andreahlert
left a comment
andreahlert
left a comment
There was a problem hiding this comment.
Tested locally with pydantic 2.12.5. All 32 existing tests pass. The core fix (accessing model_fields from the class) works correctly. However, this PR has several structural issues that need addressing.
|
|
||
| def model_to_dict(model: pydantic.BaseModel, include: Optional[List[str]] = None) -> dict: | ||
| """Utility function to convert a pydantic model to a dictionary.""" | ||
| keys = model.model_fields.keys() |
There was a problem hiding this comment.
This guard is unnecessary. model_to_dict has the signature model: pydantic.BaseModel - it always receives an instance, never a class. No call site in the entire codebase passes a class here.
Simpler and equivalent:
keys = type(model).model_fields.keys()The isinstance(model, type) branch is dead code.
| keys = model_cls.model_fields.keys() | ||
| keys = keys if include is None else [item for item in include if item in model_cls.model_fields] | ||
| return {key: getattr(model, key) for key in keys} | ||
|
|
There was a problem hiding this comment.
Same issue. subset_model has the signature model: Type[ModelType] - it always receives a class, never an instance. The isinstance(model, type) guard will always be True, making type(model) never execute.
This is dead code that adds confusion. The original model.model_fields on a class was already correct and never triggered the deprecation warning - there's nothing to fix here.
Verified locally:
>>> TestModel.model_fields # class access, no warning
>>> type(TestModel).model_fields # goes to ModelMetaclass, AttributeError!The guard accidentally prevents the type() path from executing, which would actually crash since type(SomeBaseModel) is ModelMetaclass and has no model_fields.
| @@ -76,7 +77,8 @@ def subset_model( | |||
| """ | |||
| new_fields = {} | |||
|
|
|||
There was a problem hiding this comment.
Same pattern, same problem. model_from_state takes model: Type[ModelType] - always a class. The original code model.model_fields.keys() was already correct here. No deprecation warning is emitted when accessing model_fields on a class.
This change adds unnecessary indirection without fixing anything.
| model_cls = model if isinstance(model, type) else type(model) # Handles the possibility that sometimes model is a class not instance | ||
| for name, field_info in model_cls.model_fields.items(): | ||
| if name in fields: | ||
| # copy directly |
There was a problem hiding this comment.
Same pattern again. _validate_keys takes model: Type[pydantic.BaseModel] - always a class. No fix needed.
Out of the 4 functions changed in this PR, only model_to_dict actually needed the fix. The other 3 already accessed model_fields on the class and never triggered the deprecation.
| "langchain_community", | ||
| "pandas", | ||
| "pydantic[email]", | ||
| "pydantic[email]>=2.11", |
There was a problem hiding this comment.
This version bump is a breaking change that isn't justified by the code fix.
type(model).model_fields works on all pydantic 2.x versions - it's not a new API. The deprecation warning only appears in >=2.11 when accessing model_fields on instances, and the fix removes that access pattern entirely.
Bumping the minimum to >=2.11 breaks users on pydantic 2.0-2.10 who currently work fine and would continue to work fine after this fix.
If a version pin is desired (as @skrawcz mentioned), it should be the minimum already supported pydantic 2.x version, not 2.11. Or it should be handled separately in PR #654 with proper justification.
| @@ -133,6 +134,12 @@ class MyModelWithConfig(pydantic.BaseModel): | |||
| assert SubsetModel.__name__ == "MyModelWithConfigSubset" | |||
There was a problem hiding this comment.
This test checks the installed pydantic version, not behavior. It will fail on any CI or user environment running pydantic 2.0-2.10, which currently work perfectly.
A better approach: test that model_to_dict does not emit DeprecationWarning:
import warnings
def test_model_to_dict_no_deprecation_warning():
model = OriginalModel(foo=1, bar="bar", nested=NestedModel(nested_field1=1))
with warnings.catch_warnings():
warnings.simplefilter("error")
result = model_to_dict(model)
assert "foo" in resultThis tests actual behavior regardless of pydantic version.
| @@ -22,6 +22,7 @@ | |||
| import pytest | |||
| from pydantic import BaseModel, ConfigDict, EmailStr, Field | |||
| from pydantic.fields import FieldInfo | |||
There was a problem hiding this comment.
import warnings is added but never used in any test.
5 participants
fix of issue: #652
Changes:
type(model).model_fields accesses the attribute on the class, which is the correct way per Pydantic v2.11+.