Fix mypy plugin for 1.1.0 #5077

cdce8p · 2023-02-19T19:35:15Z

The next mypy release will add better support for dataclass_transform.

Since ModelMetaclass is decorated with it, it means that mypy would use that instead of the custom plugin. The plugin is quite good at parsing BaseModel, so the result would be worse. Especially with regards to the of default arguments as pydantic.field.Field uses a slightly different syntax.

With this PR, the plugin will basically pretend ModelMetaclass isn't decorated and thus continue to use the plugin for it.
I've also added a new (undocumented) option debug_dataclass_transform to be able to toggle that behavior and test new future versions of mypy without modifying the plugin again.

If accepted, it would be great if this PR could be backported to 1.10.X and a new version released. The next mypy release which would otherwise break this behavior is currently scheduled for the end of February.

--

from __future__ import annotations

from pydantic import BaseModel, Field

class Event(BaseModel):
    id: int | None = Field(None, alias="id")

Event()

Without this PR mypy 1.1.0 would emit

error: Missing named argument "id" for "Event"  [call-arg]

yezz123

LGTM 👍🏻

dmontagu · 2023-02-21T01:39:04Z

Looks like this PR is against the main branch; should we also do a backport to v1.10.X?

(Looks good to me as well.)

cdce8p · 2023-02-21T02:03:03Z

Looks like this PR is against the main branch; should we also do a backport to v1.10.X?

That would be great. The plugin wouldn't work properly for the next mypy release (1.1.0) without this change.

hramezani · 2023-02-21T20:42:30Z

pydantic/mypy.py

+        assert info_metaclass
+        if getattr(info_metaclass.type, 'dataclass_transform_spec', None):
+            info_metaclass.type.dataclass_transform_spec = None  # type: ignore[attr-defined]
+        return None


Do we need this return here?

hramezani

LGTM 👍

Thanks @cdce8p for this.

It seems there are two Prs in mypy:

Fix crash with dataclass_transform cache python/mypy#14734 (Created by @cdce8p)
[dataclass_transform] detect transform spec changes in incremental mode python/mypy#14695

Seems we need this fix in main and 1.10.x whether your PR is merged or the other one

cdce8p · 2023-02-21T20:59:04Z

It seems there are two Prs in mypy:

python/mypy#14734 (Created by @cdce8p)

python/mypy#14695

Yeah, although both would address a different issue to this one.

Seems we need this fix in main and 1.10.x whether your PR is merged or the other one

Exactly 👍🏻

hramezani · 2023-02-21T22:20:29Z

Forgot to mention, I think we need a test for this change as well.

cdce8p · 2023-02-21T23:09:58Z

Forgot to mention, I think we need a test for this change as well.

That's a bit more complicated 😅
This PR is basically a proactive regression fix for a yet unreleased mypy version. There are already mypy tests in place and from a quick look I think those would fail without this PR if executed with the current mypy master.

Of course the obvious issue here is that the mypy change isn't released yet. So just in theory, there is a (very) slim chance the attribute could change which would require additional work here. However, with the getattr guard, it would just be a no-op and wouldn't cause any additional issues. That's also what would happen for mypy < 1.1 and what is already tested.
Personally, I don't think that will happen and having a new pydantic version out would help better test mypy, so I already opened this one.

If you really want to have a test first, it might be better to wait for the actual mypy release and see it break first. Or we move forward with the PR and I come back later to update the mypy version used for the tests.

dmontagu · 2023-02-21T23:13:24Z

@cdce8p @hramezani I think if the current mypy plugin tests fail if run using mypy 1.1.0, but pass with this change, then we might not need a new test.

If the current tests don't fail then I can help put one together quickly to get this across the line. (Should be straightforward to just copy/paste the example at the top of this into a new file in the tests/mypy folder.)

dmontagu · 2023-02-21T23:20:30Z

@cdce8p Does this mean that if using mypy without the plugin, anyone using Field will start getting errors? I'm wondering if we'll need to drop the @dataclass_transform because of that.. I'm happy using the plugin but I don't know if I love the idea that you get lots of errors doing things that should be okay unless you use the plugin (ideally there would be no false positives even without the plugin, and enabling it would just remove some of the false negatives).

