Skip to content
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

WIP Fix: init=False in dataclass fields ignored #8545

Closed

Conversation

tigeryy2
Copy link
Contributor

@tigeryy2 tigeryy2 commented Jan 13, 2024

Change Summary

  • Added init attribute to FieldInfo and Field
  • Added check for init in make_pydantic_fields_compatible
  • Added test to confirm init=False fields don't show up in the signature
  • Corrected docstring for init_var

Related issue number

WIP Partial fix for #8454

  • Previously, setting init=False in std field and pydantic's Field had no effect, the field still appears as a parameter in the generated __init__

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Notes

  • Haven't updated docs for the new init parameters yet.
  • Unfortunately, can still use the init=False fields as parameters in the dataclass constructor... I think this is because of pydantic allowing the "extra" params that aren't explicitly defined
  • Even setting ConfigDict(extra='forbid') doesn't lead to throwing a ValidationError. From some debugging, it doesn't look like the model schema understands the init=False. So more work needed to actually throw an error.

For example:

@pytest.mark.parametrize(
    'init_false_field', [dataclasses.field(init=False, default=-1), pydantic.dataclasses.Field(init=False, default=-1)]
)
def test_init_false(init_false_field):
    @pydantic.dataclasses.dataclass(config=ConfigDict(extra='forbid'))
    class MyDataclass:
        a: int = init_false_field
        b: int = pydantic.dataclasses.Field(default=2)

    signature = inspect.signature(MyDataclass)
    # `a` should not be in the __init__
    assert 'a' not in signature.parameters.keys()
    assert 'b' in signature.parameters.keys()

    assert isinstance(MyDataclass(b=2), MyDataclass)
    # test passes up until here
    
    # however, this does not throw an error
    _ = MyDataclass(a=1, b=2)
    
    # this DOES throw a validation error, when that `extra='forbid'` is used
    _ = MyDataclass(a=1, b=2, c=3)

Why
- Previously, setting `init=False` in std field and pydantic's Field had
no effect, the field still appears as a parameter in the generated `__init__`

What
- Added `init` attribute to `FieldInfo` and `Field`
- Corrected docstring for `init_var`
- Added check for `init` in `make_pydantic_fields_compatible`
- Added test to confirm `init=False` fields don't show up in the signature

Notes
- Unfortunately, can still use the `init=False` as parameters in init...
I think this is because of pydantic allowing the "extra" params that aren't
explicitly defined
- Even setting `ConfigDict(extra='forbid')` doesn't help here. From some
debugging, it doesn't look like the model schema understands the `init=False`.
So more work needed to actually throw an error.
@tigeryy2 tigeryy2 marked this pull request as draft January 13, 2024 19:50
Copy link

codspeed-hq bot commented Jan 13, 2024

CodSpeed Performance Report

Merging #8545 will not alter performance

Comparing tigeryy2:fix/dataclass-field-init-false (eea1b40) with main (2e459bb)

Summary

✅ 10 untouched benchmarks

@@ -101,7 +102,9 @@ class FieldInfo(_repr.Representation):
frozen: Whether the field is frozen.
validate_default: Whether to validate the default value of the field.
repr: Whether to include the field in representation of the model.
init_var: Whether the field should be included in the constructor of the dataclass.
init: If true (the default), this field is included as a parameter to the generated `__init__` of the dataclass
init_var: Whether the field is an "init-only" field: used as a parameter to the generated `__init__` method, but
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://docs.pydantic.dev/latest/concepts/fields/#dataclass-constraints, and looking at a few of the test cases, i think the previous docstring for init_var needed to be tweaked.

@tigeryy2
Copy link
Contributor Author

tigeryy2 commented Jan 13, 2024

@sydney-runkle I tried adding init to Field & FieldInfo, then adding handling of it to the changes from #8511. Unfortunately, this only seems to be a partial fix? Wanted to get some feedback before I try to make some more involved changes

Details

The signature for __init__ is fixed but we are still able to initialize using that parameter.

Example:

def test_example():
    @dataclasses.dataclass
    class D1:
        a: int = dataclasses.field(init=False, default=-1)

    @pydantic.dataclasses.dataclass
    class D2:
        a: int = pydantic.dataclasses.Field(init=False, default=-1)

    @pydantic.dataclasses.dataclass
    class D3:
        a: int = dataclasses.field(init=False, default=-1)

    def demo(cls):
        try:
            sig = (inspect.signature(cls))
            res = cls(a=1)
        except Exception as exc:
            print(f"res: {exc}")
        else:
            print(f"res: {res}")
        print(f"sig: {sig}")

    demo(D1)
    demo(D2)
    demo(D3)

