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
11 stars 5 forks source link

Relax pydantic requirements || Switch to transitional pydantic.v1 #765

Closed lorenzocerrone closed 1 week ago

lorenzocerrone commented 2 weeks ago

Checklist before merging

Following the discussion in #760, this PR:

lorenzocerrone commented 2 weeks ago

Hi @tcompa, I have a couple of questions about the failed CI. Could you help me?

@jluethi

tcompa commented 2 weeks ago

CI (poetry): This failure could be fixed by pushing my poetry.lock file, but I am not sure what the correct policy is here. Should I push the poetry.lock file from my machine, or should I trigger a CI to update it?

You should push the poetry.lock file from your machine (happy to review this in case it introduces too much friction). It'd be best if you use e.g. poetry 1.8.2, to avoid spurious differences.

tcompa commented 2 weeks ago

CI (pip): In the pyproject.toml I can't find a good way to set pydantic = "==1.10.16 || >=2.6.3" (I have also tried "==1.10.16,>=2.6.3"). Do you know what the correct formatting is for this kind of requirement?

Just to get started, we could start with just >=1.10.16. Other than that, I guess there should be a way but I should have a second look at it

tcompa commented 2 weeks ago

Just to get started, we could start with just >=1.10.16. Other than that, I guess there should be a way but I should have a second look at it

A first look at https://python-poetry.org/docs/dependency-specification/#multiple-constraints-dependencies suggests that the original plan (either 1.10.16 or >=2.something) doesn't work in poetry, but from what I'm seeing at the moment it seems that this option doesn't exist for pyproject.toml specifications in general.

A likely-working approach here is to use of optional extras (e.g. pip install fractal-tasks-core[pydantic1]), but I would be very skeptical about it, especially since this is meant to be a transitional support for Pydantic V1.

The downside of sticking to just ">=1.10.16" is that if fractal-tasks-core is not compatible e.g. with pydantic <2.3.0, this will not be enforced anywhere and it will lead to unexpected failures downstream. Of course we can add a runtime check of the version, as a temporary solution.


@lorenzocerrone this resonates with our latest packaging discussion. Once we migrate to Pydantic v2 for tasks and core library, the reason for accepting a pydantic V1 dependency would only be due to the schema-generation tools. Having two different packages, in principle, would shift this issue to a more narrow space (namely a dev-tools package, where it's more reasonable to also introduce all sort of verbose runtime checks).

tcompa commented 2 weeks ago
  • CI (pip): In the pyproject.toml I can't find a good way to set pydantic = "==1.10.16 || >=2.6.3" (I have also tried "==1.10.16,>=2.6.3"). Do you know what the correct formatting is for this kind of requirement?

What kind of error do you get with pydantic = "==1.10.16||>=2.6.3"?

lorenzocerrone commented 2 weeks ago

This is the error I get running pip install -e .: pip._vendor.packaging.requirements.InvalidRequirement: Parse error at "'(1.10.16'": Expected string_end

Poetry often uses the syntax xx = "==1.* || >=2.*" in the lock file, so I tried to use it in the pyproject.toml. Since everything looked alright with poetry install ..., I assumed it would be the same for pip.

So the possible solutions are:

  1. Adding pydantic1 support as an extra.
  2. Use >=1.10.16 at the risk of potentially creating broken envs. I think this is likely to work fine in practice. To avoid crashes, we could: 2.1 Have a run-time check 2.2 Have a custom after-installation hook that fails installation if pydantic is >2 and <2.3.
  3. Wait for a full transition to v2 for schema generation. This is the cleanest approach, but it blocks some of the task building.
tcompa commented 2 weeks ago

pip._vendor.packaging.requirements.InvalidRequirement: Parse error at "'(1.10.16'": Expected string_end

Got it, thanks. I saw it was working on poetry but I had not checked with pip.

  1. Use >=1.10.16 at the risk of potentially creating broken envs. I think this is likely to work fine in practice. To avoid crashes, we could: [..]

I would be in favor of this option, with one of the mitigations you suggest.

lorenzocerrone commented 2 weeks ago

Got it, thanks. I saw it was working on poetry but I had not checked with pip.

Same here

  1. Use >=1.10.16 at the risk of potentially creating broken envs. I think this is likely to work fine in practice. To avoid crashes, we could: [..]

I would be in favor of this option, with one of the mitigations you suggest.

Good! I will first try the post install hook, and if that gets too convoluted, I will add simple runtime check. @jluethi Is this compromise ok for you?

jluethi commented 2 weeks ago

This sounds good to me! Either version of 2 is a good approach for my taste, as we very likely then basically never run into this check :)