cdce8p · 2023-02-22T00:08:55Z

@cdce8p Does this mean that if using mypy without the plugin, anyone using Field will start getting errors?

Not just with Field unfortunately. The pydantic syntax for arguments with defaults is incompatible with dataclass_transform. Thus, starting with mypy 1.1 basically everyone who is not using the plugin will get errors if they are missing some arguments even if they have default values. E.g.

from pydantic import BaseModel, Field

class Event(BaseModel):
    id: int | None = Field(None, alias="event")
    required_var: str | None = Field(...)
    optional_var: str | None

Event()

# mypy v1.0 (without plugin)
--

# mypy v1.0 (with plugin)
error: Missing named argument "required_var" for "Event"

# mypy v1.1.dev (without plugin)
error: Missing named argument "event" for "Event"  [call-arg]
error: Missing named argument "required_var" for "Event"  [call-arg]
error: Missing named argument "optional_var" for "Event"  [call-arg]

# mypy v1.1.dev (with current plugin v1.10.5)
error: Missing named argument "event" for "Event"  [call-arg]
error: Missing named argument "required_var" for "Event"  [call-arg]
error: Missing named argument "optional_var" for "Event"  [call-arg]

# mypy v1.1.dev (with plugin + this PR)
error: Missing named argument "required_var" for "Event"

I'm wondering if we'll need to drop the @dataclass_transform because of that..

Tbh I don't know if other type checker depend on it, mainly pyright. I would image they also have custom logic for pydantic just because of the default value issue. However IMO, the better alternative would be to adopt the "default" syntax for custom Fields. I.e. any argument is required unless it is assigned Field with an keyword only default argument.
It would be a massive breaking change, although still possible as there is a transition path. Field(default=<some value>) is already valid.

I think if the current mypy plugin tests fail if run using mypy 1.1.0, but pass with this change, then we might not need a new test.

Ran the mypy tests. test_mypy_results[mypy-default.ini-success.py-None] does fail without this change.

dmontagu · 2023-02-22T00:26:27Z

@samuelcolvin

Thus, starting with mypy 1.1 basically everyone who is not using the plugin will get errors if they are missing some arguments even if they have default values.

Want to bring this to your attention. ☝️

I.e. any argument is required unless it is assigned Field with an keyword only default argument.
@cdce8p Can you run the same except where you set a default not using Field? (i.e., other_var: str = "abc") — want to make sure that case still works okay.

I think it's reasonable to tell people they have to use default=<value> in Field to get it to work properly without the plugin, would be better if not but I suspect the benefits from @dataclass_transform might outweigh the costs. (Also, the Optional fields being still optional without a default value set is going to change in v2, so I'm a bit less concerned about that).

cdce8p · 2023-02-22T01:21:01Z

Thus, starting with mypy 1.1 basically everyone who is not using the plugin will get errors if they are missing some arguments even if they have default values.

Want to bring this to your attention. ☝️

Just to clarify, some example

class Event(BaseModel):
    # ok
    a: int | None = Field(default=None)
    b: int | None = None
    c: int
    d: int = Field(...)  # required -> only works without 'default' keyword

    # error
    e: int | None = Field(None)  # false-positive: not recognized as optional
    f: int | None                # false-positive: not recognized as optional
    g: int = ...                 # false-negative: not recognized as required; but different mypy error [assignment]

# Add empty constructor to show errors
Event()

cdce8p · 2023-02-22T01:28:37Z

I think it's reasonable to tell people they have to use default=<value> in Field to get it to work properly without the plugin, would be better if not but I suspect the benefits from @dataclass_transform might outweigh the costs. (Also, the Optional fields being still optional without a default value set is going to change in v2, so I'm a bit less concerned about that).

It might still makes sense to add this PR and not fall back to dataclass_transform entirely. One advantage of the plugin is that it parses the Model config, i.e. to determine if the alias should be used.

Never the less, syntax that more aligns with the stdlib dataclass would certainly help.

dmontagu · 2023-02-22T03:33:51Z

Oh yeah I didn’t mean to indicate we shouldn’t merge this PR

