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

Introduce pydantic.v1 submodule use to facilitate pydantic v2 upgrade #147

Closed Tshimanga closed 8 months ago

Tshimanga commented 8 months ago

It will be a larger undertaking to fully migrate this codebase to the bonafide pydantic v2 api; however, pydantic v2 provides access to the v1 api via the pydantic.v1 submodule. While migrating imports to pydantic.v1 doesn't constitute a true migration. It will unblock fhir.resources users when trying to use fhir.resources with other packages like gqlalchemy and fastapi that have already made the move.

Tshimanga commented 8 months ago

@nazrulworld Tests and linting are passing but oddly typechecking seems to fail. Below from CI:

$ 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.

It looks like it's complaining about numpy's api (perhaps a transitive dependency), not any of the lines that I've changed. Got any suggestions?

nazrulworld commented 8 months ago

@nazrulworld Tests and linting are passing but oddly typechecking seems to fail. Below from CI:

$ 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.

It looks like it's complaining about numpy's api (perhaps a transitive dependency), not any of the lines that I've changed. Got any suggestions?

First of all thanks for your hard work. We can work on this matter later, as I don't have much time now! I suggest you keep try looking on it, until I am back.

Tshimanga commented 8 months ago

We can work on this matter later, as I don't have much time now!

No worries, totally understand. I'll poke around as per my availability as well.

First of all thanks for your hard work.

Glad to help! Thanks for putting in the groundwork building a python fhir library!

Tshimanga commented 8 months ago

@nazrulworld so after looking into this a bit I've found the following:

For https://app.travis-ci.com/github/nazrulworld/fhir.resources/jobs/614806374,

ERROR: Could not find a version that satisfies the requirement pydantic[email]>=2.0.0 (from fhir-resources[test]) (from versions: 0.0.1, 0.0.2, 0.0.3, 0.0.4, 0.0.5, 0.0.6, 0.0.7, 0.0.8, 0.1, 0.2, 0.2.1, 0.3, 0.4, 0.5, 0.6, 0.6.1, 0.6.2, 0.6.3, 0.6.4, 0.7, 0.7.1, 0.8, 0.9, 0.9.1, 0.10, 0.11, 0.11.1, 0.11.2, 0.12, 0.12.1, 0.13, 0.13.1, 0.14, 0.15, 0.16, 0.16.1, 0.17, 0.18, 0.18.1, 0.18.2, 0.19, 0.20a1, 0.20, 0.20.1, 0.21, 0.22, 0.23, 0.24, 0.25, 0.26, 0.27a1, 0.27, 0.28, 0.29, 0.30, 0.30.1, 0.31, 0.31.1, 0.32, 0.32.1, 0.32.2, 1.0b1, 1.0b2, 1.0, 1.1, 1.1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.6, 1.6.1, 1.6.2, 1.7, 1.7.1, 1.7.2, 1.7.3, 1.7.4, 1.8, 1.8.1, 1.8.2, 1.9.0a1, 1.9.0a2, 1.9.0, 1.9.1, 1.9.2)
ERROR: No matching distribution found for pydantic[email]>=2.0.0
The command "pip install -e .[test]" failed and exited with 1 during .

I would suggest dropping support for 3.6 (and honestly really anything older than 3.9, but I don't have any experience as an open source maintainer so I'll defer to you on that).

For the CI jobs running python 3.7 - 3.9

$ mypy fhir/resources/
/home/travis/virtualenv/python3.7.13/lib/python3.7/site-packages/pydantic/__init__.py:45: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.rtfd.io/en/latest/common_issues.html#using-a-development-mypy-build
If this issue continues with mypy master, please report a bug at https://github.com/python/mypy/issues
version: 0.812
/home/travis/virtualenv/python3.7.13/lib/python3.7/site-packages/pydantic/__init__.py:45: : note: please use --show-traceback to print a traceback when reporting a bug
The command "mypy fhir/resources/" exited with 2.

