fhir.resources icon indicating copy to clipboard operation
fhir.resources copied to clipboard

Pydantic V2 API Migration

Open Tshimanga opened this issue 1 year ago • 13 comments

It looks like pydantic is quite thoroughly interleaved in fhir.resources. I haven't yet identified clear boundaries or individually migrate-able portions of the codebase. I think that we may have to collaborate commit by commit discussing how to navigate each breaking change.

Hopefully we can find a way to better encapsulate some of the fancy pydantic usage as we go.

I don't mind taking point on making sure this work/pr keeps moving forward, but I expect I'll need frequent input from @nazrulworld along the way.

Tshimanga avatar Dec 17 '23 22:12 Tshimanga

I think you are on the right track here.

  • The short term goal is centered around migrating fhirabstractmodel to a pydantic2 form.
    • Ideally the intricacies around v1 ROOT_VALIDATOR_CONFIG_KEY, ROOT_KEY etc. refactored and simplified
  • Once a model for the pydantic2 version of fhirabtractmodel exists, I assume code generation needs to change? If so, perhaps pydantic adjacent tools like https://github.com/koxudaxi/datamodel-code-generator https://docs.pydantic.dev/latest/integrations/datamodel_code_generator/ could help?

bwalsh avatar Dec 18 '23 19:12 bwalsh

@nazrulworld @bwalsh continuing to work on fhirabstractmodel.py in particular with regards to the parse_raw / parse_file functionality provided in fhir.resources

in the Pydantic V2 migration guide

Some of the built-in data-loading functionality has been slated for removal. In particular, parse_raw and parse_file are now deprecated. In Pydantic V2, model_validate_json works like parse_raw. Otherwise, you should load the data and then pass it to model_validate

I think that the decision they made is that while a domain modeling library can provide convenience utilities for data IO, that is overreach into logic that ought to be fully owned by the user (or some other library). I think I agree with this and this thinking could be applied fhir.resources as well.

What do you think of removing the IO logic in fhir.resources? Obviously, there's a question of api stability/deprecation; but I think it makes sense.

An alternative would be copying the pydantic v1 IO utilities into fhir.resources (there is no replacement for them in v2) but even then I'd recommend decoupling it from FHIRAbstractModel - instead keeping it as it's own functional utility that can be composed with any FHIR resource's .model_validate_json by end users as needed.

Also, happy holidays :)

Tshimanga avatar Dec 25 '23 18:12 Tshimanga

I am fully agree with you about separation of IO functionalities. But for now we should keep those functionalities inside FHIRAbstractModel.

nazrulworld avatar Dec 26 '23 21:12 nazrulworld

Hey @Tshimanga @bwalsh @nazrulworld, just wanted to check in on this. What's the current status? We need to rework codegen and then it should work? Or are there still changes needed to the base models here?

I'm trying to use this in a FastAPI app built on pydantic V2, but I currently can't use the models in the framework due to V1 usage.

With a little direction on what's remaining I might be able to help with this effort.

nickderobertis avatar Apr 18 '24 14:04 nickderobertis

@nickderobertis

With a little direction on what's remaining I might be able to help with this effort. It has been a while since I looked at it. This is where I was stuck. https://github.com/nazrulworld/fhir.resources/issues/133#issuecomment-1810713446

bwalsh avatar Apr 18 '24 23:04 bwalsh

Hi @nazrulworld @bwalsh @Tshimanga

I saw in the thread you already managed to fix the pydantic version compatibility for the package managers and alike. However, as far as I can see the real migration to the pydantic V2 API would happen in this PR. Am I right that this is the current / latest draft to migrate to pydantic V2?

I would like to see this in action and am willing to give some cents of reviewing or contribute myself, yet right now this PR seems a bit stale.

Kind regards

skaanbilly2 avatar May 11 '24 15:05 skaanbilly2

@skaanbilly2 @nickderobertis Thanks for offering to help. This is pr is indeed stale at the moment and I'll need to re-familiarize with where I left off.

Please feel free to contribute to getting this over of the finish line as I'm still a bit preoccupied.

At the moment the moment the main effort has been migrating the fhirabstractmodel class off of the v1 api. I believe there were still some open questions (at least for me) with respect to how to migrate the parts of the v1 api that have no documentation or documented counterpart in the v2 api.

Tshimanga avatar May 11 '24 17:05 Tshimanga

@Tshimanga @skaanbilly2 I could manage some time on this development at coming sommer (around mid July or earlier).

We can have a meeting about collective contributions, if you guys have time.

nazrulworld avatar May 12 '24 10:05 nazrulworld

We can have a meeting about collective contributions, if you guys have time. Great idea. Propose some dates/times.

bwalsh avatar May 12 '24 14:05 bwalsh

Hi @Tshimanga Do you have any update in related to this migration? Should I create a new branch and then merge your PR in that branch or what approach should we take?

nazrulworld avatar Jun 23 '24 21:06 nazrulworld

@nazrulworld I definitely don't want to be a blocker for this. I'll be preoccupied for the next couple of weeks and then I'll be available to pick this up again. I'm happy to either share this branch or create a new one. If you've got spare time for this now then lets do whatever is most convenient for you

Tshimanga avatar Jun 24 '24 15:06 Tshimanga

We can have a meeting about collective contributions, if you guys have time.

@nazrulworld also definitely still interested in meeting to talk through the rest of the work :)

Tshimanga avatar Jun 24 '24 15:06 Tshimanga

@Tshimanga thanks a lot for the clarification. I think, I will try with my approach and observe the progress. After then we can combine both our ideas.

nazrulworld avatar Jun 24 '24 18:06 nazrulworld

Hi @Tshimanga @bwalsh and all I have started the workaround of the possibilities for Pydantic v2 migration during my summer vacation. It seems that we need to use completely different approached than current implementation. However I am close enough to make it working! if you follow the experimental branch https://github.com/nazrulworld/fhir.resources/tree/pydantic-v2-experimental [see fhir/resources/tests]

What I did

  1. I have created a core FHIR library based on Pydantic v2 https://github.com/nazrulworld/fhir-core || https://pypi.org/project/fhir-core/
  2. Used that library into fhir.resources and refactored (regenerated) all the fhir resources (R5).

What is current status

  1. I can run tests ``fhir/resources/tests` and most of tests can pass, yet to fix more tests.
  2. In the fhir-core library, yet to add more features and bug fixes.

Your contributions are welcome

  1. I suggest you could focus on https://github.com/nazrulworld/fhir-core as still we need to add feature, optimise codes base, for example model serialization, field type handling, etc.
  2. You are welcome to test the experimental branch of fhir.resoures

nazrulworld avatar Jul 27 '24 23:07 nazrulworld

Here is the first beta release https://pypi.org/project/fhir.resources/8.0.0b1/

nazrulworld avatar Aug 05 '24 20:08 nazrulworld