cdce8p · 2023-02-23T11:51:36Z

Just pushed a small addition which adds a version key to the plugin data. It's not strictly required for this PR as the plugin data is already changed but will help with any future changes if the mypy caches need to be invalidated.

I don't plan on adding anything else, so it would be ready to merge from my POV.

samuelcolvin

Sorry, just catching up on this.

In principle this looks good (although I don't pretend to understand all the nuances).

I'll discuss with @dmontagu today and hopefully merge

A few (perhaps related) questions:

should this be back-ported to the 1.10 pydantic branch?
do the changes to required logic in v2 affect this at all?

Thanks so much @cdce8p for digging into this and all your work.

samuelcolvin

Sorry, just catching up on this.

In principle this looks good (although I don't pretend to understand all the nuances).

I'll discuss with @dmontagu today and hopefully merge

A few (perhaps related) questions:

should this be back-ported to the 1.10 pydantic branch?
do the changes to required logic in v2 affect this at all?

Thanks so much @cdce8p for digging into this and all your work.

cdce8p · 2023-02-24T12:00:34Z

should this be back-ported to the 1.10 pydantic branch?

Yes

do the changes to required logic in v2 affect this at all?

No. The changes will improve the type checking experience for anyone not using the mypy plugin though. So they are definitely a step in the right direction. If I could suggest some more changes, I would recommend to

Remove support of g: int = ... -> This is an assignment error in mypy already and required fields can be shorted to g: int anyway.
Make Field.default a keyword only argument and remove support of Field(...). This will match PEP 681 and thus further improve the experience for anyone not using the plugin.
Similarly make FieldInfo.default a keyword only argument.

samuelcolvin · 2023-02-24T12:14:35Z

on your points:

the ellipsis is entirely optional (pun/confusion not intended) and has no effect (g: int = ... will mean exactly the the same thing as g: int in v2), we're only supporting it for backwards compatibility, we won't use it in any V2 docs except the migration guide
Again, I think we'll continue to support Field(42) rather than Field(default=42) for backwards compatibility reasons, but if you care about static typing, you can always use Field(default=42)
FieldInfo is only public so you can inspect .model_fields (perhaps we should make it private and add a protocol to represent it), it's not designed to be used publicly otherwise - so I imagine mypy shouldn't care about it?

cdce8p · 2023-02-24T12:55:25Z

the ellipsis is entirely optional (pun/confusion not intended) and has no effect (g: int = ... will mean exactly the the same thing as g: int in v2)

Not in the context of dataclass_transform unfortunately. Since ... is technically a value, g would be interpreted as optional.

Again, I think we'll continue to support Field(42) rather than Field(default=42) for backwards compatibility reasons, but if you care about static typing, you can always use Field(default=42)

True, however that doesn't work if the model is part of a library that is used somewhere else. Especially if the maintainer of said library doesn't want to change it only to better support static typing.

FieldInfo is only public so you can inspect .model_fields (perhaps we should make it private and add a protocol to represent it), it's not designed to be used publicly otherwise - so I imagine mypy shouldn't care about it?

Didn't know that. Only saw it in the dataclass_transform call as field_specifier. It should probably be removed there then

pydantic/pydantic/main.py

Lines 49 to 50 in ae481b9

    
           @typing_extensions.dataclass_transform(kw_only_default=True, field_specifiers=(Field, FieldInfo)) 
        
           class ModelMetaclass(ABCMeta):

pydantic/pydantic/dataclasses.py

Lines 175 to 176 in ae481b9

    
           @dataclass_transform(kw_only_default=True, field_specifiers=(Field, FieldInfo)) 
        
           def dataclass(

--

Tbh those suggestions would be nice but the pydantic mypy plugin resolves all of these issues, once mypy knows to ignore the dataclass_transform with this PR. It would however help for other type checker which don't have custom code for pydantic or if users don't use the plugin (maybe because they don't know about it).

samuelcolvin · 2023-02-24T16:34:17Z

@cdce8p we can add a recommendation to the docs to do those things "the right way", but we don't want to break V1 code unless we have to.

* Fix mypy plugin for 1.1.0 * Code review * Add version key to plugin data

KotlinIsland · 2023-02-27T04:35:12Z

Is this working with dataclasses? I'm seeing invalid errors:

import pydantic

@pydantic.dataclasses.dataclass
class A:
    a: str

A("")  # error: Too many positional arguments for "A"  [misc]

cdce8p · 2023-02-27T04:55:57Z

Is this working with dataclasses? I'm seeing invalid errors:

import pydantic

@pydantic.dataclasses.dataclass
class A:
    a: str

A("")  # error: Too many positional arguments for "A"  [misc]

Pydantic dataclasses are keyword only: https://docs.pydantic.dev/usage/dataclasses/
If that is wrong, the dataclass_transform decorator should be changed. So it's somewhat unrelated to the fix here for ModelMetaclass.

pydantic/pydantic/dataclasses.py

Lines 175 to 176 in 6267ae3

    
           @dataclass_transform(kw_only_default=True, field_specifiers=(Field, FieldInfo)) 
        
           def dataclass(

https://peps.python.org/pep-0681/#decorator-function-and-class-metaclass-parameters

KotlinIsland · 2023-02-27T05:01:16Z

@cdce8p I don't think they are keyword only as that example works correctly at runtime and with the pydantic mypy plugin with v1.0.1. It's only with this change that they are now reporting errors.

cdce8p · 2023-02-27T05:14:45Z

@cdce8p I don't think they are keyword only as that example works correctly at runtime and with the pydantic mypy plugin with v1.0.1. It's only with this change that they are now reporting errors.

Are you sure that it's this change? As it doesn't change anything regarding pydantic.dataclasses.dataclass, I wouldn't be so sure. What likely did trigger it was the mypy master branch itself as that starts parsing the dataclass_transform decorator.

KotlinIsland · 2023-02-27T05:23:55Z

I did mean that, that mypy caused this defect originally. But also that, this PR doesn't seem to be a complete fix, as the functionality of dataclassess has regressed.

samuelcolvin · 2023-02-27T10:30:20Z

pydantic dataclasses are not kwarg only, however we decorate them with dataclass_transform(kw_only_default=True, ..., see:

pydantic/pydantic/dataclasses.py

Line 96 in 314abca

@dataclass_transform(kw_only_default=True, field_specifiers=(Field, FieldInfo))

and other overloads.

Not sure why we did that, but I guess it could be changed?

cdce8p · 2023-03-07T05:13:04Z

Mypy v1.1 was released a few hours ago. As expected it broke the current pydantic plugin and now blocks the mypy update (at least for Home Assistant). It would be awesome if the changes here could be released. The cherry-pick PR would be ready #5111.

It could make sense to include #5120 as well, although that was already broken.

https://mypy-lang.blogspot.com/2023/03/mypy-111-released.html

dmontagu · 2023-03-07T21:05:55Z

@cdce8p Do you mind if I merge the stuff from #5120 directly into this branch, along with some changes to tests to get everything behaving nicely? If so, I'll take care of it and I think we can have this ready to merge first thing tomorrow. (If I don't hear from you in the next few hours I might just start pushing..)

cdce8p · 2023-03-07T21:09:28Z

@cdce8p Do you mind if I merge the stuff from #5120 directly into this branch, along with some changes to tests to get everything behaving nicely? If so, I'll take care of it and I think we can have this ready to merge first thing tomorrow. (If I don't hear from you in the next few hours I might just start pushing..)

Not at all! Please let me know if I can help.

dmontagu · 2023-03-07T21:44:53Z

(And to be clear, I meant the cherry-pick branch for 1.10.X, not this branch.)

My current plan is to make it so that you can get strict/lenient behavior on mypy 1.1.1 by using the plugin, but without the plugin, when using mypy 1.1.1 it becomes "strict" in accordance with mypy's handling of @dataclass_transform. Anyone who both doesn't want to use the plugin AND wants mypy to be lenient, will basically just need to not upgrade to mypy 1.1.1.

@cdce8p would that work for you?

I will add some commits onto the cherry-pick branch, once I've done I'll notify you and it would be great if you could confirm if the changes work for you on Home Assistant.

cdce8p · 2023-03-07T21:52:30Z

My current plan is to make it so that you can get strict/lenient behavior on mypy 1.1.1 by using the plugin, but without the plugin, when using mypy 1.1.1 it becomes "strict" in accordance with mypy's handling of @dataclass_transform. Anyone who both doesn't want to use the plugin AND wants mypy to be lenient, will basically just need to not upgrade to mypy 1.1.1.

@cdce8p would that work for you?

That sounds like exactly what this PR / the cherry-pick one does, so yes. Basically the PR only restores the plugin behavior for mypy 1.1.1, without the plugin the dataclass_transform is used.

I will add some commits onto the cherry-pick branch, once I've done I'll notify you and it would be great if you could confirm if the changes work for you on Home Assistant.

Sounds good

--
Do you want to include a backport for #5120 as well? It also deals with the dataclass_transform decorator. The PR is ready. Just needs to be backported after it's merged.

dmontagu · 2023-03-07T22:19:55Z

Yeah I realized that I didn't need to do anything because I wasn't going to make the tests actually pass on 1.1.1. So I just merged your changes in there.

* Fix mypy plugin for 1.1.0 (#5077) * Fix mypy plugin for 1.1.0 * Code review * Add version key to plugin data (cherry picked from commit 6267ae3) * Change file name * Add the changes from #5120 * Update changes file * Remove additional unneeded dataclass import (from #5120) --------- Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>

Mypy 1.1.1 throws errors with pydantic models that specify default values with positional arguments. The next pydantic version will probably fix it by updating the mypy-pydantic plugin, but converting the default value to a keyword argument is no big deal either. So let's fix it today. For more info pydantic/pydantic#5077

andreagrandi · 2023-03-16T13:05:08Z

from __future__ import annotations

from pydantic import BaseModel, Field

class Event(BaseModel):
    id: int | None = Field(None, alias="id")

Event()

Hi, first of all thank you for you patch, but I'm still having this issue with pydantic==1.10.6 (which afaik should include this patch) and mypy==1.1.1

I was getting a lot of these errors, so I added to my code exactly your same class:

class Event(BaseModel):
    id: int | None = Field(None, alias="id")

and when I run mypy checks, I get: error: Missing named argument "id" for "Event" [call-arg]
I've also tried without the Field(None, alias="id") part and I get the same error.

~~Was there a further change/regression from mypy==1.1.0 and mypy==1.1.1~~ ?

update: there was no 1.1.0 release, only 1.1.1 so the problem seems to be still there.

Thanks

dmontagu · 2023-03-16T13:15:39Z

@andreagrandi See #5192 — I'll make sure that we address this once we get back to looking at the mypy plugin again (hopefully not too far out).

andreagrandi · 2023-03-16T13:24:39Z

@dmontagu thank you for the quick reply and my bad I didn't check among existing issues 🙇🏻 I will stay tuned, thanks again!

We need to use the pydantic mypy plugin to work around the breaking changes in mypy 1.1 that required pydantic/pydantic#5077

getting dozens of mypy errors with upgraded pydantic (1.10.6) and mypy (1.1.1); ... ``` tests/test_transform.py:57: error: Missing named argument "doaj_id" for "ScholarBiblio" [call-arg] tests/test_transform.py:57: error: Missing named argument "dblp_id" for "ScholarBiblio" [call-arg] tests/test_transform.py:57: error: Missing named argument "oai_id" for "ScholarBiblio" [call-arg] ``` related: pydantic/pydantic#5077 For the moment, try to follow dependency upgrades to not let the gap become too larget; albeit these issues are time-consuming to sort out.

cdce8p mentioned this pull request Feb 19, 2023

Fix crash with dataclass_transform cache python/mypy#14734

Closed

Fix mypy plugin for 1.1.0

9e109a1

yezz123 approved these changes Feb 20, 2023

View reviewed changes

cdce8p mentioned this pull request Feb 21, 2023

Mypy plugin doesn't consider allow_population_by_field_name #5070

Closed

15 tasks

hramezani reviewed Feb 21, 2023

View reviewed changes

hramezani approved these changes Feb 21, 2023

View reviewed changes

Code review

005c4ac

Add version key to plugin data

4c7d5e5

dmontagu approved these changes Feb 23, 2023

View reviewed changes

samuelcolvin added the cherry-pick-v1 cherry-pick back on 1.10.X-fixes label Feb 24, 2023

samuelcolvin approved these changes Feb 24, 2023

View reviewed changes

dmontagu merged commit 6267ae3 into pydantic:main Feb 24, 2023

cdce8p deleted the mypy-dataclass_transform branch February 24, 2023 16:50

github-actions bot mentioned this pull request Feb 24, 2023

[cherry-pick] Fix mypy plugin for 1.1.0 #5110

Closed

github-actions bot pushed a commit that referenced this pull request Feb 24, 2023

Fix mypy plugin for 1.1.0 (#5077)

a7c0ce7

* Fix mypy plugin for 1.1.0 * Code review * Add version key to plugin data

cdce8p mentioned this pull request Feb 27, 2023

[cherry-pick] Fix mypy plugin for 1.1.0 (#5077) #5111

Merged

KotlinIsland mentioned this pull request Feb 27, 2023

(🐞) Mypy plugin doesn't work with positional arguments on dataclasses #5117

Closed

cdce8p mentioned this pull request Mar 7, 2023

Update mypy to 1.1.1 home-assistant/core#89268

Merged

19 tasks

jolynch added a commit to Netflix-Skunkworks/service-capacity-modeling that referenced this pull request Mar 21, 2023

Hotfix for mypy 1.1 breakage

bb70adc

We need to use the pydantic mypy plugin to work around the breaking changes in mypy 1.1 that required pydantic/pydantic#5077

Fix mypy plugin for 1.1.0 #5077

Fix mypy plugin for 1.1.0 #5077

Conversation

cdce8p commented Feb 19, 2023

yezz123 left a comment

Choose a reason for hiding this comment

dmontagu commented Feb 21, 2023

cdce8p commented Feb 21, 2023

hramezani Feb 21, 2023

Choose a reason for hiding this comment

hramezani left a comment • edited

Choose a reason for hiding this comment

cdce8p commented Feb 21, 2023

hramezani commented Feb 21, 2023

cdce8p commented Feb 21, 2023

dmontagu commented Feb 21, 2023 • edited

dmontagu commented Feb 21, 2023 • edited

cdce8p commented Feb 22, 2023

dmontagu commented Feb 22, 2023 • edited

cdce8p commented Feb 22, 2023 • edited

cdce8p commented Feb 22, 2023

dmontagu commented Feb 22, 2023

cdce8p commented Feb 23, 2023

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin left a comment

Choose a reason for hiding this comment

cdce8p commented Feb 24, 2023 • edited

samuelcolvin commented Feb 24, 2023 • edited

cdce8p commented Feb 24, 2023

samuelcolvin commented Feb 24, 2023

KotlinIsland commented Feb 27, 2023

cdce8p commented Feb 27, 2023 • edited

KotlinIsland commented Feb 27, 2023

cdce8p commented Feb 27, 2023 • edited

KotlinIsland commented Feb 27, 2023

samuelcolvin commented Feb 27, 2023

cdce8p commented Mar 7, 2023 • edited

dmontagu commented Mar 7, 2023

cdce8p commented Mar 7, 2023

dmontagu commented Mar 7, 2023

cdce8p commented Mar 7, 2023

dmontagu commented Mar 7, 2023

andreagrandi commented Mar 16, 2023 • edited

dmontagu commented Mar 16, 2023

andreagrandi commented Mar 16, 2023

hramezani left a comment •

edited

dmontagu commented Feb 21, 2023 •

edited

dmontagu commented Feb 21, 2023 •

edited

dmontagu commented Feb 22, 2023 •

edited

cdce8p commented Feb 22, 2023 •

edited

cdce8p commented Feb 24, 2023 •

edited

samuelcolvin commented Feb 24, 2023 •

edited

cdce8p commented Feb 27, 2023 •

edited

cdce8p commented Feb 27, 2023 •

edited

cdce8p commented Mar 7, 2023 •

edited

andreagrandi commented Mar 16, 2023 •

edited