github-actions[bot] commented 2 weeks ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  fractal_tasks_core
  __init__.py 16
  channels.py
  labels.py
  fractal_tasks_core/ngff
  specs.py
  fractal_tasks_core/roi
  v1_checks.py
  fractal_tasks_core/tables
  v1.py
  fractal_tasks_core/tasks
  apply_registration_to_image.py
  calculate_registration_image_based.py
  cellpose_segmentation.py
  cellpose_utils.py
  cellvoyager_to_ome_zarr_compute.py
  cellvoyager_to_ome_zarr_init.py
  cellvoyager_to_ome_zarr_init_multiplex.py
  copy_ome_zarr_hcs_plate.py
  find_registration_consensus.py
  illumination_correction.py
  image_based_registration_hcs_init.py
  import_ome_zarr.py
  init_group_by_well_for_multiplexing.py
  io_models.py
  maximum_intensity_projection.py
  napari_workflows_wrapper.py
Project Total  

This report was generated by python-coverage-comment-action

lorenzocerrone commented 2 weeks ago

In the end I implemented a simple run time check in the main __init__.py

https://github.com/fractal-analytics-platform/fractal-tasks-core/blob/ac8a453d19d13b180f27eee4aa0eeef0ea4c7f5f/fractal_tasks_core/__init__.py#L4-L20

I think this should be ready for review @tcompa @jluethi

tcompa commented 2 weeks ago

From a clean virtual env

$ pip install -e . pydantic==2.0.0
...

$ python -c 'import fractal_tasks_core'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/tommaso/Fractal/fractal-tasks-core/fractal_tasks_core/__init__.py", line 22, in <module>
    _check_pydantic_version()
  File "/home/tommaso/Fractal/fractal-tasks-core/fractal_tasks_core/__init__.py", line 16, in _check_pydantic_version
    raise ImportError(
ImportError: Pydantic version 2.0 is not supported. Please use version ==1.10.16 or  >=2.6.3.

---------------------

$ pip install -e . pydantic==1.10.15
Obtaining file:///home/tommaso/Fractal/fractal-tasks-core
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
  Getting requirements to build editable ... done
  Preparing editable metadata (pyproject.toml) ... done
Collecting pydantic==1.10.15
  Using cached pydantic-1.10.15-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (3.1 MB)
Requirement already satisfied: typing-extensions>=4.2.0 in ./venv/lib/python3.10/site-packages (from pydantic==1.10.15) (4.12.2)
Requirement already satisfied: docstring-parser<0.16,>=0.15 in ./venv/lib/python3.10/site-packages (from fractal-tasks-core==1.0.3a0) (0.15)
Requirement already satisfied: defusedxml<0.8.0,>=0.7.1 in ./venv/lib/python3.10/site-packages (from fractal-tasks-core==1.0.3a0) (0.7.1)
Requirement already satisfied: lxml<5.0.0,>=4.9.1 in ./venv/lib/python3.10/site-packages (from fractal-tasks-core==1.0.3a0) (4.9.4)
Requirement already satisfied: filelock==3.13.* in ./venv/lib/python3.10/site-packages (from fractal-tasks-core==1.0.3a0) (3.13.4)
Requirement already satisfied: pandas<2,>=1.2.0 in ./venv/lib/python3.10/site-packages (from fractal-tasks-core==1.0.3a0) (1.5.3)
Requirement already satisfied: dask>=2023.1.0 in ./venv/lib/python3.10/site-packages (from fractal-tasks-core==1.0.3a0) (2024.6.0)
INFO: pip is looking at multiple versions of <Python from Requires-Python> to determine which version is compatible with other requirements. This could take a while.
INFO: pip is looking at multiple versions of pydantic to determine which version is compatible with other requirements. This could take a while.
ERROR: Cannot install fractal-tasks-core==1.0.3a0 and pydantic==1.10.15 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested pydantic==1.10.15
    fractal-tasks-core 1.0.3a0 depends on pydantic>=1.10.16

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

---------------------

$ pip install -e . pydantic==1.10.16
...

$ python -c 'import fractal_tasks_core; import pydantic; print(pydantic.__version__)'
1.10.16

---------------------

$ pip install -e .
...

$ python -c 'import fractal_tasks_core; import pydantic; print(pydantic.__version__)'
2.7.4

This is all as expected.

tcompa commented 2 weeks ago

Quick question: is there any specific reason for accepting up to v2.6.2? Anything special supported only starting from 2. No strong opinion here, just a curiosity.

jluethi commented 2 weeks ago

Looks good to me overall! :)

lorenzocerrone commented 1 week ago

Quick question: is there any specific reason for accepting up to v2.6.2? Anything special supported only starting from 2. No strong opinion here, just a curiosity.

There are no strong reasons on my side. It was the lowest version I tested on my tasks (including generating some schemas). pydantic.v1 has existed since version 2.0, so in principle, we could test it a bit and lower the requirement.