nazrulworld / fhir.resources

FHIR Resources https://www.hl7.org/fhir/resourcelist.html
https://pypi.org/project/fhir.resources/
Other
365 stars 104 forks source link

Pydantic 2.0 Migration Plan #133

Open alysivji opened 1 year ago

alysivji commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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. https://github.com/tiangolo/fastapi/commit/0976185af96ab2ee39c949c0456be616b01f8669

bwalsh commented 9 months ago

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

nazrulworld commented 9 months ago

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

bwalsh commented 9 months ago

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 commented 9 months ago

@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 commented 8 months ago

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 https://github.com/nazrulworld/fhir.resources/pull/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 commented 8 months ago

Merged! https://github.com/nazrulworld/fhir.resources/pull/147

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

jstoebel commented 8 months ago

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

bwalsh commented 8 months ago

@Tshimanga Thanks very much for taking the initiative.

nazrulworld commented 8 months ago

@jstoebel we have an issue https://github.com/nazrulworld/fhir.resources/issues/144, may be good to have a new release after fixing this.

nazrulworld commented 8 months ago

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

bwalsh commented 8 months ago

Nice work everyone!

gilmorera commented 8 months ago

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

gilmorera commented 8 months ago

If so you may want to keep this issue open.

Tshimanga commented 8 months ago

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.

Tshimanga commented 8 months ago

@nazrulworld @bwalsh

I've started a branch and draft pr for collaborating on the pydantic v2 migration here https://github.com/nazrulworld/fhir.resources/pull/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!