Skip to content

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

Merged
hramezani merged 6 commits into1.10.X-fixesfrom
is_literal_type
May 23, 2023
Merged

Fix Literal bug with typing-extension==4.6.0#5826
hramezani merged 6 commits into1.10.X-fixesfrom
is_literal_type

Conversation

@hramezani
Copy link
Copy Markdown
Member

@hramezani hramezani commented May 23, 2023

Fixes #5821

@cloudflare-workers-and-pages
Copy link
Copy Markdown

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
Comment thread pydantic/typing.py Outdated
AlexWaygood
AlexWaygood reviewed May 23, 2023
View reviewed changes
Copy link
Copy Markdown
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!

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

hramezani commented May 23, 2023

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
Copy Markdown

smartYSC commented May 23, 2023

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

@sanderr
Copy link
Copy Markdown

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
Copy Markdown
Contributor

AlexWaygood commented May 23, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

samuelcolvin commented May 23, 2023

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
Copy Markdown
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.

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

@samuelcolvin samuelcolvin May 23, 2023

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
Copy Markdown
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
Copy Markdown
Member

@samuelcolvin samuelcolvin May 23, 2023

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
Copy Markdown
Member Author

@hramezani hramezani May 23, 2023

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
Copy Markdown
Contributor

dmontagu commented May 23, 2023

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
Copy Markdown
Member

samuelcolvin commented May 23, 2023

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 samuelcolvin left review comments

@dmontagu dmontagu dmontagu approved these changes

+1 more reviewer

@AlexWaygood AlexWaygood AlexWaygood left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hramezani @smartYSC @sanderr @AlexWaygood @samuelcolvin @dmontagu