-
Couldn't load subscription status.
- Fork 404
Pydantic v2 #19071
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
base: develop
Are you sure you want to change the base?
Pydantic v2 #19071
Conversation
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.
Thank you very much for filing this - it saves the dev team a ton of effort ❤️
The sytest failures appear to be due to Sytest not following the spec - hooray for validation!
I've started on fixing them - by converting the tests to Complement and debugging them there, starting with: matrix-org/complement#811
Some minor comments below.
pyproject.toml
Outdated
| # We need packaging.verison.Version(...).major added in 20.0. | ||
| packaging = ">=20.0" | ||
| pydantic = "~=2.1" | ||
| pydantic = "~=2.12" |
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.
Do we require 2.12?
Several Linux distributions do not carry that version, so will struggle to package Synapse: https://repology.org/project/python%3Apydantic/versions
In general we should aim to specify the lowest possible version in pyproject.toml that's compatible with the code. Then in poetry.lock we specify highest possible (so those distributing Synapse using pyenv's can get the latest fixes and improvements).
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.
It’s dependent on the Python version. Pydantic 2.7.4 was the first version to give correct results. For Python 3.13 and Python 3.14 later versions are required, with Pydantic 2.12 being the first version to support Python 3.14.
We could discriminate based on Python version, something like pydantic>=2.7.4;python_version<'3.13', etc.
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 2.8 added support for Python 3.13. Because most of the distros in the table packaged at least Pydantic 2.8, I’d suggest the following:
pydantic = [
{ version = "~= 2.8", python = "< 3.14" },
{ version = "~= 2.12", python = ">= 3.14" },
]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.
I see, I didn't realise >=2.7.4 was necessary. Indeed, I don't see any distros in that list with 2.7.*, so it would make sense to go to the next minor version up, 2.8*.
Your suggestion looks sound!
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.
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.
Oh man, trial-olddeps is complaining. But I don’t get what’s going on just yet.
Edit: My first guess is a logic error in check_dependencies.py.
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.
You're correct. check_dependencies.py doesn't take the python attribute into account when determining which dependencies it needs to install. It then fails when attempting to install 2.12.x even though it's on Python 3.9.
I did some preliminary work on updating it in a separate branch. Let me bring that up into a PR.
Committing this to this branch to check whether it fixes `tests-olddeps`. If so, will extract this to a separate PR for review from the team.
This appears to be tripping old-deps. First, see if removing fixes it.
|
@V02460 I believe I've bypassed the previous old-deps errors and now we've moved on to something else (likely just changing a Heading to bed now, and can pick it up in the morning. |
synapse/rest/synapse/mas/devices.py
Outdated
| class PostBody(RequestBodyModel): | ||
| localpart: StrictStr | ||
| devices: set[StrictStr] | ||
| devices: set[str] |
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.
I believe I've bypassed the previous old-deps errors and now we've moved on to something else (likely just changing a set to a list in the defined type?).
Nice!
I found v2.9.0 to be the first version that no longer has this bug. Raising the lower version bound of Pydantic would fix the concrete error. Would that be ok?
Another approach would be to fix all the call sites to actually pass a set.
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.
Looks like there was only one call site to fix? After 13fe3fa the tests now pass!
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.
This PR is now blocked on #19110 merging, which contains the fixes to check_dependencies.py.
I'll revert the commits in this branch for now.
JSON has no way to translate to `set`, and the type is converted to a `set` below anyways.
A regression test for the issue we were seeing in #19071 (comment)
| pydantic = [ | ||
| { version = "~=2.8", python = "<3.14" }, | ||
| { version = "~=2.12", python = ">=3.14" }, | ||
| ] |
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.
@V02460 could you also update (probably remove) the comment above pydantic under the [tool.poetry.group.dev.dependencies] header?
synapse/rest/synapse/mas/devices.py
Outdated
| class PostBody(RequestBodyModel): | ||
| localpart: StrictStr | ||
| devices: set[StrictStr] | ||
| devices: set[str] |
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.
This PR is now blocked on #19110 merging, which contains the fixes to check_dependencies.py.
I'll revert the commits in this branch for now.
This reverts commit 5a4eb77.
Update Pydantic to v2.
Steps I took to create this change:
check_pydantic_models.pyNotable changes are
_metadata_urland_introspection_endpointAnyEventIdtupleandsettypesCloses #15858
Closes #15979
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.