-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
fix: Support Pydantic v2 #1229
fix: Support Pydantic v2 #1229
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1229 +/- ##
==========================================
+ Coverage 96.75% 96.77% +0.01%
==========================================
Files 47 47
Lines 3944 3935 -9
==========================================
- Hits 3816 3808 -8
+ Misses 128 127 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM! 🎉 The code coverage reduction seems irrelevant to me.
Could you bump the pydantic version in the CI too? Looks like it’s pinned and/or cached too:
|
Please release this a a quick fix for both 7.x an 8.x unless #1220 is released. |
@henryiii, good catch. I'm personally not used to track lock files for libraries, even when they provide a CLI 😄 @sisp should I update the lock file in this PR or another? @lhupfeldt yes, don't worry, we'll surely merge and release this. We can't go faster than what our free time allows. Waiting for either: |
Thank you, I'm aware it is a volunteer effort and it is much appreciated. |
The fix is simple, so I think that's reasonable 🙂 |
@lhupfeldt Why not? You should be able to cap Pydantic as a temporary workaround on your end.
@pawamoy Slightly off-topic, but how do you ensure deterministic dev and test environments then? 🤔
@pawamoy I think in this PR to check that the fix actually works for Pydantic v2, although this means we'll be developing against Pydantic v2 then since that'll be the installed version; meaning we can't test against v1 anymore, but the same is true vice versa. I assume Pydantic won't keep supporting v1 anyway. To test against both v1 and v2 we'd need something like Tox which doesn't work well with lock files though. 😵💫 I think @henryiii has been thinking about dependencies, constraints, upper bounds, etc. a lot (fantastic article by the way 👏). Any advice from your side? |
I don't. Each dev has their own lock file. I let things break as new versions of dependencies come out. It sometimes break my own CI (for PRs on my projects), but I'm fine with that as it warns me early that something is broken. Locally, I can update my lock file to test on latest dependencies versions, without worrying about reverting the lock file before commits. Also, allowing each dev to have their own lock file integrates better with projects offering insiders versions (Material for MkDocs, my own projects 😉), as these versions can be fetched from local, self-hosted PyPI servers. With a tracked lock file, devs using insiders versions have to re-lock, test/check then revert each time. It's also easier for devs that do not have access to the same index as the one that was used to lock (users whose government blocks pypi.org for example, and who have to use mirrors).
Not sure about Poetry, but PDM can be used with both Nox and Tox. |
Yes, it would have been nice for this breakage to have been caught here, rather than in all the downstream projects that weren't using lock files. Same reasoning, I'd much rather get early warning of a breakage than have stable CI that doesn't tell me that my users are currently broken. :) (For a library, not a deployment of something, obviously) However, the caching is nice to have. I wonder if enabling dev versions would generally help? There were a lot of dev releases of Poetry. Not all packages do that, but locking + periodic automated updates + dev versions would be really a good way to go if packages did that often enough. On important projects, I usually test against minimum supported versions too - I have a test/constraints.txt file with all my specified minimums, and have a job or two dedicated to testing that.
Huge fan of PDM. It supports all PEP 621 backends, so you can use hatchling, flit-core, setuptools, one of the compiled ones, etc. - not just pdm-backend. And because you are using standards instead of custom Poetry config, Black and Ruff can pick up your minimum version of Python directly from your config, no need to set it for each tool! :) Hatch is nice too, and has multiple environment support, but doesn't support locking yet, which is the main reason I would use a tool like that. |
Yes, looks like someone has mentioned it at nix-community/poetry2nix#1183. Not familiar at all with nix. The GitHub dependency graph supports PEP 621 now. Not sure about dependabot, but it's easy to write a job that runs pdm update (though making CI run on that job requires an extra step). |
This is what Poetry's docs say about committing a lock file for a library: https://python-poetry.org/docs/basic-usage/#committing-your-poetrylock-file-to-version-control |
Feasible was probably not the right word. Just some pinning, committing, PR review, build, and then undoing it all which I would rather not have to do:) We are generally setup for CI. Pinning stuff that breaks should be easier than it is, but that is work in progress. |
👋 Have you guys seen rye ? it creates two lockfiles, one for running and another for dev'ing. From there, the path to nix I think would be pip2nix. |
PDM is also able to create/use multiple lockfiles with specific groups of dependencies 🙂 |
Now that #1220 has been merged, it is probably not necessary to backport this fix to 7.x. |
@pawamoy Could you update the locked Pydantic version to the latest v2, so that also CI can verify the correctness of the fix? poetry update pydantic |
Oh yes, thanks for the reminder! Done. |
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.
@pawamoy Could you please update your Poetry version to the most recent? The lock file nkw contains many entries category = "..."
again which got dropped in Poetry v1.5.
Oh, it seems there are more incompatibilities with Pydantic v2. |
Done. Let me know if I should rework the history to keep the lock file commits apart from the fix. Otherwise feel free to squash them all. |
Also I'll take a look at the other errors 🙂 |
Ha, too bad, bump-pydantic didn't update anything 🤔 |
Hi folks! Many things popped up here, sorry to be late to the party 😆 I see a bunch of off-topic discussions and I think they are interesting, but let me kindly suggest you to move them to the forum. Regarding the PR, I think the important part here is:
IMHO no. They broke compatibility. We should just update the dependency and forget Pydantic v1. |
@pawamoy Is it okay for you if I push commits to this PR? I have finished the Pydantic v2-only adaptations and would like to (a) retain your credit for your work on this and (b) avoid creating another PR. |
Of course, no need to ask :) |
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.
Pydantic v2 is working now, only the flake-check
jobs are failing with some error seemingly related to Nix that I don't understand yet.
@@ -16,7 +16,7 @@ _exclude: | |||
|
|||
_tasks: | |||
- "python [[ _copier_conf.src_path / 'tasks.py' ]] 1" | |||
- [python, "[[ _copier_conf.src_path / 'tasks.py' ]]", 2] | |||
- [python, "[[ _copier_conf.src_path / 'tasks.py' ]]", "2"] |
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.
Pydantic v2 doesn't parse an integer as a string anymore. But I think changing this tests isn't terrible because an integer was never officially supported according to the type hint of a task and the docs. Just FYI.
I suspect that |
Yes, that's it. I'll see if I can do that later, it's just a routine task. |
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.
The rest of the code looks great. I'll push later the Nix build fix so we can merge.
Closes #1225.