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

PEP 563, PEP 649 and pydantic #2678

Closed
samuelcolvin opened this issue Apr 15, 2021 · 45 comments
Closed

PEP 563, PEP 649 and pydantic #2678

samuelcolvin opened this issue Apr 15, 2021 · 45 comments
Labels

Comments

@samuelcolvin
Copy link
Member

samuelcolvin commented Apr 15, 2021

Update 2: see #2678 (comment), for a summary of how PEP 563 could effect pydantic.


Update: see below this has been resolved by a changes in python 3.10 from the python steering council.

Thanks everyone for your thoughts, patience and hard work.


PEP 563 "Postponed Evaluation of Annotations" was introduced in python 3.7 behind the from __future__ import annotations switch, it basically meant all annotations are strings, not python objects.

Pydantic has tried to supported Postponed Annotations since v0.18.0, see #348.

The problem however is that trying to evaluate those strings to get the real annotation objects is really hard, perhaps impossible to always do correctly, see #248 #234 #397 #415 #486 #531 #545 #635 #655 #704 #1298 #1332 #1345 #1370 #1668 #1736 #1873 #1895 #2112 #2314 #2323 #2411

The reasons are complicated but basically typing.get_type_hints() doesn't work all the time and neither do the numerous hacks we've introduced to try and get fix it. Even if typing.get_type_hints() was faultless, it would still be massively slower than the current semantics or PEP 649 (see below).

In short - pydantic doesn't work very well with postponed annotations, perhaps it never will.

The Problem

The problem is that postponed annotations are set to be come default in 3.10, features of which will be frozen in about three week.

Even worse, there's no way to switch back to the current behaviour.

The Solution

The solution to this is PEP 649 developed by Larry Hastings. This basically means annotations evaluation is lazy - the work of building __annotations__ isn't done until you access __annotations__.

As far as I can tell this is the best of both worlds.

The Second Problem

The sad reality however is that it seems very possible that PEP 649 will get rejected and when python 3.10 is released it will break much of pydantic, and thereby FastAPI and all the other libraries that rely on pydantic (as well as other libraries that use annotations at runtime like enforce and typer presumably).

See this very long discussion of the issue on python-dev about whether PEP 649 is a good idea.


The Point

So why am I saying all this apart from whinging about something I don't agree with?

1. This is fair warning

That pydantic might break in a big way if python's core developers continue value principle over pragmatism.

2. You can help

Thousands of developers and organisations big and small rely on pydantic. Type hints might have been conceived to help readers and static type checkers, but they're now used to great effect at runtime by lots and lots of people - they make python better.

In the end I don't think python's core developers and steering council want to make your experience of python worse. They just haven't realised how important this is. (Even Larry hadn't heard of pydantic or FastAPI until yesterday when he emailed me, I guess they don't read the python developer survey 😉)

If you value pydantic or FastAPI or other libraries that use annotations at runtime, please (constructively and respectfully) let the python steering council know that you would like PEP 649 to be accepted. Please don't contact members of the python community, they're aware of this issue (see below) and are taking it seriously.

I understand the decision on PEP 649 will be made over the next few days, so if you're going to do anything do it today.

@samuelcolvin samuelcolvin pinned this issue Apr 15, 2021
@mathematicalmichael
Copy link

mathematicalmichael commented Apr 15, 2021

Could you please provide some suggestions for the best way to accomplish this ask?:

If you value pydantic or FastAPI or other libraries that use annotations at runtime, please (constructively and respectfully) let the python steering council know that you would like PEP 649 to be accepted.

Is there an email I can write to? Some forum to post on? What's the most impactful course of action?

@samuelcolvin
Copy link
Member Author

Not really, it's precisely because I don't know how to make my concerns clear that I created this issue.

I've posted to python-dev supporting the PEP, but I suspect that more people chiming in to just support what I've said might do more harm than good.

I've you've spoken to core devs in the past and they know you, perhaps prompting them might be the best thing to do.

@notatallshaw
Copy link

notatallshaw commented Apr 15, 2021