I would suggest dropping python 3.7 and 3.8 support (again I'll defer to you on that) and updating the mypy dependency to something more modern as the error message suggests.

For https://app.travis-ci.com/github/nazrulworld/fhir.resources/jobs/614806378,

$ 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.

This issue is a known mypy bug affecting versions <0.981 as documented here https://github.com/python/mypy/issues/13627. Updating the mypy dependency to at least 0.981 should resolve that issue.

For https://app.travis-ci.com/github/nazrulworld/fhir.resources/jobs/614806379,

$ mypy fhir/resources/
fhir/resources/core/utils/yaml.py:6: error: Library stubs not installed for "yaml"  [import-untyped]
fhir/resources/core/utils/yaml.py:7: error: Library stubs not installed for "yaml.representer"  [import-untyped]
fhir/resources/core/utils/yaml.py:7: note: Hint: "python3 -m pip install types-PyYAML"
fhir/resources/DSTU2/fhirabstractmodel.py:36: error: Library stubs not installed for "simplejson"  [import-untyped]
fhir/resources/DSTU2/fhirabstractmodel.py:36: note: Hint: "python3 -m pip install types-simplejson"
fhir/resources/DSTU2/fhirabstractmodel.py:36: error: Name "json_loads" already defined on line 32  [no-redef]
fhir/resources/DSTU2/fhirabstractmodel.py:231: error: Argument 1 of "dict" is incompatible with supertype "BaseModel"; supertype defines the argument type as "AbstractSet[int | str] | Mapping[int | str, Any] | None"  [override]
fhir/resources/DSTU2/fhirabstractmodel.py:231: note: This violates the Liskov substitution principle
.
.
.
fhir/resources/DSTU2/tests/fixtures.py:24: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
fhir/resources/R4B/tests/fixtures.py:24: error: Library stubs not installed for "requests"  [import-untyped]
Found 66 errors in 13 files (checked 1155 source files)
The command "mypy fhir/resources/" exited with 1.

There seem to be actual complaints from mypy that need to be resolved; though some seem unrelated to fhir.resources and instead problems with external dependencies that mypy could be configured to ignore. Since, 3.11 is currently marked as "allowed to fail" and mypy presents the only failures I would suggest handling the resolution of those issues as a separate follow up PR dedicated to getting 3.11 support from "allowed to fail" to a more robust state.

Aside: Might also be worth separating testing, linting, and typechecking into separate CI workflows so that testing, linting, can be required for 3.11 while typechecking is allowed to fail.

In the meantime I'll testing out unpinning/playing with mypy versions and see what happens.

Tshimanga commented 8 months ago

@nazrulworld

Ok. After unpinning mypy the odd errors are gone, but I'll have to chip away at the typechecking issues little by little.

Tshimanga commented 8 months ago

@nazrulworld it looks like there are a lot of files that don't have type annotations at all. Would you be open to making the typechecking "allowed to fail" in CI and I can add better type coverage in a subsequent PR?

Tshimanga commented 8 months ago

@nazrulworld ok, all CI passing except for the python 3.6 job https://app.travis-ci.com/github/nazrulworld/fhir.resources/jobs/614828171

Pydantic v2 is not available for python 3.6, which makes sense given its age. I suggest removing support for it.

nazrulworld commented 8 months ago

@nazrulworld ok, all CI passing except for the python 3.6 job https://app.travis-ci.com/github/nazrulworld/fhir.resources/jobs/614828171

Pydantic v2 is not available for python 3.6, which makes sense given its age. I suggested removing support for it.

I fully agree with you about dropping python 3.6 support. Again thanks a lot for your hard work. Can you please add yourself here https://github.com/Tshimanga/fhir.resources/blob/feature/support-pydantic-v2-via-v1-compat-module/AUTHORS.rst while I am looking on merging.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1fd6ea4) 99.97% compared to head (61faec9) 99.97%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #147 +/- ## ======================================= Coverage 99.97% 99.97% ======================================= Files 302 302 Lines 60584 60584 ======================================= Hits 60566 60566 Misses 18 18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.