CodSpeed Performance Report

Merging #14582 will not alter performance

_{Comparing johnslavik:fix-regr-prepare-response-content (83d55e4) with master (1d93d53)¹}

Summary

✅ 40 untouched

Hmm, I think we need to check for both classes. This proposes not dumping v1, which doesn't seem inline with the recent upgrade.

For safe typing, it would be better to use unions of v1.BaseModel and BaseModel (v2) in all proper functions.
The type checker could then detect unsafe ops on two jointly. I'll shoot for # type: ignore from the old version for now.

Looks like the coverage job deadlocked. Closing and reopening to re-run CI.

LMK if we want some tests here.

Hello @tiangolo - question - do you have some initial assessment of the issue (even if not yet deciding to merge the PR) - 0.127.0 has also started to fail in Airflow and while it seems the root cause is some interaction between FastAPI and Cadwyn - we should soon take some direction with Airflow - we might also add an issue to Cadwyn if you think it should be solved there - just our reproducers clearly show that currently this case is broken

I think if it only fails with Cadwyn, you should first open issue there.
Or, try to come up with the MRE without third-party dependencies

The MRE in #14575 shows that it only happens with Cadwyn and @johnslavik even proposed a PR fixing it - explaining why it happens (because of Cadwyn copying classes). I will definitely open issue with Cadwyn, but the investigation and fix suggests that it might happen with other tools as well, so my question is more on whether it is intended change or more of something that can (continue as it was before 0.126.0) be handled by Fastapi.

Note, that it's - as usual In those cases - hard to pinpoint who is 'guilty' - I..e. Whether the fact that it worked before was accidental, and whether the change was intentional, so likely if I open an issue in Cadwyn, they will also expect some assessment from FastApI point of view.

So I guess just looking at it and assessing by the maintainers might shorten the ping-pong

Thanks all for the feedback!

Rationale and current state

So, in Pydantic v2, with the default behavior in FastAPI (without Cadwyn), this code path would never be reached (if I'm understanding my own code correctly 😅 ), because a Pydantic v2 model would always have a ModelField with a serialize() method.

Before that PR it was passing everything through this serialization to account for installations of Pydantic v1.

But that also means that data is (slowly) double serialized in Python using this code. In Pydantic v2, the serialize method is used, and that uses the TypeAdapter.dump_python method, which serializes the data using the internal Pydantic core schema on the Rust side. So, things are much faster.

The state before this change was sub-optimal, for the sake of backwards compatibility, but now that I'm removing support for Pydantic v1, it doesn't make sense to do double serialization.

Yesterday I made another release raising a warning when users use Pydantic v1. This week I plan on fully dropping support for Pydantic v1 and raise an error if people use it.

That will mean a major internal cleanup, removing a lot of workarounds to support both v1 and v2 at the same time. This will improve performance, correctness, remove a whole range of existing/potential bugs happening from all that custom logic, and will enable a bunch of new features.

One of the important new features, which is partially now available, is precisely serializing on the Pydantic side when possible.

There are these refactors and some other internal improvements I need to make, some of the future ones will include router refactors that will allow other tools to add their own logic without needing to resort to touching FastAPI internals. But first I have to finish these things.

This specific issue

Now, given all the above, soon I will remove most of this code, once I drop support for Pydantic v1, probably this week or the next.

I could still make a quick patch version now with this PR, for this specific use case, ideally with a test that fails with master but passes with this PR, to ensure it solves the problem. But it would be something temporary, just to have a release that works and you can use for now.

But I imagine some other things will change as well this week or the next once I fully drop support for Pydantic v1. 🤔

ideally with a test that fails with master but passes with this PR, to ensure it solves the problem

Noted. Thank you very much for the quick response. I'll add the test shortly and ping you back.

Also I think if that's the case, then indeed this is something to be fixed on Cadwyn side. I do not think @tiangolo - you should even relese a new version for us - we we can stay with 0.125.0 until Cadwyn solves it.

The issue is already open zmievsa/cadwyn#314

Thanks @potiuk! It might then even make sense for Cadwyn to wait a few days (this or next week) until I fully remove Pydantic v1, as that will be a larger change, but will probably simplify things. 🤔

Also makes me think that maybe I should "hurry" to fully remove it instead of waiting more, as that will accelerate/simplify any fixes needed in other projects, instead of having a fix for this version and then having to change more in a few days... 😅

Thanks @potiuk! It might then even make sense for Cadwyn to wait a few days (this or next week) until I fully remove Pydantic v1, as that will be a larger change, but will probably simplify things. 🤔

Also makes me think that maybe I should "hurry" to fully remove it instead of waiting more, as that will accelerate/simplify any fixes needed in other projects, instead of having a fix for this version and then having to change more in a few days... 😅

Quite agree :)

This is BTW. precisely wanted to wait before I talk to Cadwyn, because it was not clear whether it was a delliberate or accidental change, and what direction it will take - and now we can have a very clear message - which I just did: zmievsa/cadwyn#314 (comment)

And limiting airflow for now so that you can release new fastapi versions as you wish apache/airflow#59726

Thank you for the help and coordination @potiuk! 🙌

The least we can do as a way to support Fast APi

To wrap this up:

I do not think @tiangolo - you should even relese a new version for us - we we can stay with 0.125.0 until Cadwyn solves it.

I understand that there's no need to go forward with this PR.
I'll see if I can help fixing this in Cadwyn.

If you'd like me to reopen, please let me know.

Thank y'all as well!

Thanks for your help here as well @johnslavik :)