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

Fix false positives on v1 models in mypy plugin for from_orm check requiring from_attributes=True config #9938

Merged

Conversation

radekwlsk
Copy link
Contributor

@radekwlsk radekwlsk commented Jul 22, 2024

Change Summary

This is a fix for bug reported in #9936 when using both v1 and v2 for migration period with pydantic.mypy plugin enabled the "<ModelName>" does not have from_attributes=True [pydantic-orm] error is getting raised for pydantic.v1.BaseModel models that should not be checked by pydantic.mypy plugin (that is supposed to work on current - v2 - pydantic).

Please review but note that test will fail unless the other bug with @typing_extensions.deprecated decorator mentioned in #9936 is fixed.

Open for discussion how to address the decorator messing with mypy hooks.

Related issue number

fix #9936

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

Selected Reviewer: @sydney-runkle

Copy link

codspeed-hq bot commented Jul 22, 2024

CodSpeed Performance Report

Merging #9938 will not alter performance

Comparing radekwlsk:radekwlsk/fix-mypy-plugin-pydantic-orm (d3f6567) with main (fe08181)

Summary

✅ 14 untouched benchmarks

Copy link
Contributor

@dmontagu dmontagu left a comment

Choose a reason for hiding this comment

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

This looks good to me, other than — can we try removing the duplicates of the from_orm_v1_noconflict.py file so we only keep the one in the 1.0.1 folder? I think that will still pass CI since it seems there are no changes in the subsequent versions.

@dmontagu
Copy link
Contributor

Unfortunately it seems that it may be difficult to resolve the issue with deprecated, as I think mypy doesn't have a way for plugins to override the behavior of decorated methods. I might be misinterpreting something, but it seems that python/mypy#9915 might be necessary to achieve this (not sure).

I think there are things we could do to avoid the decorator affecting the plugin behavior, but I would expect that to get in the way of some desirable errors produced by calling deprecated methods. So I'm not sure what's best here

@radekwlsk
Copy link
Contributor Author

radekwlsk commented Jul 23, 2024

@dmontagu Thanks for the review, removed the duplicate result files. For the decorator I have no clue how to approach it without breaking the deprecation warning either.

One way to "resolve" it would be to focus the unit of test on not raising "pydantic-orm" error for v1.BaseModel and skip the case for raising it where expected. WDYT? See the last commit.

@radekwlsk radekwlsk requested a review from dmontagu July 23, 2024 07:09
@radekwlsk
Copy link
Contributor Author

This looks good to me, other than — can we try removing the duplicates of the from_orm_v1_noconflict.py file so we only keep the one in the 1.0.1 folder? I think that will still pass CI since it seems there are no changes in the subsequent versions.

CI passes as expected, thanks.

@radekwlsk
Copy link
Contributor Author

@dmontagu Please re-review. Is there a hope to get this fix included in a 2.8 patch release without waiting for 2.9 rollout?

@radekwlsk radekwlsk marked this pull request as draft July 29, 2024 07:16
@radekwlsk radekwlsk marked this pull request as ready for review July 29, 2024 07:16
@radekwlsk
Copy link
Contributor Author

Please review
It is stuck in waiting for review for over a week now 😞

@sydney-runkle
Copy link
Member

@radekwlsk,

Thanks for the ping. I messaged @dmontagu and we'll take a look shortly!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you shouldn’t need this file at all if there are no mypy errors produced

@sydney-runkle sydney-runkle merged commit 22265f6 into pydantic:main Jul 31, 2024
60 checks passed
@radekwlsk
Copy link
Contributor Author

@sydney-runkle Are you able to provide any timeline for release containing that fix?

@radekwlsk radekwlsk deleted the radekwlsk/fix-mypy-plugin-pydantic-orm branch August 1, 2024 12:25
@sydney-runkle
Copy link
Member

@radekwlsk,

Was planning on releasing this with v2.9 in late August!

@radekwlsk
Copy link
Contributor Author

So I guess I shouldn't wait for 2.8.3 with bugfixes? Thanks, I'll fork-patch mypy plugin locally for the time being.

@sydney-runkle
Copy link
Member

Right, I don't think we're in a rush for a v2.8.3 at the moment, but I'll keep you updated!

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