Closed yurnih closed 5 months ago
SonarCloud Quality Gate failed.
53 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells
No Coverage information
7.6% Duplication
Catch issues before they fail your Quality Gate with our IDE extension SonarLint
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
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 https://github.com/Deltares/HYDROLIB-core/issues/588, after migrating completely in https://github.com/Deltares/Ribasim/pull/731.
Ok, so it seems my intended upgrade path (update Pydantic and using the pydantic.v1 API doesn't work with FastAPI https://github.com/tiangolo/fastapi/issues/10360) doesn't work. It seems this PR approach (maybe without the backwards compatibility) is the way forward.
It was exactly the issue I discovered too.
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.
Support for pydantic V2 has been added in https://github.com/Deltares/GEOLib/pull/179
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.