-
Notifications
You must be signed in to change notification settings - Fork 23
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
Pydantic >=2.0 support #215
Pydantic >=2.0 support #215
Conversation
I also alphabetized the dependencies
…an incorrect typehint.
My nodejs installation was not working, therefore I had to turn off `ruff` and `prettier`
Does conda or mamba not pick up the
I think it's from the
You can ask pre-commit.ci to run the checks for you and push the changes back. See the bottom of https://results.pre-commit.ci/run/github/235846200/1689788417.B7uBcS6vRRy61WJYv_tI8Q
That doesn't need to change. We're generating the version # from git tags. When you run
We may want to add to also add to our test matrix to test against both Pydantic 2.0 and lower to make sure we don't break anything in older versions. Which I can take care of, or you can give it a shot if you want. |
Yes conda/mamba will fail with the "-r" syntax if you do it directly as in Thanks for the tip on pre-commit CI. I will use it! I'll be trying to get the tests to work now |
pre-commit.ci autofix |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
I made a PR with a tweaked test matrix that'll test both Pydantic 1 & 2, so we make sure we don't inadvertently break anything with the upgrades. If you want you can pull it into your PR #216 |
Okay all tests and pre-commit checks are working..yay! Only issue is pydantics 2.0 upgrade has caused an issue with the Sphinx I made a Issue over there. I would just try to fix it, but their test suite seems to be a mess rn and I'd hate to break something without realizing it. If no one responds in a few days I'll give it a go. I'm converting to a real PR. Feel free to merge in your expanded test matrix? Or maybe just wait until after this is merged. |
I'll take a fuller look through in a bit, but we could pin Pydantic to <2 in https://github.com/xpublish-community/xpublish/blob/main/docs/requirements.txt for the docs. |
Good idea, forgot the docs have their own dependencies. Also don't merge quire yet. I'm going to fix some of the warning related to deprecated pydantic methods we are using. Should prevent headaches in the future. Edit: Done |
So I pinned the docs to the old pydantic and it still fails. This is a tad odd since the I tried Pydantics experimental "bump-pydantic" package, which made one small change (905688c). I also manually addressed the only deprecation warning coming from the xpublish codebase directly (9a502bd). |
Ah, looking at the RTD logs, it's the order things are getting installed in. Since xpublish is being installed last, pip ends up upgrading Pydantic then. Lines 21 to 25 in b425bc1
We specify our install here. I think we may be able to tweak the package install step to install the docs requirements afterwards, or add another step after it ( python:
install:
- method: pip
path: .
- requirements: docs/requirements.txt |
All is fixed 💯 |
I think 9a502bd broke Pydantic 1.x compatibility. We'll probably have to live with that deprecation until the rest of the ecosystem is Pydantic 2 compatible. I found that by testing locally with nox, but you could also grab my PR that changes the Github Actions test matrix to see. # noxfile.py
import nox
python_versions = [
"3.9",
#"3.10",
#"3.11"
]
@nox.session(python=python_versions)
@nox.parametrize("pydantic", ["<2", ">=2"])
def tests(session: nox.Session, pydantic: str):
"""Run py.test"""
session.install("-r", "dev-requirements.txt")
session.install(".")
session.install(f"pydantic{pydantic}")
session.run("pytest", "--verbose") |
Sounds good, the old methods (BaseModel.dict()) still works but will be gone in Pydantic 3.0...we definitely have plenty of time. So no big issue. So I guess options are:
TO_DICT_METHODS = {
'1': 'dict',
'2': 'dump_model',
}
...
getattr(self, TO_DICT_METHODS[pydantic.__version__[0]])() I'll leave the call up to you? |
How about an exception? try:
self.dump_model()
except AttributeError
self.dict() |
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.
Most of it looks pretty good. There are a lot of changes that I guess are probably due to Black or Ruff updates, but thats fine.
The one thing that I'm unsure about is the changes to all of the response types. In most cases we'll want to specify the Pythonic type so that FastAPI can document things appropriately. Maybe we can set the default response class for the Zarr APIRouter for the way that Zarr expects JSON.
xpublish/plugins/included/zarr.py
Outdated
def get_zarr_metadata( | ||
dataset=Depends(deps.dataset), | ||
cache=Depends(deps.cache), | ||
): | ||
) -> JSONResponse: |
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 think these all should probably be dicts as response types, unless we want to make Pydantic models to represent Zarr (and maybe encapsulate encoding)?
Or there is an existing Pydantic Zarr package we could build on...but not something for this PR.
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 idea of encapsulating dict response types if definitely interesting and I agree outside the scope of this PR (also apologies for scope-creep...I got carried away!).
In general I am big into TypedDicts as they don't hard enforce but you can know what to expect somewhat and help contributors. That said, the Pydanic Zarr package seems very interesting. It seems you need to use their pydantic_zarr.GroupSpec.from_zarr()
which may introduce a performance hit and appears to need to be done for each Zarr specifically. In that way it may not be ideal. But I just took a quick glance.
Maybe encapsulation could be a topic of a meeting at some point in the future? My next priority on xpublish
will be the catalog plugin. I'm going to open an issue where we can start a discussion.
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! Thank you!
Let me know when you are ready to land it.
Just waiting on the tests/docs to run for sanity but I'm ready to go! |
That loss in Codecov is fine for now, as the except is only hit for Pydantic v1. I've tested it against v1 locally and it works. I can land my PR after this that will test against both v1 and v2 to help us maintain compatibility in the future, and maybe set up Codecov to merge coverage runs. |
Overview
BaseModel
's fields to have the overridden value type annotated. I added these small type annotations for thename
field, and also added information to the docs.-r requirements.txt
syntax indev-requirements.txt
such that one can install via conda, mamba, or pip. All dependencies are in alphabetical order.