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

Allow pydantic v2 #152

Closed
wants to merge 32 commits into from
Closed

Allow pydantic v2 #152

wants to merge 32 commits into from

Conversation

yurnih
Copy link
Collaborator

@yurnih yurnih commented Sep 29, 2023

This PR introduces essential code changes required for upgrading to Pydantic v2 while maintaining compatibility with Pydantic v1. Since Pydantic v1 is used with D-Geolib-plus, it still needs to have backwards compatibility to that version.

Pydantic v2 contains a lot of optimalisations which improves processing speed of specifically result files (tested with D-settlement and D-Stability). However, version 1 had a “lax” way of handling typing of properties where in v2 this has changed to strict. This will result in a lot of failures in tests. For now I used the specific v1 module in pydantic v2, so it will not break tests.
When running tests in v2, I’ll receive the following (Tests with consoles won’t work locally for me since I don’t have the right licenses anymore):
=== 120 failed, 1382 passed, 2 skipped, 2 xfailed, 95000 warnings, 2 errors in 120.73s (0:02:00) ===

I posted this PR as draft, to start maybe a discussion how to proceed further and if it is wished to proceed further. Personally I think it is needed to split this PR since it is too big, some can be grouped as improved type hinting or adding default values (is required for Optional values in future versions), others are for migration or compatibility code itself.

I've added the pytest summary for pydantic_v1.txt / pydantic_v2.txt, both are run in pydantic==2.4.2. For v1 all errors are only related to the executables, for v2 some failures are also in unittests.

@sonarcloud
Copy link

sonarcloud bot commented Sep 29, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 53 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
7.6% 7.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@carloslubbers
Copy link
Contributor

Hey yurnih, thanks for the effort in writing up this draft. I'm also interested in updating GEOLib to pydantic v2 at some point, but also encountered a massive amount of breaking changes and changes in behavior which kind of made me put it off as well. I'm not too experienced with pydantic either.

I think it would be good to support just pydantic v2 in the future, we can probably update GEOLib-Plus to also use pydantic v2. Supporting both sounds like a hassle

@evetion
Copy link
Member

evetion commented Nov 20, 2023

For the sake of the ecosystem, it might be good to migrate to pydantic v2, but use the v1 API still available in the new version: https://docs.pydantic.dev/2.0/migration/#continue-using-pydantic-v1-features first.

I'm planning on doing that in Deltares/HYDROLIB-core#588, after migrating completely in Deltares/Ribasim#731.

@evetion
Copy link
Member

evetion commented Nov 21, 2023

Ok, so it seems my intended upgrade path (update Pydantic and using the pydantic.v1 API doesn't work with FastAPI fastapi/fastapi#10360) doesn't work. It seems this PR approach (maybe without the backwards compatibility) is the way forward.

@yurnih
Copy link
Collaborator Author

yurnih commented Nov 21, 2023

It was exactly the issue I discovered too.

pytestmark = pytest.mark.skipif(
pydanticv1_loaded, reason="FastApi uses pydantic v2 when it is available."
)

Afterall it is a quite complex migration path, to use the API-service and Geolib+, FastApi & Pydantic v1 is required to make it work. But moving forward, and using Pydantic v2 from the v1 module, the Api service won't work anymore. Also, the v1-module namespace in Pydantic v1 won't exist either.

In my progress towards adopting Pydantic v2, I've made specific code updates related to this transition. However, moving forward with Pydantic v2 also necessitates an upgrade for Geolib+. This pull request supports Pydantic v2 from the v1 module while maintaining compatibility with Pydantic v1. Yet, it comes with the requirement of a polyfill until both Geolib+ and the Geolib parts associated with parsing D-Series text format are completely migrated to ensure Pydantic v2 compatibility.

The only other way to make ik work with Pydantic v2 with using the v1-module is to patch a few functions in FastApi _compat file.

@wfaustmann
Copy link
Collaborator

Support for pydantic V2 has been added in #179

@wfaustmann wfaustmann closed this Jun 5, 2024
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

Successfully merging this pull request may close these issues.

4 participants