Skip to content
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 Migration Plan #133

Open
alysivji opened this issue Jul 3, 2023 · 17 comments
Open

Pydantic 2.0 Migration Plan #133

alysivji opened this issue Jul 3, 2023 · 17 comments

Comments

@alysivji
Copy link

alysivji commented Jul 3, 2023

  • fhir.resources version: fhir-resources==7.0.1
  • Python version: Python 3.11.3
  • Operating System: `macOS Monterey 12.5.11

Description

Pydantic 2.0 was released on Friday. New version has a lot of underlying changes + some API changes -- see migration guide.

Is there a plan for fhir.resources to support the new version of Pydantic? Please let me know if you need a hand migrating. I use this library all the time and would love to contribute

What I Did

Created a fresh virtual environment with the following requirements file:

pydantic>=2
fhir.resources

Tried to run the Quickstart example in the README and got the following error:

from pydantic.class_validators import ROOT_VALIDATOR_CONFIG_KEY, root_validator
ImportError: cannot import name 'ROOT_VALIDATOR_CONFIG_KEY' from 'pydantic.class_validators'
@nazrulworld
Copy link
Owner

nazrulworld commented Jul 3, 2023

I think that would be good idea to move with newer Pedantic 2.x. Also I think that it would be good idea to wait for another new release of Pydantic for proven stabilities as well other popular library is starting to support.
For example https://github.com/tiangolo/fastapi/blob/master/pyproject.toml#L45 I can see that the FastAPI restricts Pydantic <2.X, what is important to follow as many projects use this library along side with FastAPI.

@gilmorera
Copy link

@nazrulworld about 4 days after your last comment here FastAPI moved to pydantic 2.0 haha. The minor versions are restricted, but they have migrated.
fastapi/fastapi@0976185

@bwalsh
Copy link

bwalsh commented Nov 2, 2023

Checking in to see if I can help on this issue. Is anyone working on it. If not, will open a PR.

@nazrulworld
Copy link
Owner

@bwalsh thanks a lot, you are welcome to open any PR.

@bwalsh
Copy link

bwalsh commented Nov 14, 2023

Ouch. This will be a lot of work 🤯

Currently stuck on AbstractBaseType see here. Wondering if there is a better way without relying on internals

    """ """

    __fhir_release__: str = "R5"
    __resource_type__: str = ...  # type: ignore

    # @classmethod
    # # TODO[pydantic]: We couldn't refactor `__modify_schema__`, please create the `__get_pydantic_json_schema__` manually.
    # # Check https://docs.pydantic.dev/latest/migration/#defining-custom-types for more information.
    # def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None:
    #     field_schema.update(type=cls.__resource_type__)

    @classmethod
    def __get_pydantic_json_schema__(
        cls, _core_schema: core_schema.CoreSchema, handler: GetJsonSchemaHandler
    ) -> JsonSchemaValue:
        # return schema for generated class
        json_schema = {}  # TODO HOW-TO-IMPLEMENT        
        return json_schema

    # @classmethod
    # # TODO[pydantic]: We couldn't refactor `__get_validators__`, please create the `__get_pydantic_core_schema__` manually.
    # # Check https://docs.pydantic.dev/latest/migration/#defining-custom-types for more information.
    # def __get_validators__(cls) -> "CallableGenerator":
    #     yield cls.validate
    @classmethod
    def __get_pydantic_core_schema__(
        cls, source_type: Any, handler: GetCoreSchemaHandler
    ) -> CoreSchema:
        return return core_schema.general_plain_validator_function(cls.validate)

    @classmethod
    def validate(cls, v, values, config, field):
     ..... 

    ```

@bwalsh
Copy link

bwalsh commented Nov 15, 2023

@nazrulworld

It will be a while before there is a PR on this one.

1: I don't think a brute force 'liftover' will work.
2: Currently, I'm thinking to manually craft an R5B resource, perhaps Specimen with 'stock' validation and json schema and see what the delta is. Hopefully/maybe that will minimize or eliminate low level pydantic code in the base classes.
3: If the above works, then we can use it as a basis for code generation.

Let me know your thoughts

@Tshimanga
Copy link
Contributor

Just an fyi. I had run into dependency resolution issues using poetry when trying to comingle fhir.resources with packages like gqlalchemy that have already migrated to pydantic v2.

Pydantic V2 currently provides access to the v1 api via the pydantic.v1 submodule so as an intermediate step toward v2 adoption, I've opened a PR updating the pydantic depedency of fhir.resources to >=2.0.0 and replaced all pydantic imports with pydantic.v1 here #147

Tests and linting are passing but mypy (the typechecker) is complaining about a transitive dependency on numpy that it's unhappy about

$ mypy fhir/resources/
/home/travis/virtualenv/python3.10.13/lib/python3.10/site-packages/numpy/__init__.pyi:650: error: Positional-only parameters are only supported in Python 3.8 and greater
Found 1 error in 1 file (errors prevented further checking)
The command "mypy fhir/resources/" exited with 2.

I'll look into resolving that last issue (suggestions welcome!) as I'm able and hopefully this will help keep things moving forward :)

cc @bwalsh @gilmorera @alysivji

@Tshimanga
Copy link
Contributor

Merged! #147

Now we actually need to migrate from pydantic.v1 to pydantic 😅

@hstoebel
Copy link

Thank you! Just curious when we might see this released? Not trying to pressure, just want to set a reminder to check back.

@bwalsh
Copy link

bwalsh commented Dec 12, 2023

@Tshimanga Thanks very much for taking the initiative.

@nazrulworld
Copy link
Owner

@jstoebel we have an issue #144, may be good to have a new release after fixing this.

@nazrulworld
Copy link
Owner

Here we go! https://pypi.org/project/fhir.resources/7.1.0/ @jstoebel

@bwalsh
Copy link

bwalsh commented Dec 14, 2023

Nice work everyone!

@gilmorera
Copy link

So this isn't an actual migration though right? It's still using the v1 schema?

@gilmorera
Copy link

If so you may want to keep this issue open.

@Tshimanga
Copy link
Contributor

So this isn't an actual migration though right? It's still using the v1 schema?
If so you may want to keep this issue open.
@gilmorera

Correct, I didn't actually migrate off the v1 api. I just updated the pydantic dependency to the v2.x and adjusted the imports accordingly.

There will still need to be work done to migrate to the v2 api, but In the meantime this will appease dependency resolution for package managers like Poetry when trying to co-mingle fhir.resources with projects like fast api that have already upgraded.

I agree that this issue is still unresolved.

I would be interested in picking @bwalsh and @nazrulworld 's brains on the current best thinking around the actual api migration. Updating import statements didn't require a terribly deep understanding on how everything currently hooks together.

@nazrulworld nazrulworld reopened this Dec 17, 2023
@Tshimanga
Copy link
Contributor

@nazrulworld @bwalsh

I've started a branch and draft pr for collaborating on the pydantic v2 migration here #148

I think that we'll probably have a lot of commit by commit discussion, and it may take a while; but i wanted to get some forward movement going :)

Anyone else with 2 cents to give please chime in as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants