-
Notifications
You must be signed in to change notification settings - Fork 113
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
bump fastapi and add pydantic compatibility #1529
bump fastapi and add pydantic compatibility #1529
Conversation
Signed-off-by: fdroessler <[email protected]>
Signed-off-by: fdroessler <[email protected]>
Signed-off-by: fdroessler <[email protected]>
Thank you so much for this. |
I think is meant to resolve #1532 |
Is 0.100 the minimal version that works for v1 and v2? If so I'd be very cautious, it is only out 2 months ago. Bumping the upper bound should be fine. I think what we want to do here is
I have tested this locally and it works, so this is something that we could already do now (it's only half of what this PR/issue request tho). In most cases, the performance gain for kedro-viz are not meaningful. If you need to run kedro-viz, it's better to have a separate environment to run this application, I think this is acceptable as often application has it own Docker, you don't need the same dependency to run Kedro and visualise them. (It would be nice, but I think kedro/kedro-viz users base is much larger than Kedro/Fastapi users, it's not worth to break it).
Ideally if we can support both v1 & v2, it would be perfect. Otherwise I will stick with V1, this way at least most of the fastapi (supporting both v1,v2 atm) users can still use kedro-viz at the same time. |
In this pydantic page are some tips how you can have pydantic v1 and v2 at the same time.
Is this something that we could consider? For our team we are newly considering kedro, but not having pydantic v2 is a breaking factor. I could try to do it. But I guess it would make sense to make a new pull request then. |
I extracted some data from BigQuery (https://packaging.python.org/en/latest/guides/analyzing-pypi-package-downloads/) and I have to say that users don't seem scared of new FastAPI versions, quite the opposite: Almost 50 % of fastapi installs are Bumping the minimum version from |
viz doesn't use pydantic directly (via FastApI BaseModel), and there isn't any issue to upgrade fastapi version. The only problem here is fastapi >0.100 default to pydantic v2, currently it only works with v1 (fastapi support both v1 and v2) cc @astrojuanlu |
Actually, the only change here I see is that Otherwise, the worst case is that we need to define class conditionally base on version of pydantic. |
AFAIU, fastapi 0.100 supports pydantic v1 and v2 simultaneously, so Kedro Viz would continue working with pydantic v1 and v2 |
@astrojuanlu If you look at the PR, V2 require changes in the The problem here is user installed v2 pydantic (as they need to use it in their app), but viz need v1. |
Oh gotcha 👍🏽 Thanks for the clarification |
Yeah but if you rewrite it using the above. So it will have the same syntax for both v1 and v2. Then it can work with both right?
And then if the user has pydantic v2 installed the top part will be executed, and if the user has v1 installed the bottom part will be executed. It does mean that kedro itself is not using the improvements of pydantic v2, but don't know if that improvement is needed for something like kedro viz. However, it does allow for support for pydantic v2 and v1. |
@IngerMathilde I am not sure if I am following, I think you will need to update both the import and the class definitions? Any chance you can open a PR to demonstrate this? |
Also happy to update this PR according to the input to see if we can make it compatible with both. From the top of my head I think having both configs won't cause an issue and just raise a deprecation warning in the logs. Let me try it later unless there is already another PR |
Signed-off-by: fdroessler <[email protected]>
Signed-off-by: fdroessler <[email protected]>
@noklam @IngerMathilde This seems to me like a potentially good compromise. It would still require some projects to update fastapi to introduce pydantic v2 compatibility but allow users to use both pydantic v1 or v2. Based on my experience the creator of fastapi does a phenomenal job at backwards compatibility such that I rarely have to do anything when bumping fastapi versions. Also there are some weird mypy issues when defining the test dict as a variable and passing it to Will fix the test coverage if this is an agreeable solution :) |
@fdroessler , @IngerMathilde |
Hi created a draft pull request to show what I had in mind. Not too familiar with pull requests, so was not really able to test it out. But maybe you can use it as an idea in the sprint review. #1539 There is quite some overlap with this pr so we can also continue here. |
Hi @fdroessler , Thanks for the PR. We had an internal discussion with the team and decided to bump fast api version but keep using pydantic v1 for kedro viz, taking into account that pydantic v2 is pretty new. Can you please update the PR to reflect the fastapi upgrade - In requirements.txt - In responses.py updating only the import but keep using pydantic v1 - try:
# Triggered if pydantic v2 is installed
from pydantic.v1 import BaseModel
except ImportError:
# Triggered if pydantic v1 is installed
from pydantic import BaseModel Thank you |
sure will do this tomorrow |
Hi @ravi-kumar-pilla, If you say that getting the warning is preferred as opposed to have new code that is potentially changing in the future (although I think pydantic usually does a reasonable job in not changing the API once they picked it) in the code base I can accept that and will revert the changes. Just thought I bring up the question before making the changes. |
@fdroessler thank you for bringing this up. I agree on your thoughts of having support for both versions. But at this moment, for Kedro Viz, we do not see any significant improvements using pydantic v2. But, we want to address the issue of users upgrading Fast API and who want to use pydantic v2 in their apps. (Though Kedro Viz will not be using pydantic v2 itself).
As of now, I think getting a warning should be fine. what do you think @rashidakanchwala @noklam ? |
I agree with you that it will work for both cases. I would say that from viz perspective it adds overhead to support 2 different version of Both pydantic and fastapi are moving fast, so I believe this discussion will soon be obsoleted that we can safely migrate to pydantic v2. Personally I think this is largely driven by fastapi, if they decide to drop support of v1 people will be moving to v2 very quickly. This PR will still be a good reference that we can re-use when we decide to upgrade pydantic. This PR is trying to address two independent problems, and we don't necessary block the bumping of fastapi with this pydantic disucssion. Support pydantic v1 and v2 user however is important because it's preventing people from installing it, so we would like to get this merged sooner than later. Hope this make sense! 🙏🏼 |
Signed-off-by: Florian Roessler <[email protected]>
Signed-off-by: Florian Roessler <[email protected]>
Signed-off-by: Florian Roessler <[email protected]>
Signed-off-by: Florian Roessler <[email protected]>
Signed-off-by: Florian Roessler <[email protected]>
Ok, understood the points. I reverted the changes. Getting mypy and the |
@noklam @ravi-kumar-pilla struggling a bit here with the error. If I use |
Let me have a look. Thank you for your time here. |
@fdroessler , I can confirm there is some issue with using pydantic v2. I get the below error - (Let me know if you are facing a different error) TypeError: validate() takes 2 positional arguments but 3 were given
in validate_python return self.validator.validate_python(__object, strict=strict, from_attributes=from_attributes, context=context) Let me investigate a bit on this. I see the pydantic docs site to be down. I will work on this and let you know if there is any progress. Thank you |
@ravi-kumar-pilla Yep getting the same error |
Unable to figure this out. @noklam did you encountered this before ? Can you please take a look if you get some time. Thank you |
Another option would be to go back to the initial proposal which has the downside of having to maintain two versions of the code. |
@fdroessler this looks ideal at the moment. But I believe the issues are due to the change in the validate method. I see TypeAdapter in action which was not present in pydantic v1. Since our request models are dataclasses and not BaseModel, supporting fastapi >= 0.100.0 and pydantic v1 seems complex. It would be great if we can momentarily pause this update until the request models are converted to pydantic BaseModels ( #1538 ) and then decide on this. Thank you for your patience. |
@fdroessler - We've spend a considerable time to explore the feasibility of supporting both Pydantic v1 and v2 simultaneously. Maintaining dual code bases is not ideal for us. Our initial plan was to let users install Pydantic v2.0 while continuing to use v1.0 via I am closing this request for now. Thanks for your contribution on this. |
Description
Bump version of fastapi and add compatibility with pydantic v2 using the bump-pydantic script.
This PR might be a partial duplicate of #1483 however, it adds minor code changes that are inline with the migration to pydantic v2 that comes with bumping fastapi to the >0.100.0 version.
I guess the main question here is as this would bump the lower version to 0.100.0 which is quite a jump if that is wanted. The alternative is that so far fastapi 0.100.0 is backwards compatible to pydantic v1 so as long as this is there one could omit the code changes to the code (BaseModel and examples) and pin pydantic to <2.0 to keep the lower bounds on the fastapi requirements until one wants to move to v2. Not sure what is preferred, I'd probably go for v2 but I know there is a lot of code there that does not comply with pydantic v2 so this could introduce some issues for downstream projects. Although I guess it's often straightforward to migrate 🤷
QA notes
Ran local tests.
Checklist
RELEASE.md
file