-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Support Annotated type hints and extracting Field from Annotated #2147
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
Support Annotated type hints and extracting Field from Annotated #2147
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2147 +/- ##
===========================================
- Coverage 100.00% 99.97% -0.03%
===========================================
Files 23 23
Lines 4426 4477 +51
Branches 888 903 +15
===========================================
+ Hits 4426 4476 +50
- Misses 0 1 +1
Continue to review full report at Codecov.
|
dc9faa3 to
b183faa
Compare
|
On field "x" the following field constraints are set but not enforced
Tests are passing. |
|
I think this looks like a great start, but I think before we merge we need:
|
539abb1 to
3337876
Compare
3337876 to
b72dfa9
Compare
I'll dig in. Planning to add two parts:
|
b72dfa9 to
f5ba8e7
Compare
|
@samuelcolvin any chance we could make |
e3e5f48 to
6fd8826
Compare
|
Looks like we need to either bump the docs builder to py3.9, install typing_extensions, or I can just inline the example (so it's not run). Any preferences? I also added a few failing tests around defaults - I need to add:
|
|
I don't think we can (cleanly) "error if the Field in Annotated has a default set" since we also need to support
edit: Error cases cleaned up a bit with some more refactoring, so I opted for case 1 above. |
255621e to
d7ac367
Compare
|
@samuelcolvin I think this is ready for another look! There may be a bit of a usage disconnect between |
d7ac367 to
8e5d016
Compare
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.
this is looking great, a few things to fix.
pydantic/typing.py
Outdated
| # typing_extensions.Annotated, so wrap them to short-circuit. We still want to use our wrapped | ||
| # get_origins defined above for non-Annotated data. | ||
|
|
||
| def get_args(tp: Type[Any], _get_args=get_args) -> Type[Any]: |
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.
I think get_args is already defined in this file, we should either:
- use that version
- add this logic to that method
- give this a different name
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.
1/3: We need the "Annotated aware" version in all places as the "default".
2: This probably gets tricky without larger "refactor now that typing-extensions is always available" changes, I think I'd have to add this logic to 3 versions of get_args and 2 versions of get_origin (the ones defined above) vs just wrapping whatever is decided above. Let me know if you'd like to just dup the check in those or refactor a bit (presuming #2147 (comment)).
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.
I was able to refactor the get_args/get_origin stuff a bit with typing_extensions, but it's not a panacea:
- those helper funcs aren't available on 3.6 so I kept the old "polyfills"
- I had to remove two tests using a bare
Callable(thetyping_extensionsfunctions seem to expect theCallablegenerics to be[...]). Might get just as messy again to add the shim if those are truly valid cases (creating aModelwith a bareCallablefield hint seemed to still work) - Looks like this makes the "test compiled without deps" fail (since we now import
typing_extensions). Does this need to be tweaked somehow?
I'm content reverting those changes and/or doing something else if you prefer.
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.
Given that ^ messiness, I've pulled those changes (will reserve them for a future typing_extensions PR). How do the latest commits look?
| @@ -130,7 +130,7 @@ def extra(self): | |||
| ], | |||
| extras_require={ | |||
| 'email': ['email-validator>=1.0.3'], | |||
| 'typing_extensions': ['typing-extensions>=3.7.2'], | |||
| 'typing_extensions': ['typing-extensions>=3.7.4'], | |||
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.
I think we should move this to install_requires but perhaps that should be a separate PR.
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.
I'm happy to do this in this PR - it would may greatly simplify #2147 (comment). Perhaps I leave a lot of the other cleanup out of this PR though to reduce noise.
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.
I'll leave it out in the sake of getting this PR in. I have a separate branch with partial support I can push after this.
8e5d016 to
05b2ee5
Compare
fac50da to
769a006
Compare
bb6bd51 to
769a006
Compare
769a006 to
aee676f
Compare
|
Good on my end - open discussions: |
|
I've added a test for my use case in https://github.com/thomascobb/pydantic/blob/support-annotated-fields/tests/test_decorator.py: def test_annotated_field_reuse():
AOne = Annotated[int, Field(description='A long description of `one` we don\'t want to repeat')]
class Model(BaseModel):
one: AOne
two: Annotated[int, Field(description='An optional integer')] = 3
@classmethod
@validate_arguments
def alternative(
cls,
one: AOne,
have_two: Annotated[bool, Field(description='Include two?')],
) -> 'Model':
return cls(one=one, two=2 if have_two else 0)
assert Model(one=1).dict() == dict(one=1, two=3)
assert Model.alternative(one=1, have_two=False) == dict(one=1, two=0)This is failing for a few reasons. First is in the type hint evaluations: If I remove the Finally, if I run Any ideas? |
|
@thomascobb The |
Thanks for looking into things! Do you mind linking to or describing some of the other issues (aside from @thomascobb's) in case I or others have a chance to check them out? If this gets too messy, I'm also happy to split this PR up into 2 parts - the first PR to just not break on unrelated @thomascobb
This part is unrelated to this PR/doesn't work on broken minimum test_model_forward_refdef test_model_forward_ref():
from pydantic.decorator import validate_arguments
class Model(BaseModel):
one: int
@classmethod
@validate_arguments
def alternative(cls, one) -> 'Model': # still fails
return cls(one=one)I think it's due to the working test_model_forward_refdef test_model_forward_ref():
from pydantic.decorator import validate_arguments
class Model(BaseModel):
one: int
@classmethod
@validate_arguments
def alternative(cls, one) -> Model:
return cls(one=one)
Model.alternative = alternative |
|
The PR that can't be merged yet is the fix I made for |
Thanks, that fixes it
Good point. The following works though, and it probably more correct... from typing import TypeVar
def test_annotated_field_reuse():
T = TypeVar("T")
AOne = Annotated[int, Field(description='A long description of `one` we don\'t want to repeat')]
class Model(BaseModel):
one: AOne
two: Annotated[int, Field(description='An optional integer')] = 3
@classmethod
@validate_arguments
def alternative(
cls: T,
one: AOne,
have_two: Annotated[bool, Field(description='Include two?')],
) -> T:
return cls(one=one, two=2 if have_two else 0)
assert Model(one=1).dict() == dict(one=1, two=3)
assert Model.alternative(one=1, have_two=False) == dict(one=1, two=0) |
|
I'm a bit unclear on this. Is there anything (except conflicts which are trivial) which is blocking this? I would be keen to get it merged asap to include in v1.8 if possible. |
|
No blockers I'm aware of, just another round of reviews. I'll push a conflict fix for reqs in a min. |
| except ImportError: | ||
| # Create mock Annotated values distinct from `None`, which is a valid `get_origin` | ||
| # return value. | ||
| class _FalseMeta(type): |
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.
this is clever, even if it's sad we have to do it. 🙃
|
|
||
|
|
||
| @pytest.mark.skipif(not Annotated, reason='typing_extensions not installed') | ||
| @pytest.mark.parametrize( |
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.
I think this is okay, but as a piece of feedback, it's generally a good idea to include a simple test that demonstrates the basic behaviour at the top of test file. All this parametrize stuff is very clever but it's not that easy to read or reason with when things break.
You could also use pytest.importorskip here to avoid lots of skipif, or just pytestmark = ....
Change Summary
As discussed elsewhere (1, 2), Pydantic doesn't support
typing.Annotated/typing_extensions.Annotated. When using anAnnotatedfield, Pydantic raises:This PR adds support for
Annotatedby simply unwrapping the root type inModelField. The fullAnnotatedinformation is still visible withget_type_hints(x, include_extras=True)on theBaseModelsubclasses.I'm not sure if this needs doc updates, perhaps just a small note saying "Annotated type hints are supported"?
Separately, I'm working on a prototype for #2129 in https://github.com/JacobHayes/pydantic-annotated which expands on this functionality. I have a small check (based on the presence of
pydantic.typing.Annotated) in there to do the unwrapping until this or similar is merged.Related issue number
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)