Skip to content

Fix Literal bug with typing-extension==4.6.0 #5826

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 23, 2023
Merged

Fix Literal bug with typing-extension==4.6.0 #5826

merged 6 commits into from
May 23, 2023
+45 −7

Conversation

hramezani
Copy link
Member

Fixes #5821

@hramezani
Fix Literal bug with typing-extension==4.6.0

Verified

This commit was signed with the committer’s verified signature.
IvanGoncharov Ivan Goncharov
GPG key ID: 565A593B3E13C939
Verified
Learn about vigilant mode
93ef367
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 23, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2ed3ca1
Status:🚫  Build failed.

View logs

AlexWaygood
AlexWaygood reviewed May 23, 2023
View reviewed changes
AlexWaygood
AlexWaygood reviewed May 23, 2023
View reviewed changes
Copy link
Contributor

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I'm not familiar with pydantic internals, but this looks like a reasonable fix to me!

@hramezani @AlexWaygood
Update tests/test_typing.py
c009124 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@hramezani
Copy link
Member Author

I'm not familiar with pydantic internals, but this looks like a reasonable fix to me!

Sure. Thanks!

@AlexWaygood AlexWaygood mentioned this pull request May 23, 2023
Closed
@smartYSC
Copy link

Will this fix be delivered as Pydantic 1.10.8 immediately? If yes, I will wait and not implement any workarounds.

@sanderr
Copy link

sanderr commented May 23, 2023

I wonder if it might not be a good idea to introduce an upper bound (<5) on the typing-extensions package as well. The project declares that they follow semantic versioning so they should only introduce breaking changes in major releases. Given how this issue has caused problems on a lot of projects, I think it would be a good idea to protect against similar issues when typing-extensions gets a major release. It wouldn't have prevented this specific issue because this really was a bug in pydantic, but it could help prevent similar issues in the future.

The same applies to the master branch of course.

@AlexWaygood
Copy link
Contributor

Honestly, I feel like this situation illustrates the inadequacies of SemVer more than anything else

@hramezani hramezani requested a review from samuelcolvin May 23, 2023 11:26
@samuelcolvin
Copy link
Member

samuelcolvin commented May 23, 2023 •
edited
Loading

Honestly, I feel like this situation illustrates the inadequacies of SemVer more than anything else

agreed.

I wonder if it might not be a good idea to introduce an upper bound (<5) on the typing-extensions package as well.

No. There are many articles out there about why including upper bounds in package dependencies is almost always a bad idea.

Will this fix be delivered as Pydantic 1.10.8 immediately? If yes, I will wait and not implement any workarounds.

We'll do our best to get it out asap.

@AlexWaygood
Copy link
Contributor

AlexWaygood commented May 23, 2023 •
edited
Loading

Although I don't feel like typing_extensions is at fault here (and this was a change that pydantic folks originally asked for, anyway 😉), I would nonetheless like to apologise for the breakage here. I didn't anticipate that reimplementing Literal on Python 3.8-3.9 at typing_extensions could have the capacity to break pydantic in this way and cause so much disruption.

@samuelcolvin
Copy link
Member

No problem @AlexWaygood, these things happen. It's the responsibility of app developers to pin dependencies.

CF build failures is a random download issue, see my angry tweet.

@AlexWaygood AlexWaygood mentioned this pull request May 23, 2023
Closed
samuelcolvin
samuelcolvin reviewed May 23, 2023
View reviewed changes
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

pydantic/typing.py
if type_ is none_type:
return True
return False
return type_ in NONE_TYPES
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird enough that we might have done it for some strange reason, can we confirm this was just a code smell?

Copy link
Member Author

@hramezani hramezani May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The existing approach(checking with is) does not work in Python 3.9 and typing-extension==4.6.0. That's why I've changed it.

Note: the new approach is also Ok with typing-extension==4.5.0. So, it might be a wrong implementation before

Copy link
Member

Choose a reason for hiding this comment

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

thanks @hramezani.

Can we uprev the version we test with?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@mirekdlugosz mirekdlugosz mentioned this pull request May 23, 2023
Merged
@tand826 tand826 mentioned this pull request May 23, 2023
Closed
14 tasks
@tcompa tcompa mentioned this pull request May 23, 2023
Closed
@mikeweltevrede mikeweltevrede mentioned this pull request May 23, 2023
Closed
@tcompa tcompa mentioned this pull request May 23, 2023
Closed
bdura added a commit to explosion/spacy-llm that referenced this pull request May 23, 2023
@bdura
ci: temporarily disable fail-fast
2ca6422 
(pydantic/pydantic#5826)
@bdura bdura mentioned this pull request May 23, 2023
Merged
5 tasks
rmitsch pushed a commit to explosion/spacy-llm that referenced this pull request May 23, 2023
@bdura
ci: temporarily disable fail-fast (#116)
0687232 
(pydantic/pydantic#5826)
@hramezani hramezani merged commit c4d3dee into 1.10.X-fixes May 23, 2023
@hramezani hramezani deleted the is_literal_type branch May 23, 2023 14:44
@stefsmeets stefsmeets mentioned this pull request May 23, 2023
Closed
@dmontagu
Copy link
Contributor

This should be fixed now in the just-released 1.10.8, which should be available through PyPI shortly.

@AlexWaygood AlexWaygood mentioned this pull request May 23, 2023
Closed
@samuelcolvin
Copy link
Member

This should be fixed now in the just-released 1.10.8, which should be available through PyPI shortly.

Done

@desertaxle desertaxle mentioned this pull request May 23, 2023
Closed
4 tasks
@Kilo59 Kilo59 mentioned this pull request May 23, 2023
Merged
4 tasks
@JaskRendix JaskRendix mentioned this pull request May 23, 2023
Closed
@AlexWaygood AlexWaygood mentioned this pull request May 26, 2023
Closed
@temichus temichus mentioned this pull request May 29, 2023
Merged
7 tasks
This was referenced Jun 1, 2023
Closed
Closed
Merged
frnhr added a commit to cognitedata/pygen that referenced this pull request Jun 9, 2023
@frnhr
typing-extensions 4.6 broke pydantic
9e5f16a 
pydantic/pydantic#5826
@frnhr frnhr mentioned this pull request Jun 9, 2023
Closed
4 tasks
@devstein devstein mentioned this pull request Jun 16, 2023
Closed
14 tasks
This was referenced Jul 8, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelcolvin samuelcolvin

@AlexWaygood AlexWaygood

@dmontagu dmontagu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@hramezani @smartYSC @sanderr @AlexWaygood @samuelcolvin @dmontagu