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

Reduce dependency on `pydantic._internal` #835

Open tcompa opened 2 months ago

tcompa commented 2 months ago

See https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/834.

We currently have these imports

from pydantic._internal import _generate_schema
from pydantic._internal import _typing_extra
from pydantic._internal._config import ConfigWrapper

I think we can at least get rid of the specific _typing_extra.add_module_globals function that changed when passing from 2.8.2 to 2.9.0 (https://github.com/pydantic/pydantic/commit/8dc0ce3c626d49d94ce688ffc450abf2b491aff3), and maybe we can also reduce dependency on ConfigWrapper. Whether we can also avoid depending on _generate_schema.GenerateSchema is TBD.

tcompa commented 2 months ago

Ref:

jluethi commented 1 month ago

Raising this again: It will be useful if we don't need to have too strict opinions on the pydantic version eventually. Currently, using pixi to set up environments (which some of our users are starting to do & then reporting issues to me) can lead to issues, because pixi is very strict in the dependency resolving and other libraries start to depend on e.g. pydantic 2.9.2.

In that specific case, I'm having an issue making napari-ome-zarr-navigator (which depends on fractal-tasks-core for some NGFF library functionality) work in a pixi environment with napari-ome-zarr, because they apparently pin pydantic==2.9.2.

I then get:

pixi add napari-ome-zarr
  × failed to solve the pypi requirements of 'default' 'osx-arm64'
  ├─▶ failed to resolve pypi dependencies
  ╰─▶ Because fractal-tasks-core==1.3.2 depends on pydantic>2,<=2.8.2 and pydantic==2.9.2, we can conclude that fractal-tasks-
      core==1.3.2 cannot be used.

(I am not sure why pixi finds a disagreement within just fractal-tasks-core 1.3.2. It's fine installing just that, but runs into conflicts when I add napari or napari-ome-zarr. My suspicion is that because we don't pin napari, that exploration of finding compatible napari versions & their dependencies leads to issues. I apparently can't put fractal-tasks-core & napari >=0.5.0 into the same pixi project)

jluethi commented 1 month ago

See resulting dependency hell here: https://github.com/fractal-napari-plugins-collection/napari-ome-zarr-navigator/issues/29

But there will be other solutions to solve this than changing things in fractal-tasks-core => The conclusion is not that we need a quick fix, but that we'll need to eventually relax this to avoid similar issues

tcompa commented 3 days ago

For the record, there's hope for some activity on the pydantic side, re: schema generation for optional attributes:

This isn't going to fit into the scope of v2.10, but is definitely a high priority, and we're hoping this can be one of our banner features for v2.11. (https://github.com/pydantic/pydantic/issues/8394#issuecomment-2397163834)

(see also https://github.com/pydantic/pydantic/issues/7161)


If/when this lands upstream, we can review whether we can find a supported way of producing the same JSON Schemas as we do now. This option would be clearly better than implementing custom solutions (which we are already doing, but which would become even more custom if we need to support different incompatible pydantic versions as <2.9 and >=2.9).

Meanwhile, I'll explore/propose a different solution to mitigate or fix the dependency issue for napari-ome-zarr-navigator - see upcoming issue.

jluethi commented 3 days ago

Great, thanks a lot @tcompa and then let's just keep an eye on the upstream developments here before we do more work on the schema & pydantic version!

tcompa commented 2 days ago

Meanwhile, I'll explore/propose a different solution to mitigate or fix the dependency issue for napari-ome-zarr-navigator - see upcoming issue.

Part of the current issue is made a bit less urgent by https://github.com/fractal-napari-plugins-collection/napari-ome-zarr-navigator/issues/29, but I'll still mention what I had in mind.

  1. We have a loosely-defined future plan of extracting manifest-building tools into a separate package, which would then be an optional dependency for a tasks package (only used when actually building the manifest locally).
  2. A potential complexity of that plan appears if someone is using very fancy&recent pydantic features in their input models. In that case it would not be trivial to mix two pydantic versions (the recent one to use at runtime and e.g. the 2.8.2. one to use for manifest building). The previous point suggest that we should still aim at removing the pydantic-version constraint, even if this adds a bit of customization.
  3. If point 2 is not an issue, then we can already decouple the two dependencies by adding an extra, such that pip install fractal-tasks-core comes with no pydantic upper bounds but e.g. pip install fractal-tasks-core[xxx] comes with a pydantic<=2.8.2 bound. This can be done with optional dependencies in pyproject.toml. Here is a first example (coming from a quick test, and still TBD):

    diff --git a/pyproject.toml b/pyproject.toml
    index fb083afa..8474615f 100755
    --- a/pyproject.toml
    +++ b/pyproject.toml
    @@ -30,7 +30,11 @@ zarr = ">=2.13.6,<3"
    numpy = "<=2.1.0"
    pandas = ">=1.2.0"
    lxml = "^4.9.1"
    -pydantic = ">2,<=2.8.2"
    +
    +pydantic = [
    +    { version = ">2", optional = false },
    +    { version = ">2, <2.9", markers = "extra == 'xxx'", optional = true}
    +]
    docstring-parser = "^0.15"
    anndata = ">=0.8.0,<0.11.0"
    filelock = "3.13.*"
    @@ -50,6 +54,7 @@ image_registration = { version = ">=0.2.9", optional = true }
    
    [tool.poetry.extras]
    fractal-tasks = ["Pillow", "imageio-ffmpeg", "scikit-image", "llvmlite", "napari-segment-blobs-and-things-with-membranes", "napari-workflows", "stackview", "napari-skimage-regionprops", "napari-tools-menu", "cellpose", "torch", "image_registration"]
    +xxx = ["pydantic"]
    
    [tool.poetry.group.dev]
    optional = true
jluethi commented 15 hours ago

Agreed! Let's have manifest creation in a separate package eventually (by fractal-tasks-core 2.0) and longer term, let's try to avoid this coming with a pydantic upper bound. A lower bound for pydantic seems necessary though.