-
-
Notifications
You must be signed in to change notification settings - Fork 56
Adding support for pydantic extras in models #732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 6857 6895 +38
=========================================
+ Hits 6857 6895 +38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing!
Overall looks good. But it seems that the tests don't cover the code. Every line should be covered.
Oh wired the full pytest report had it at 100% locally. Will pull the pr back to fresh env and double check ^.^ |
57364b3
to
e1da4b1
Compare
Hopefully this Dockerfile is useful to someone else.
|
d0edcc3
to
0940383
Compare
Following up here. Any additional change or comment to this pr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I have been busy.
Now I have looked in more details and I have several comments. Note that many comments I wrote only once or twice, but the same would apply in other places where the same pattern appears.
# Check if this is a Pydantic model with extra configuration | ||
group = self.groups[group_key] | ||
should_raise_error = True | ||
if hasattr(group, "group_class") and group.group_class: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if hasattr(group, "group_class") and group.group_class: | |
if getattr(group, "group_class", None): |
should_raise_error = True | ||
if hasattr(group, "group_class") and group.group_class: | ||
from ._optionals import get_pydantic_extra_config | ||
|
||
extra_config = get_pydantic_extra_config(group.group_class) | ||
if extra_config == "allow": | ||
# Allow extra fields - don't raise an error | ||
should_raise_error = False | ||
elif extra_config == "ignore": | ||
# Ignore extra fields - don't raise an error, Pydantic will ignore during instantiation | ||
should_raise_error = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this logic seems to be pydantic-specific. So it might be better to have all of it in a function imported from optionals, e.g. is_allowed_by_pydantic_extra(...)
. The function get_pydantic_extra_config
could still exist just to not have one single big function.
@@ -825,6 +825,62 @@ class NestedModel(pydantic.BaseModel): | |||
class PydanticNestedDict(pydantic.BaseModel): | |||
nested: Optional[Dict[str, NestedModel]] = None | |||
|
|||
# Helper function to create test models dynamically based on current pydantic version | |||
def _create_extra_models(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I don't like this function. First, in every place it is used, part of the output is ignored. Also, note that in all the tests, classes are not generated dynamically. An important feature of jsonargparse is serialization, which means that classes/functions should be importable. Dynamically generated classes are not importable. Could be that there is no problem with the current tests, but would be a problem if for some reason we need to extend the tests to include serialization.
Please do as the rest of the tests. Defined globally inside if-else statements, not generated in a function.
_pydantic_v2_syntax = ( | ||
hasattr(pydantic, "ConfigDict") | ||
and hasattr(pydantic.ConfigDict, "__module__") | ||
and "pydantic.config" in pydantic.ConfigDict.__module__ | ||
and "v1" not in pydantic.ConfigDict.__module__ | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that pydantic_support
already tells if it is v1 or v2, can it be used to simplify this?
class Config: | ||
extra = "allow" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This produces a Support for class-based config is deprecated, use ConfigDict instead...
warning. I want the tests output to be as clean as possible, so that when some actual new warning is introduced, then it becomes immediately obvious. So please capture and assert this specific warning so that pytest does not show it in its output.
result = get_pydantic_extra_config(PydanticNoExtra) | ||
# In pydantic v1, models without explicit extra config default to 'ignore' | ||
# In pydantic v2, they default to 'forbid' (but our function returns None for default) | ||
from jsonargparse._optionals import is_pydantic_model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import at the top of the module.
except (ImportError, AttributeError): | ||
# pydantic v1 not available, skip this test | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced about this except. Better if it is a separate test function that has a decorator that properly skips the test when no v1, e.g. here.
Also, is there any pydantic release that doesn't have v1?
result = get_pydantic_extra_config(ProblematicClass) | ||
assert result is None | ||
|
||
def test_pydantic_extra_config_v2_direct_attribute(self, monkeypatch): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something only applies to pydantic v2, then only run the test when such a version is installed. Add a decorator so that it gets skipped when pydantic v1 is installed, e.g. here.
result = get_pydantic_extra_config(mock_model_v2) | ||
assert result == "allow" | ||
|
||
def test_pydantic_extra_config_v2_legacy_config(self, monkeypatch): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If several tests are similar/related, it would be nicer to be in a single function and use the subtests
fixture.
- Support for Pydantic models with ``extra`` field configuration (``allow``, | ||
``forbid``, ``ignore``). Models with ``extra="allow"`` now accept additional | ||
fields, while ``extra="forbid"`` properly rejects them and ``extra="ignore"`` | ||
accepts but ignores extra fields during instantiation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepts but ignores extra fields during instantiation. | |
accepts but ignores extra fields during instantiation (`#732 | |
<https://github.com/omni-us/jsonargparse/pull/732>`__). |
sweet appreciate the feedback. will look once i have a bit of time the rest of this month is gunna be stupid for me so ill likely look at addressing these in aug once i recover some spare brain cells ^.^ |
This PR adds support for Pydantic models with
extra
field configuration (allow
,forbid
,ignore
).Key changes:
extra="allow"
now accept and include additional fields during parsing and instantiationextra="forbid"
properly reject extra fields with clear error messagesextra="ignore"
accept extra fields during parsing but ignore them during instantiationBehavior:
extra="allow"
: Extra fields are accepted and included in the final modelextra="forbid"
: Extra fields cause aNSKeyError
during parsingextra="ignore"
: Extra fields are accepted during parsing but ignored during model instantiationThis resolves the limitation where jsonargparse would always reject extra fields regardless of the Pydantic model's
extra
configuration.