As a long time reader of the Python Dev mailing list I would say that it is not necessary to contribute to the mailing list beyond what @samuelcolvin has already posted, unless there are specific technical questions that you are highly qualified to answer.

The mail is already extremely forthright and I have no doubt it will generate a lot of discussion. One thing I would say though @samuelcolvin is it will probably help your case a lot to go in to more details about why typing.get_type_hints() doesn't work for pydantic, either because of specific issues, real world performance examples, or if there is some fundamental flaw in the approach itself.

There's a lot of smart people in the Python core Dev team, so it makes sense to state the problem not the solution. It may be that typing.get_type_hints() can be fixed or some other method could be implemented that would breach the gap.

My 2 cents anyway, hope it helps.

@samuelcolvin
Copy link
Member Author

I take your point, but the problems with typing.get_type_hints() and PEP 563 are well described in PEP 649 and debated at length in the other thread, I didn't want to bring all that up again.

@notatallshaw
Copy link

I'm going to be honest, I skimmed over many dozens of mails for this one.

But to me it seemed the discussions were talking about problems in the abstract, without a real motivation or need for them to be fixed. Whereas specifics on actual problems implementing it, with an actual library, that's used by lots of users, is a more much compelling position.

I would guess some core devs are going to ask questions about this though anyway, so responses to those questions will likely fill the gap.

Best of luck! Don't use this tool on a day to day basis, but it's been great when I have 😄

@gaborbernat
Copy link

I think you should explain better why it's impossible to implement lazy evaluation of type hints. The long list of issues doesn't help get a better understanding of the problem in achieving this, and in many it's never explained why it's hard to do.

@methane
Copy link

methane commented Apr 15, 2021

I take your point, but the problems with typing.get_type_hints() and PEP 563 are well described in PEP 649 and debated at length in the other thread, I didn't want to bring all that up again.

Even there is a long thread, I don't know what point is actually breaking pydantic.

@brettcannon
Copy link

Do note that as a steering council member it is more important to me to hear from you directly, @samuelcolvin , in PEP 649 itself on how its acceptance/rejection will impact Pydantic as a maintainer of a popular package using runtime annotations. But inundating us with a flood of emails or trying to get a high upvote count is actually a negative as it implicitly tries to put the SC in a position of "us versus them" for this decision (on top of having to sift through more emails with what limited time I have for SC topics).

I am personally already well aware of the popularity of Pydantic, FastAPI, and Typer, so there's no need to prove that to the SC.

@pablogsal
Copy link

pablogsal commented Apr 16, 2021

My recommendation here is to gather as much detailed, objective and technical information on how PEP 563 is impacting pydantic in a way it cannot adapt without compromising the core of the package. As a member of the steering council, I normally spend many hours parsing the mailing lists, https://discuss.python.org/ and other sources to gather a complete and holistic view of all the pros, cons and different views and how different parties should be impacted in order to make the best decision possible. The fact that this is bringing to our attention very close to beta freeze gives us very limited time to react in a way that keeps out standards, so it would be very very helpful to get this information directly from you without needing to be extracted from tons of messages.

Also, as a release manager of Python 3.10, it makes me sad that the first issues that are mentioned here:

#248 #234 #397 #415 #486 #531 #545 #635 #655 #704 #1298 #1332 #1345 #1370 #1668 #1736 #1873 #1895 #2112 #2314 #2323 #2411

go back to 2018 but we are hearing about all these problems and how they impact pydantic critically dangerously close to beta freeze. My priority as a release manager is to veil for the highest stability for the release. A considerable amount of features are merged days before feature freeze and this already has a considerable amount of chaos and work from the release management team and the buildbot team and the potential of reverting or including such big changes on top of all this with too little time close to feature freeze makes my job much harder as you can imagine.

In any case, for us, making sure that all our user base is taken into account is a very serious matter so you can be sure that we will take this into account when discussing the overall issue.

I do understand that everyone is very nervous and there is a lot of people worried and all of this is augmented with time limitations, but we are all volunteers that want the best for our users and the community so let's all be empathetic to each other, and try to work together towards the best solution.

