-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Refactor namespace logic for annotations evaluation #10530
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
Merged
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
d32107f
Add utils related to namespace management
Viicos e5c4207
Add temporary xfailing tests that we will hopefully fix
Viicos 9ef1965
Improve correctness in `get_cls_type_hints_lenient`
Viicos 1da3698
Improve annotations resolving for functions
Viicos f4f2ae8
Adapt `collect_model_fields` to handle the parent namespace
Viicos bb7ffff
Adapt the `GenerateSchema` class to use the `NsResolver` class
Viicos 2835c7f
Update `ModelMetaclass` to be compatible with the previous changes
Viicos 9b77d05
Change `BaseModel.model_rebuid` logic to match the proposed spec
Viicos 132c06c
Make the dataclass logic compatible with the proposed changes
Viicos b40468e
Refactor how we eval fields and generate a schema for dataclasses
Viicos cee89ad
Change `rebuild_dataclass` logic to match the proposed spec
Viicos 24338c9
Change `validate_call` namespace logic
Viicos 6a86ac8
Adapt `TypeAdapter` namespace logic
Viicos 93bd021
Misc. changes to adapt with the new structures
Viicos 7c3bf4f
Process some feedback
Viicos dd7e44c
Fix comment
Viicos 7ae989e
WIP refactor to be more backwards compatible regarding parent ns
Viicos d2d549d
WIP fix remaining tests
Viicos e182033
Almost finished
Viicos 89199ba
First round of feedback
Viicos 127aefe
Cleanup xfail tests, last fixes
Viicos 66c024e
Add back `eval_type_lenient` and deprecate it
Viicos 2a371ca
lint
Viicos 8d57b83
Fix pipeline example
Viicos 7861286
Compat fixes, optimize `get_cls_type_hints`
Viicos 08691ec
Update outdated comment
Viicos 86fe4cc
Feedback
Viicos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @adriangb I think this is broken by this PR but maybe worth breaking. I think it's possible to fix by fiddling with the
__get_pydantic_core_schema__on the Pipeline and making sure we plumb through the namespace stuff, but maybe harder than it's worth doing this secondThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Viicos decided this was worth breaking. As long as there's clear documentation showing the alternative path forward I'm okay with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy pasting what I added on Slack so that it's not lost:
the TL;DR is using lambdas with references to other symbols in annotations opens the door to a lot of weird behaviors.
This is the (simplified) test failing on the PR:
On
main, when we evaluate (read: Python will call eval) the annotation for family, we use the following as globals:{'User': __main__.User, 'Annotated': ..., 'BaseModel': ..., ...}and locals are empty.
On this PR, we cleaned up how globals and locals were mixed up before. This means that we now use the following as globals:
{'Annotated': ..., 'BaseModel': ..., ...}and locals:
{'User': __main__.User, ...}And the issue comes from what could be considered as a limitation of
eval. Consider this example:The
evallimitation is that it does not have access to the non-locals of thelambdaenvironment (which is a new scope, like with adefstatement). Even thoughAis present in locals, it won't be used to resolveAand soevalwill look up in theglobalsinstead (that's why it works onmainbecauseUserwas added inglobalsfor theevalcall).This limitation is documented in this open CPython PR.