search

fractal-analytics-platform / fractal-tasks-core

Main tasks for the Fractal analytics platform
https://fractal-analytics-platform.github.io/fractal-tasks-core/
BSD 3-Clause "New" or "Revised" License
14 stars 6 forks source link

Pin pydantic to `<=2.8.2` #834

Closed jluethi closed 2 months ago

jluethi commented 2 months ago

In our arg schema creation, we rely quite heavily on pydantic._internal imports. Some of those appear to break in pydantic v2.9.0. I can't currently trace down which PR broke it, but for example this part was removed in Pydantic 2.9.0:

from pydantic._internal import _typing_extra
_typing_extra.add_module_globals()

add_module_globals does not appear to exist anymore in pydantic v2.9.0.

We either pin to pydantic 2.8.2 (for the moment) & update that in the templates. Or we come up with a fix to adapt to this pydantic change.

Big picture, the less we need to rely on _internal the better. As long as we rely on it somewhat heavily: Do we want to pin pydantic for the time being to a max version? Requiring 2.9.0 may also be somewhat aggressive, given that it was just released on September 5th.

jluethi commented 2 months ago

See e.g. changes in https://github.com/pydantic/pydantic/blob/v2.8.2/pydantic/_internal/_typing_extra.py from 2.8.2 to 2.9.0

tcompa commented 2 months ago

The bottom line here is that part of our use cases (extracting a model from a call signature) is not supported by pydantic, and we made it work by mimicking what they do within validate_call (see https://github.com/pydantic/pydantic/blob/cdca7aebba33770416c260335e914c1755c48f13/pydantic/_internal/_validate_call.py).

We can likely reduce/remove some _internal imports, but it'd still be a trade-off because we'd have to copy part of their code or logic into our dev tools. I still think it's a good direction, at least when this concerns very small logic (e.g. the typing-extra one, that mostly boils down to a single statement). I'll open a "maintenance" issue about that.

On the other hand, I think that in this case the context is key: we don't really want/need to encourage task developers to rely on cutting-edge pydantic features (which is different from depending on e.g. scipy/numpy/sklearn/skimage/anndata/dask/cellpose cutting-edge features - see e.g. #821 or #833). Thus I'm strongly in favor of introducing a (very mild) usage limitation, by constraining pydantic to the latest version that works with our current main (<=2.8.2), and reducing our dependency on pydantic._internal with a lower priority.

jluethi commented 2 months ago

Agree that supporting the most cutting edge pydantic features isn't a September priority for sure! Fully fine with pinning this for the time being.

Let's discuss the best way forward to ensure our manifest building is maintainable and that we eventually can support newer pydantic versions longer-term.

tcompa commented 2 months ago

I merged #836 and released fractal-tasks-core v1.3.1