Thanks for your patience and for your understanding.

P.S.

I guess they don't read the python developer survey 😉

We certainly do (at least from the Steering Council 😄 ).

@willingc
Copy link

Hi folks.

Let me state up front that I'm a very satisfied user of pydantic and FastAPI, and I'm very thankful for the work and contributions the maintainers and community around them have put in. ☀️

I also appreciate the volunteer efforts of the Python core team, Steering Council, and many contributors to Python.

I'm optimistic that we can find a win-win for pydantic / FastAPI and Python. I believe this is possible if we try not to polarize the solution prematurely to "all or nothing" or "accept or reject 649". To accomplish that we need to look at this through the lens of "what is possible", balance the tradeoffs, and work toward a "good but perhaps not ideal" solution.

Thank you and I'm looking forward to seeing us move forward together.

@tirkarthi
Copy link

There is an API proposal over adding typing.get_annotations(object) that could help and seems to have similar motivation : https://bugs.python.org/issue43817

Not sure if it helps but an issue were KeyError was raised for typing.get_type_hints was resolved : https://bugs.python.org/issue41515

@FelixTheC
Copy link

FelixTheC commented Apr 16, 2021

I will have the same issue in the future.
I wrote my own implementation which works in that way that all my tests are passing.

https://github.com/FelixTheC/strongtyping/blob/%2347_in_py10_globals_from_func_must_be_known/strongtyping/utils.py

Feel free to adapt and/or change it for your needs.

@samuelcolvin
Copy link
Member Author

First of all, I really appreciate what the python core developers in general, and the steering council in particular do for us all.

I only heard about the debate about PEP 649 on Wednesday night, and given the extremely narrow window to get my point heard, I banged the drum in every way I could think of. The lesson here is "careful what you wish for", my message has been amplified more than I expected.

I'm sorry if that has wasted anyone's time, even more so if it reduces the chance my request gets a positive reception.

You're also right that i should have engaged with python-dev long ago about this, I'm sorry I did not. Lesson (hopefully) learnt.

I never meant to generate an "us vs. them" atmosphere, and re-reading what I wrote, I don't think that's what I did. I think the most contentious think i said was that python's core might value "principle over pragmatism" - that would be a perfectly justifiable thing for you do in some scenarios, even though I disagree in this case.

A few points in my defence:

  • I had already (before Larry emailed me about PEP 649) requested to talk about this at the python developer summit in May, it was on my radar and I didn't realise how little time we had
  • The situation has changed in the last week as Guido himself points out "... the hypothetical future where annotations are not always syntactically expressions (which did not even exist before this week)..." - this change really would be a problem for pydantic and other such libraries
  • I specifically avoided telling people to email a particular address or contact particular people to avoid individuals being spammed, I also specifically suggested people didn't reply to the python-dev thread with "me too"s.

overall: Thank you for taking this seriously and I'm sorry to have caused a fuss.


I'm now going to spend some time doing my best to produce a coherent explanation of the short-comings of PEP 563 for pydantic to post both here and on python-dev (I assume just posting a link to here on the mailing list will annoy people?)

@cheesycod
Copy link

cheesycod commented Apr 16, 2021

Would it be possible to use eval() to bypass this in some way?

Like eval what you can and make the rest internally a ForwardRef or something

@PrettyWood
Copy link
Member

PrettyWood commented Apr 16, 2021

@cheesycod get_type_hints() uses eval() behind the scene with forward references. Unfortunately eval() is slow, not always available and may not have the necessary context to work. For example if you define a model or a type inside a function, there is no easy way to retrieve it and make it work.
I tried some time ago to test pydantic with 3.10.0a4 in local and had to play with the stack trace on top of get_type_hints() but couldn't make it work all the time. But I thought it was just a matter of time before get_type_hints() was improved or anything else was added to ensure this would still be doable with the new annotation system.
I didn't pay enough attention to the PEPs and since python 3.10 was still in alpha I didn't panic...

@henryiii
Copy link

henryiii commented Apr 16, 2021

