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
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_extensions
functions seem to expect theCallable
generics to be[...]
). Might get just as messy again to add the shim if those are truly valid cases (creating aModel
with a bareCallable
field 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 anAnnotated
field, Pydantic raises:This PR adds support for
Annotated
by simply unwrapping the root type inModelField
. The fullAnnotated
information is still visible withget_type_hints(x, include_extras=True)
on theBaseModel
subclasses.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>.md
file added describing change(see changes/README.md for details)