-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Import modules/objects directly from v1 namespace #9162
Import modules/objects directly from v1 namespace #9162
Conversation
This PR is ready for reviewing, please let me know if there are any other steps I can help out with here! Please review :) |
@PrettyWood is there anything else I need to do on my end on this PR? This is my first PR into pydantic so apologies if I've missed an automation somewhere! |
Great, thanks so much for your work! Apologies for the review delay - was focusing on FastUI for a bit. You've done everything perfectly here! |
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.
Great, thanks! We can release this with 1.10.16!
Working on getting the pipeline all green, will merge when that's done! |
Hmph, looks like tests are failing in some cases:
|
Thanks @sydney-runkle for looking through it! hmm weird, running locally all my tests pass, though perhaps I should exclude the mypy module from being imported directly in the import test, given that I get the feeling that the |
oh actually nevermind the
seems to do the trick. |
Hmph, looks like we're still getting some test failures. I'd be ok with a more minimal approach here - not testing Thanks! |
I'm not entirely sure there is a nice fix for the linux compiled tests to be honest - not one that I can think of at least :( (that doesn't involve converting all the relative imports in pydantic to absolute) - given it only raises an ImportWarning - and that this |
3bc7aa0
to
7a43881
Compare
Also, I think we can revert the commit that added the absolute imports 👍 |
not too sure what changes need to be made to |
Try using the |
I believe thats what I've done here (the diff comes from the fact this PR points to pydantic:1.10.X-fixes not |
Ah yes sorry, let's pull whatever is on the |
Ahhh ok I forgot that this was the PR where I made the CI changes - I'll play around with it later today and try to fix things! Could you please revert the relative import changes? |
I would but I'm afraid that doing so might cause the linux-compiled tests to fail 😅 whereas swapping the relative --> absolute imports causes all the tests to pass (apart from the Windows ones) |
Some context for my CI change, btw: https://github.com/pydantic/pydantic/pull/9305/files
Hmm I see what you mean... I'll take a closer look. |
Also just as a side note - we can get this PR into v1.10.X-fixes, but this will probably be the last pydantic 1.10.X release, as we put a June 1st 2024 end date on maintenance 🚀. |
I pinged @hramezani re these test failures, let's see what he thinks :). |
Hmm, so I got most of the tests passing. Let's revert those import changes and see if we can get the linux build tests passing :). |
To your point, with the relative imports and the cibuildwheel pin: https://github.com/pydantic/pydantic/actions/runs/9351505506 |
Yeah - looks like it might need the absolute imports - are you happy for me to revert the revert and use absolute imports? |
@exs-dwoodward I'd like to get confirmation / explanation from @davidhewitt or @hramezani first, but I'll send an update soon! |
I don't have an immediate answer for what Cython might be doing to the code to break things when compiled; is it possible that the new |
I believe that is exactly the issue - which appears to be 'fixed' by using absolute rather than relative imports. If all the tests pass with absolute rather than relative imports - is there a particular reason other than caution to stick with relative? |
I guess my worry is that if the relative imports broke things in a way we don't understand and the absolute imports will fixes them, is that just because stuff is still broken but we don't happen to be triggering the unhappy case? What happens if we modify |
Thats my only worry as I do not know exactly what cases pydantics CI tests cover - I can give that a go, see if it helps! |
I'm not too certain it will make a difference - since the modules / subpackages will still be importable, and it looks like their |
Well the new error message seems interesting 👀 In PyO3 I think we've worked around similar errors by adding to Is this a good idea? I have no idea 😂 |
Yeah so it looks like this hasn't really helped and is creating a lot of pain. Is the conclusion that the absolute imports was the only working option then? Maybe we just go with that and hope for the best... |
Okay nice - looks like the CI is all passing with the absolute imports in place now! 🚀 |
Great work! We'll include this in the next v1.10.X patch release, then be done with V1 changes :). Thanks for all of your help (and patience) here! Ping me anytime if you're looking for issues to contribute to, we really appreciate your work! |
Change Summary
Following on from this PR about adding a
v1.py
namespace to pydantic to better follow the v2 syntax of importing v1 objects, imports such as:or:
cannot be directly swapped for:
or:
imports. If this was the case, this would make unpinning from v1 --> v2 much simpler as a simple find and replace would be all that is needed to convert
from pydantic
-->from pydantic.v1
instead (ofc still using v1 types and models).One caveat with this approach is if this happens:
but ideally the import syntax should be changed across a codebase rather than only in some places. Note that anything in the
__init__.py
pydantic namespace is still imported wholesale:since these modules are imported directly from the underlying submodules. This also means that for the former import block, the
ModelField
in this case is imported twice.Additionally, things like
Pylance
can't directly resolve the import path forpydantic.v1.fields
imports using this syntax, but ideally that will be very transient since as soon as the find and replace is executed, anypydantic<2
pins should be able to be removed wholesale and the import will be resolved in pydantic v2.Checklist
Selected reviewer: @sydney-runkle
skip change file check
Fix #9357
Selected Reviewer: @PrettyWood