How important is performance here? I hear "eval is slow", but when looking at that PEP, it looks like while PEP 649 is a little better than Python 3.9, Python 3.10/PEP 563 is much better, as it makes all annotation strings, pretty much leaving you with 0 cost for type annotations - which is affects all code, not just these ~three libraries. If pedantic is a few percent slower but all other code is half a percent faster, you might even win for pydantic libraries, and a big win for all other libraries. There might be some caching tricks to be used here (most of the PEP 563 improvements are for startup / memory usage)?

Also, being able to put invalid objects or usage into type annotations is huge(*); it's the main reason using Python 3.7 is so much better than 3.6 for typed code, since you can use Python 3.9 and 3.10 constructs in your type annotations, even though they had not been developed yet, you just need a recent MyPy or other type checker. To me it looks like PEP 649 nullifies some of that benefit, as accessing an annotation can crash if invalid objects are used, and invalid objects (which might be used eventually, like Guido's --flag example) might also open up new library ideas. Also, under "other uses" in the description, you have "documentation" - that usage is certainly not better with PEP 649 than PEP 563!

I'm not very familiar with pydantic, just a bit with typer; to me pydantic is a better dataclasses/attrs with serialization (I actually just heard about it a couple of days ago before this tweet/issue from a happy user). A MWE for something hard to do would be helpful. "Good" usages of runtime attributes still look like typing, and Pydantic falls into that, so seeing what is hard might really help. Is making a local type a good idea? Is it for a loop or something like that? This seems like it might be very specific to Pydantic, as I can't think of a good reason to make a local type for Typer.

(PS: I'm sort of assuming that PEP 649 requires the object to exist, since the point of this is to capture a reference to the local object. That actually may not be the case...)

@cheesycod
Copy link

It appears that there are ways to make eval faster if you are using that route using lambdas: https://stackoverflow.com/questions/12467570/python-way-to-speed-up-a-repeatedly-executed-eval-statement

Could also use a optional C module to speed things up as well in performance

@samuelcolvin
Copy link
Member Author

I'm now going to spend some time doing my best to produce a coherent explanation of the short-comings of PEP 563 for pydantic to post both here and on python-dev

Well luckily, I was saved the effort. Łukasz Langa (the instigator of PEP 563) has explained it very clearly in another thread on python-dev "PEP 563 in light of PEP 649".

There's another very interesting description of the subtle changes in scope associated with PEP 563, in PEP 649.

I won't go over everything here, but I'll just give one taster of challenges presented by PEP 563:

This works fine:

from __future__ import annotations
from pydantic import BaseModel, PositiveInt

class TestModel(BaseModel):
    foo: PositiveInt

debug(TestModel(foo=1))

But this fails:

from __future__ import annotations

def main():
    from pydantic import BaseModel, PositiveInt

    class TestModel(BaseModel):
        foo: PositiveInt

    debug(TestModel(foo=1))

main()

Why? Because PositiveInt is not defined in the module scope.

Even adding TestModel.update_forward_refs() doesn't help, you need to add TestModel.update_forward_refs(PositiveInt=PositiveInt) (update_forwards_refs() uses logic similar to get_type_hints(), though it doesn't call it).

Note: this isn't a limitation of pydantic or even a true "bug" in python, it is defined (albeit in passing) in PEP 563 "Annotations can only use names present in the module scope..."

This means if pydantic implemented some magic to get the second example to work, it would be contradicting PEP 563.

You might say "well imports like this should always be module level anyway", perhaps true, but what about custom data types which are extensively used with pydantic? There are many good reasons why you might want to use non-module level custom types, currently with python 3.10 that would require complex .update_forward_refs(...) calls.

@WhyNotHugo
Copy link

If you value pydantic or FastAPI or other libraries that use annotations at runtime, please (constructively and respectfully) let the python steering council know that you would like PEP 649 to be accepted.

Considering that PEP 563 is from around four years ago, and that the plan to change this on Python 3.10 has been set in stone for so long, it doesn't see right to unleash a mass of complaints to the core Python developers three weeks before the release happens.

The plan to implement and release these changes on Python were clear even before FastAPI was even written. It was clear from that moment that it was not going to be future-proof. Yet here we are, with a last minute campaign trying to push back on this.

@jayqi
Copy link

jayqi commented Apr 17, 2021

@samuelcolvin I think it would be helpful for you to update the root post for this issue thread to reflect the current state of things. For example, it seems like the Pydantic project has gotten sufficient attention and engagement from the Python Steering Council and that efforts are in motion to figure it out (that's my perception anyways), so the stuff in "You can help" is probably no longer needed. That way, nobody will inadvertently make unnecessary noise for the SC, and people who are worried about that noise won't keep commenting about it.

Strikethrough (~text~) might be a good way to indicate an update to that information but also preserving its history for transparency.

@samuelcolvin
Copy link
Member Author

Thanks @jayqi, done.

I'll update the issue again when we hear from the steering council.

@alexmojaki
Copy link
Contributor

Note: this isn't a limitation of pydantic or even a true "bug" in python, it is defined (albeit in passing) in PEP 563 "Annotations can only use names present in the module scope..."

This means if pydantic implemented some magic to get the second example to work, it would be contradicting PEP 563.

Is the reasoning behind this bit of the PEP explained somewhere in more detail than "postponed evaluation using local names is not reliable"? What would be wrong with pydantic getting frame.f_locals from where the class is defined (main in your example) and passing that to get_type_hints or update_forward_refs?

You might say "well imports like this should always be module level anyway", perhaps true, but what about custom data types which are extensively used with pydantic? There are many good reasons why you might want to use non-module level custom types, currently with python 3.10 that would require complex .update_forward_refs(...) calls.

I think using locally defined types (including imports) is perfectly fine and useful and allowing it is important. However I can't see how your custom data types link supports your argument, what are you referring to?

@samuelcolvin
Copy link
Member Author

Is the reasoning behind this bit of the PEP explained somewhere in more detail than "postponed evaluation using local names is not reliable"?

The best explanation I can find is here in PEP 649, if you want more detail you would probably need to dig into the PR or python-dev mailing from the time.

However I can't see how your custom data types link supports your argument, what are you referring to?

Because custom types (class definitions which are used as annotations) as well as nested models, might often be defined outside the module scope.

@notatallshaw
Copy link

Yup, I know about the delay of PEP-s. I was more thinking if there are plans for permanent long term solution.

I can't speak for authors of pydantic but in other projects I work on that could be affected by this we can not plan a long term solution until a decision on the PEPs and further the future of from __future__ import annotations is decided by the Python Steering Council. As it is unclear what the long term options available will be until then.

@gvanrossum
Copy link

It would be helpful if there was a brief summary of the problem that PEP 563 causes for pydantic (and perhaps other projects). I looked at the top of this thread but it is very long and emotional and did not give me a clear understanding of the problem. I clicked on one of the referenced issues (#248) but IIUC it actually describes a problem in pydantic that could be solved with the help of PEP 563 (or with PEP 649).

Note that I am not a user of pydantic; from the README it looks like a nice tool but the example there doesn't look like it would be affected adversely by PEP 563 (assuming pydantic calls get_type_hints() when it sees a string annotation).

Am I to understand that the real problem (i.e. something where working code would break due to PEP 563) is that it is common to define pydantic classes in a function's local scope that are then used in another scope, in a way that causes get_type_hints() to fail?

@samuelcolvin
Copy link
Member Author

I'll answer in more detail tomorrow, but yes, in summary the really hard problem is scope. It's not about whether the class is used right there or elsewhere, if the class is not defined at module level, things start breaking.

Larry went through this (and the fact that it was just a side note in the PEP) in great detail, in the many discussions that happened about this at the time. Maybe he would be worth asking?

@samuelcolvin
Copy link
Member Author

See #2678 (comment) above.

@methane
Copy link

methane commented Jan 8, 2022

I am light user of FastAPI and pydantic, but I never used non-module level types with it. You wrote:

You might say "well imports like this should always be module level anyway", perhaps true, but what about custom data types which are extensively used with pydantic? There are many good reasons why you might want to use non-module level custom types, currently with python 3.10 that would require complex .update_forward_refs(...) calls.

I read the custom data types. But there are no examples using non-module level types for good reason.
Would you explain some of "many good reason" for using non-module level custom types?


I am not interested in PEP 649 vs PEP 563 (enable by default), but PEP 649 vs current behavior (opt-in PEP 563). PEP 649 has still some behavior different from current behavior:

types = [int, str, float]

def make_constructors():
    cs = []
    for t in types:
        def c(t=t) -> t:
            return t()
        cs.append(c)
    return cs

cs = make_constructors()
for c in cs:
    print(c.__annotations__["return"])

# Current: int, str, float
# PEP 649: float, float, float

Is there no good reason for this?

@samuelcolvin
Copy link
Member Author

samuelcolvin commented Jan 8, 2022

A fuller answer to @gvanrossum's question:

There were 3 things that worried me back when I created this issue in April:

1. Performance

Time taken to construct pydantic models - effectively startup time.

typing.get_type_hints(Foo) is much slower than Foo.__annotations__, however that's only a small part of what pydantic does and from some unscientific experiments, it seems that creating models is only around 7-10% slower with from __future__ import annotations than without.

On the other hand, from __future__ import annotations/PEP-563/string-annotations provide a performance improvement in general, so there's two counteracting effects.

I've therefore also tested this in practice by running

[p.write_text(f'from __future__ import annotations\n{p.read_text()}') for p in Path('src').glob('**/*.py')]

on a ~10k fastapi/pydantic web application (a few files caused errors so I had to remove the __future__ declaration), overall this caused starting the app to slow down by 6% - roughly matching what I would expect from experiments on a single model.

Overall, while this isn't ideal, I can live with the performance impact. (If anyone else has seen a wildly different performance impact, feel free to bring that up)

2. Potential change of syntax

There were discussions back in April about a "hypothetical future where annotations are not always syntactically expressions".

Clearly if this became commonplace, it could be a problem for pydantic. However this seems unlikely, and even if it were the case, typing.get_type_hints() or some other function (inside or outside the stdlib) would need to be able to parse those expressions.

And anyway, blocking progress because it would open the door to something in future that could be a problem would be a mistake.

Overall, I think we can ignore this.

3. Scope

This is the only real concern I have with from __future__ import annotations/PEP-563.

Please note: this is not an issue with the concept of annotations being left as strings. It's a problem with the specifics of how PEP-563 defined annotation evaluations, from PEP 563:

Annotations can only use names present in the module scope as postponed evaluation using local names is not reliable (with the sole exception of class-level names resolved by typing.get_type_hints())

Because of that, the following breaks:

from __future__ import annotations
from pydantic import BaseModel

def main():
    from pydantic import PositiveInt

    class TestModel(BaseModel):
        foo: PositiveInt

    print(TestModel(foo=1))

main()

Of course, I've used from pydantic import PositiveInt here, but it could be any import or a custom type defined within the function, including another pydantic model. It could even be simple type alias like Method = Literal['GET', 'POST']

Pydantic might be able to do some clever stuff to recognise locally defined annotations, but that would:

  1. Require some magic that might be hard to maintain and unreliable
  2. Would mean pydantic was being intentionally un-pythonic by not following PEP 563

Would you explain some of "many good reason" for using non-module level custom types?

Well, all the same reasons people do local imports in any other code (import performance, circular references, name clashes) and the same reasons people define classes or variables locally (performance, name clashes, readability, accessing local variables).

[...] pydantic classes in a function's local scope that are then used in another scope

It's not about where the class is used, in the above example PEP 563 explicitly states that PositiveInt or anything else defined within the function cannot be used a a valid annotation. The code will fail when the class is defined, not when it's used.

Overall, while there could be workarounds, pydantic and other code that relies on runtime use of annotations would be uglier with PEP 563 style annotations scope than without


I hope that helps. I'll add a note to the top of this issue to help any future travellers who pass this way.

Overall, even if PEP 563 is accept as-is, I don't think pydantic is doomed (just a little damaged) - I was mistaken to suggest that originally. Sorry.

@gvanrossum
Copy link

Thanks for the clear explanation.

Let's focus on (3). (The example is missing from devtools import debug BTW, and in addition I get an error urging me to call TestModel.update_forward_refs() with the future import enabled. Adding that will indeed give the error about PositiveInt.)

I have a feeling you are reading a bit too much into the sentence you quote from the PEP. What the PEP meant was just that you have to pass the locals containing needed definitions into tying.get_type_hints() or inspect.get_annotations(), which is a bit of a pain when it is being called deep inside a library. Hence the PEP suggests to keep definitions global. The PEP doesn't imply that using local definitions is invalid or unpythonic!

Of the reasons for doing local imports:

  • performance: totally legit, if it's in a function/method that might never be called and a module that would otherwise not be imported (but I'm guessing importing anything from pydantic does not qualify)
  • circular references: I wouldn't solve those with local imports (PEP 563 is meant to help here actually)
  • name clashes: that sounds silly; why not use import ... as ...?

In general, defining classes locally is not a great pattern, except in certain types of unit tests.

With all that said, I agree that the situation is not great for pydantic. But there's no reason for despair either. :-)

Now, I can't look in the future to know what the SC will decide, but it looks like for now they are kicking the can down the road and 3.11 will end up being the same as 3.10 in this respect. So from __future__ import annotations is here to stay for another few releases at least, and people will want to use it for other reasons (as you say, it makes certain things faster).

I see that update_forward_refs() already takes a local namespace, e.g. TestModel.update_forward_refs(**locals()). Why not just recommend that people use that? You could even have a helper that uses sys._getframe(1) to access the parent frame and takes the locals from there.

@willingc
Copy link

@samuelcolvin @gvanrossum (including @tiangolo as well) I'm happy to see this discussion. Our team uses pydantic and fastapi in our product. Our engineers really enjoy using these libraries (they really rave about the productivity benefits), and there is a lot of positive impact that the libraries are having on Python adoption. If we do a Typing Summit at PyCon again, I encourage Samuel and Sebastian to join the discussion.

Samuel, thanks for the detailed response to Guido's questions and for being open to collaborating and investigating going forward. Let's keep the positive discussion going. Thanks ☀️

@henryiii
Copy link

The error message is here:

https://github.com/samuelcolvin/pydantic/blob/9d631a3429a66f30742c1a52c94ac18ec6ba848d/pydantic/fields.py#L832

It could have **locals() added to it.

And here's the update_forward_refs: https://github.com/samuelcolvin/pydantic/blob/87da9ac23f0917741c0536b474cf13cdbed50dfc/pydantic/main.py#L776 - this could have the sys._getframe(1) trick to try to pull the locals (I would assume this would be dependent on if sys contains _getframe, as it's not guaranteed to exist, from what I gather - so both changes together likely would be helpful)