Output:

res: test_example.<locals>.D1.__init__() got an unexpected keyword argument 'a'
sig: () -> None
res: test_example.<locals>.D2(a=1)
sig: () -> None
res: test_example.<locals>.D3(a=1)
sig: () -> None

With ConfigDict(extra='forbid')

I think the first example might be expected by default since it supports "extra" fields passed to the initializer, so I also tried it with extra='forbid'. This doesn't seem to help:

@pytest.mark.parametrize(
    'init_false_field', [dataclasses.field(init=False, default=-1), pydantic.dataclasses.Field(init=False, default=-1)]
)
def test_init_false(init_false_field):
    @pydantic.dataclasses.dataclass(config=ConfigDict(extra='forbid'))
    class MyDataclass:
        a: int = init_false_field
        b: int = pydantic.dataclasses.Field(default=2)

    signature = inspect.signature(MyDataclass)
    # `a` should not be in the __init__
    assert 'a' not in signature.parameters.keys()
    assert 'b' in signature.parameters.keys()

    assert isinstance(MyDataclass(b=2), MyDataclass)
    # test passes up until here
    
    # however, this does not throw an error
    _ = MyDataclass(a=1, b=2)
    
    # this DOES throw a validation error, when that `extra='forbid'` is used
    _ = MyDataclass(a=1, b=2, c=3)

Some debugging

I took a look at the schema and DataclassValidator that is generated. My guess is, since there's not support for init=False here yet, the validator doesn't know that a shouldn't be allowed?

The definitions from collect_definitions in (_generate_schema.py):

{'tests.test_dataclasses.test_init_false.<locals>.MyDataclass:4483203024': {'type': 'dataclass', 'cls': <class 'tests.test_dataclasses.test_init_false.<locals>.MyDataclass'>, 'fields': ['a', 'b'], 'schema': {'type': 'dataclass-args', 'dataclass_name': 'MyDataclass', 'fields': [{'type': 'dataclass-field', 'name': 'a', 'schema': {'type': 'default', 'schema': {'type': 'int', 'metadata': {'pydantic.internal.needs_apply_discriminated_union': False}}, 'default': -1}, 'kw_only': False, 'metadata': {'pydantic_js_functions': [], 'pydantic_js_annotation_functions': [<function get_json_schema_update_func.<locals>.json_schema_update_func at 0x106d5c220>]}}, {'type': 'dataclass-field', 'name': 'b', 'schema': {'type': 'default', 'schema': {'type': 'int', 'metadata': {'pydantic.internal.needs_apply_discriminated_union': False}}, 'default': 2}, 'kw_only': False, 'metadata': {'pydantic_js_functions': [], 'pydantic_js_annotation_functions': [<function get_json_schema_update_func.<locals>.json_schema_update_func at 0x106d5c4a0>]}}], 'computed_fields': [], 'collect_init_only': False}, 'post_init': False, 'slots': False, 'config': {'title': 'MyDataclass', 'extra_fields_behavior': 'forbid'}, 'ref': 'tests.test_dataclasses.test_init_false.<locals>.MyDataclass:4483203024', 'metadata': {'pydantic.internal.needs_apply_discriminated_union': False}}}

Version Info

             pydantic version: 2.6.0a1
        pydantic-core version: 2.14.5
          pydantic-core build: profile=release pgo=true
                 install path: /Users/tigery/Documents/dev/python/github/pydantic/pydantic
               python version: 3.11.7 (main, Jan  7 2024, 18:03:31) [Clang 15.0.0 (clang-1500.0.40.1)]
                     platform: macOS-14.1.1-arm64-arm-64bit
             related packages: pydantic-extra-types-2.1.0 typing_extensions-4.8.0 pydantic-settings-2.1.0 mypy-1.1.1 email-validator-2.1.0.post1
                       commit: eea1b409

@sydney-runkle
Copy link
Member

@tigeryy2,

Thanks for your initial work on this, and your detailed report / questions.

@dmontagu has been kind enough to look into this a bit and has opened these two PRS:

Adding support for init does require changes to pydantic-core, as seen in the first PR.

The second PR still needs some tests and some additions to documentation. You've done some great work with adding tests already, as well as adding some much needed docs changes. I'll port the changes you've added over to the second PR.

I'm going to close this in favor of #8552.

Let me know if you have any questions - we appreciate your contributions and hope that you'll continue to help us out with your awesome work 🚀.

@tigeryy2
Copy link
Contributor Author

Thanks @sydney-runkle ! Definitely planning on continuing to contribute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants