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

Relaxing pydantic requirements #760

Closed lorenzocerrone closed 1 day ago

lorenzocerrone commented 3 weeks ago

Hi @tcompa

I have seen that switching to pydantic v2 #592 still has some issues. But I have encountered a couple of critical installation issues with packages depending on pydantic>2 while trying to create new tasks (for example, https://github.com/bioimage-io/spec-bioimage-io that requires pydantic>=2.6.3, 3).

Would it be an option to use the legacy pydantic.v1 like in https://github.com/napari/napari/pull/6358 until there is a good solution for V2 and, in the meantime, update the requirements for fractal-tasks-core?

tcompa commented 3 weeks ago

For context, we use Pydantic for three parts of fractal-tasks-core

  1. We define models for objects to be used within the core library and/or tasks (including for task arguments - see point 3).
  2. We validate arguments of the task functions, with the validate_arguments decorator
  3. We generate JSON Schemas for the task arguments, with the core functionality being the one in this block https://github.com/fractal-analytics-platform/fractal-tasks-core/blob/243b3f43fde234be77d3469866938d9ae803ef58/fractal_tasks_core/dev/lib_args_schemas.py#L230-L235

Would it be an option to use the legacy pydantic.v1 like in https://github.com/napari/napari/pull/6358 until there is a good solution for V2 and, in the meantime, update the requirements for fractal-tasks-core?

Yes, I think this should work. We would bump the required version e.g. to >=2.0.0, and replace all current imports with pydantic.v1. Note that the three items above would still all work in Pydantic V1. This means that if spec-bioimage-io defines a Pydantic V2 model that you may want to use for a task argument, it's not guaranteed that it will work. If V2 models are only used internally, within spec-bioimage-io, then I don't think there will be issues.

Whether this intermediate step makes sense mostly depends on when we plan to tackle #592. For the record, preparing #737 was not a crazy amount of work (there are a bunch of validators that need to be ported to the new style, and https://github.com/pydantic/bump-pydantic also helped). There is a single critical point which concerns point 3 above, but the rest is mostly repetitive work..

lorenzocerrone commented 3 weeks ago

Thanks for the clarification. In both problematic dependencies I have, pydantic is only used internally. So it won't be exposed to fractal-tasks-core validation/schema generation. The only issue I can foresee is that if not properly documented, this can be confusing for task creators. They might be tempted to use V2 models in their tasks.

@jluethi, what's your opinion on this issue?

If we decide to take the intermediate step, I can prepare a PR. It should be fairly straightforward, and I will see more of the fractal code base.

jluethi commented 3 weeks ago

@lorenzocerrone Is this urgent or can we review on Wednesday on how best to proceed?

My takeaway would be: I'm generally in favor of moving us to actually use Pydantic V2 in tasks core.

We will need to ensure that the manifest building works for Pydantic V1 & v2 at least for a certain transition phase though, as the current task packages by other people may also use Pydantic V1. If we can have a good way of switching fractal tasks core to Pydantic V2 while maintaining this backwards compatibility, I'd be in favor of this being focused on instead of the pydantic.v1 imports. Maybe also a good way to have deeper fractal task core discussion between @tcompa & @lorenzocerrone :)

If we see this transition to be tricky, then I'm fine with first building pydantic.v1 version of fractal-tasks-core.

lorenzocerrone commented 3 weeks ago

@jluethi, No, it's not urgent; we can discuss it on Wednesday. @tcompa, we can also find a time slot on Wednesday to discuss the fractal core in detail.

tcompa commented 2 weeks ago

The most recent versions of Pydantic V1 now also offer the possibility of doing from pydantic.v1 import ..., to simplify the V2 transition (see https://docs.pydantic.dev/1.10/changelog).

I think this is helpful for us, and we could proceed as follows:

  1. Bump the Pydantic requirement to ==1.10.16 (which is likely the last V1 version).
  2. Replace all imports with pydantic.v1.
  3. Check that all tests pass.
  4. Change the Pydantic requirement to accept both ==1.10.16 and >=2.7.0 (or any other recent 2.X version).
  5. Update Pydantic to V2 in poetry.lock, and check that all tests pass.

This allows us to unlock the dependency constraint, while keeping fractal-tasks-core fully in Pydantic V1 and with not version-specific code. The next step is then #592, which means that:

What would not be supported is the following scenario

Another non-supported use case (possibly more critical) would be

tcompa commented 1 day ago

This was closed with #765