The fact you can detect this and produce a nice suggestive error message is great to see, though! As far as I can tell, local class definitions is still an antipattern for Pydantic, but is just something that comes up occasionally due to the number of users? A minor, detectable inconvenience is not a bad tradeoff, perhaps?

Working example code, FWIW, since a few bits were missing:

# Install pydantic and devtools
from __future__ import annotations
from pydantic import BaseModel
from devtools import debug


def main():
    from pydantic import PositiveInt

    class TestModel(BaseModel):
        foo: PositiveInt

    TestModel.update_forward_refs(**locals()) # can remove this line with the patch below

    debug(TestModel(foo=1))


main()

PS: really dumb question - why can't update_forward_refs() be called in __new__ with this already? Specially, __try_update_forward_refs__ could include the locals. In fact, the following patch completely fixes your example for me - without the update_forward_refs call at all:

diff --git a/pydantic/main.py b/pydantic/main.py
index eea8abc..bc8afdf 100644
--- a/pydantic/main.py
+++ b/pydantic/main.py
@@ -1,4 +1,5 @@
 import warnings
+import sys
 from abc import ABCMeta
 from copy import deepcopy
 from enum import Enum
@@ -289,7 +290,9 @@ class ModelMetaclass(ABCMeta):
         cls = super().__new__(mcs, name, bases, new_namespace, **kwargs)
         # set __signature__ attr only for model class, but not for its instances
         cls.__signature__ = ClassAttribute('__signature__', generate_model_signature(cls.__init__, fields, config))
