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

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

Merged
merged 16 commits into from
Jun 20, 2024

Conversation

lorenzocerrone
Copy link
Collaborator

@lorenzocerrone lorenzocerrone commented Jun 17, 2024

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

Following the discussion in #760, this PR:

  • Changes all imports to transitional pydantic.v1
  • Relax pydantic to pydantic = ">=1.10.16"
  • Tests pass both with pydantic==1.10.16 & pydantic>=2.6.3
  • Add version check that fails if pydantic >2* & <2.6.3

@lorenzocerrone lorenzocerrone requested review from jluethi and tcompa and removed request for tcompa and jluethi June 17, 2024 13:19
@lorenzocerrone
Copy link
Collaborator Author

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

  • 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?

  • 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?

@jluethi

@tcompa
Copy link
Collaborator

tcompa commented Jun 17, 2024

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
Copy link
Collaborator

tcompa commented Jun 17, 2024

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
Copy link
Collaborator

tcompa commented Jun 17, 2024

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 tcompa mentioned this pull request Jun 17, 2024
@tcompa
Copy link
Collaborator

tcompa commented Jun 18, 2024

  • 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
Copy link
Collaborator Author

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
Copy link
Collaborator

tcompa commented Jun 18, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

jluethi commented Jun 18, 2024

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 :)

Copy link

github-actions bot commented Jun 18, 2024

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
Copy link
Collaborator Author

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

def _check_pydantic_version():
"""
Temporary check for pydantic version.
To be removed after moving to pydantic v2 is complete.
"""
import importlib.metadata
from packaging import version
pydantic_version = version.parse(importlib.metadata.version("pydantic"))
pydantic_v1 = version.parse("1.10.16")
pydantic_v2 = version.parse("2.6.3")
if pydantic_version != pydantic_v1 and pydantic_version < pydantic_v2:
raise ImportError(
f"Pydantic version {pydantic_version} is not supported. "
f"Please use version =={pydantic_v1} or >={pydantic_v2}."
)

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

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -208,7 +208,7 @@ def napari_workflows_wrapper(
if image_inputs:
img_array = da.from_zarr(f"{zarr_url}/{level}")
# Loop over image inputs and assign corresponding channel of the image
for (name, params) in image_inputs:
for name, params in image_inputs:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question: which tool/version/configuration is making this change? (or did you make it on purpose?)

We don't have such a strict definition of syntax in the repo, and the change is OK.
But let's make sure we use the same tools/versions/configurations to avoid unnecessary diffs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was by mistake, and I agree we should avoid this unnecessary diff. I have been using pro-commits, but I still have some nonstandard setup in my VSCode. I will try to fix it, and if I still have some issues, we can discuss them in our next one-on-one.

Copy link
Collaborator

@tcompa tcompa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple of tiny comments, but it's all good from my side.

@tcompa
Copy link
Collaborator

tcompa commented Jun 18, 2024

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
Copy link
Collaborator

tcompa commented Jun 18, 2024

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
Copy link
Collaborator

jluethi commented Jun 18, 2024

Looks good to me overall! :)

@lorenzocerrone
Copy link
Collaborator Author

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.

@lorenzocerrone lorenzocerrone merged commit 62c4192 into main Jun 20, 2024
19 checks passed
@lorenzocerrone lorenzocerrone deleted the pydantic_v2 branch June 20, 2024 07:55
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.

3 participants