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 Literal bug with typing-extension==4.6.0 #5826

Merged
merged 6 commits into from May 23, 2023
Merged

Conversation

hramezani
Copy link
Member

Fixes #5821

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 23, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2ed3ca1
Status:🚫  Build failed.

View logs

pydantic/typing.py Outdated Show resolved Hide resolved
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!

tests/test_typing.py Outdated Show resolved Hide resolved
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!

@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

@samuelcolvin
Copy link
Member

samuelcolvin commented May 23, 2023

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

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.

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.

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

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!

rmitsch pushed a commit to explosion/spacy-llm that referenced this pull request May 23, 2023
@hramezani hramezani merged commit c4d3dee into 1.10.X-fixes May 23, 2023
49 of 50 checks passed
@hramezani hramezani deleted the is_literal_type branch May 23, 2023 14:44
@dmontagu
Copy link
Contributor

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

@samuelcolvin
Copy link
Member

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

Done

frnhr added a commit to cognitedata/pygen that referenced this pull request Jun 9, 2023
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

6 participants