-        cls.__try_update_forward_refs__()
+
+        localns = sys._getframe(1).f_locals if hasattr(sys, "_getframe") else {}
+        cls.__try_update_forward_refs__(**localns)

         return cls

@@ -765,12 +768,12 @@ class BaseModel(Representation, metaclass=ModelMetaclass):
             return v

     @classmethod
-    def __try_update_forward_refs__(cls) -> None:
+    def __try_update_forward_refs__(cls, **localns: Any) -> None:
         """
         Same as update_forward_refs but will not raise exception
         when forward references are not defined.
         """
-        update_model_forward_refs(cls, cls.__fields__.values(), cls.__config__.json_encoders, {}, (NameError,))
+        update_model_forward_refs(cls, cls.__fields__.values(), cls.__config__.json_encoders, localns, (NameError,))

     @classmethod
     def update_forward_refs(cls, **localns: Any) -> None:

PS: I think __init_subclass__ might really simplify your two-stage metaclass code.

@samuelcolvin
Copy link
Member Author

Thanks for everyone's input.

@gvanrossum, good point about assuming debug() is already shoehorned into builtins, I've switched to print().

I noticed that there's a strong argument that annotations as strings doesn't kill pydantic:

It's hidden away in my comments about performance - I modified a medium sized application which makes extensive use of pydantic to insert from __future__ import annotations everywhere and .... it mostly just worked! (albeit, a bit slower to launch, and some issues with annotations-as-strings in FastAPI).

