Skip to content

Relaxing pydantic requirements #760

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

Closed
lorenzocerrone opened this issue Jun 10, 2024 · 6 comments
Closed

Relaxing pydantic requirements #760

lorenzocerrone opened this issue Jun 10, 2024 · 6 comments
Labels
enhancement New feature or request good first issue

Comments

@lorenzocerrone
Copy link
Collaborator

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 napari/napari#6358 until there is a good solution for V2 and, in the meantime, update the requirements for fractal-tasks-core?

@tcompa
Copy link
Collaborator

tcompa commented Jun 10, 2024

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
    # Create and clean up schema
    vf = ValidatedFunction(task_function, config=None)
    schema = vf.model.schema()
    schema = _remove_args_kwargs_properties(schema)
    schema = _remove_pydantic_internals(schema)
    schema = _remove_attributes_from_descriptions(schema)

Would it be an option to use the legacy pydantic.v1 like in napari/napari#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
Copy link
Collaborator Author

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

jluethi commented Jun 10, 2024

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

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

tcompa commented Jun 12, 2024

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:

  • The core-library and tasks will fully move to Pydantic V2
  • Manifest building will work both for generating pydantic_v1 schemas (by using pydantic.v1 imports and the current logic) and pydantic_v2 schemas (by using a new logic similar to the one I tested in [DO NOT MERGE] Update pydantic to v2 #737).
  • Manifest building for fractal-tasks-core itself will use the pydantic_v2 option. Other packages can specify this by setting args_schema_version, or we can infer it at runtime by looking at the installed Pydantic version.

What would not be supported is the following scenario

  • I have a task package with Pydantic V1 models, also used as argument
  • I want to build the manifest with Pydantic V2

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

  • I re-use some of the fractal-tasks-core input models, which will be (after Switch to Pydantic v2 #592) written in Pydantic V2, and I need these models when building my manifest.
  • I want to stick to Pydantic V1 for my package dependency and internal models.

@tcompa
Copy link
Collaborator

tcompa commented Jul 1, 2024

This was closed with #765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue
Projects
Development

No branches or pull requests

3 participants