But still I think the FastAPI stuff could be fixed fairly easily. Overall, I think this is a fairly good indicator that in real life most pydantic usage won't be too adversely affected.

On circular imports, I need to think about this more but I'm not entirely convinced it won't be a problem - to avoid circular import problems while making my import global, I need to put the import behind if TYPE_CHECKING, if I do that, at runtime the import won't be available. Am I missing something?


@willingc I'll do my very best to get to pycon. If I can make it, I'll of course join a Typing Summit.

@samuelcolvin samuelcolvin changed the title IMPORTANT: PEP 563, PEP 649 and the future of pydantic PEP 563, PEP 649 and pydantic Jan 20, 2022
@gvanrossum
Copy link

On circular imports, I need to think about this more but I'm not entirely convinced it won't be a problem - to avoid circular import problems while making my import global, I need to put the import behind if TYPE_CHECKING, if I do that, at runtime the import won't be available. Am I missing something?

It depends on how the modules are structured. The problem doesn't occur when each module has a function that uses something from the other module. But it may occur when two classes in different modules reference each other. The solution normally (when using annotations for static typing) is to put the annotations in "forward reference" string quotes -- or use PEP 563. However if each class does runtime introspection at (roughly) the point of definition then you will indeed be in trouble.

@caniko
Copy link
Contributor

caniko commented Aug 31, 2022

However if each class does runtime introspection at (roughly) the point of definition then you will indeed be in trouble.

tortoise-orm uses TYPE_CHECKING when working with tortoise.Models that reside in different modules. This is not an issue unless we try to generate a pydantic model from either of the models. tortoise has a function for this, pydantic_model_creator.

Example, will raise exception:

if TYPE_CHECKING:
    from module.models import FromAnotherModule

class MyModel(tortoise.Model):
    other_model: fields.ForeignKeyRelation[
        "FromAnotherModule"
    ] = fields.ForeignKeyField("module.models")


# Ayyy, there goes PEP8
Pydantic_MyModel = pydantic_model_creator(MyModel)

The pydantic models are really useful when used with FastAPI, hence the reason I stumbled over this issue.

I am guessing the best is to implement a workaround for now, which is fine; that is what abstract models are for 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests