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

Pin pydantic to <=2.8.2 #834

Closed
jluethi opened this issue Sep 7, 2024 · 4 comments · Fixed by #836
Closed

Pin pydantic to <=2.8.2 #834

jluethi opened this issue Sep 7, 2024 · 4 comments · Fixed by #836
Labels
dependencies Pull requests that update a dependency file JSON Schemas

Comments

@jluethi
Copy link
Collaborator

jluethi commented Sep 7, 2024

In our arg schema creation, we rely quite heavily on pydantic._internal imports. Some of those appear to break in pydantic v2.9.0. I can't currently trace down which PR broke it, but for example this part was removed in Pydantic 2.9.0:

from pydantic._internal import _typing_extra
_typing_extra.add_module_globals()

add_module_globals does not appear to exist anymore in pydantic v2.9.0.

We either pin to pydantic 2.8.2 (for the moment) & update that in the templates. Or we come up with a fix to adapt to this pydantic change.

Big picture, the less we need to rely on _internal the better. As long as we rely on it somewhat heavily: Do we want to pin pydantic for the time being to a max version? Requiring 2.9.0 may also be somewhat aggressive, given that it was just released on September 5th.

@jluethi
Copy link
Collaborator Author

jluethi commented Sep 7, 2024

@tcompa
Copy link
Collaborator

tcompa commented Sep 9, 2024

The bottom line here is that part of our use cases (extracting a model from a call signature) is not supported by pydantic, and we made it work by mimicking what they do within validate_call (see https://github.com/pydantic/pydantic/blob/cdca7aebba33770416c260335e914c1755c48f13/pydantic/_internal/_validate_call.py).

We can likely reduce/remove some _internal imports, but it'd still be a trade-off because we'd have to copy part of their code or logic into our dev tools. I still think it's a good direction, at least when this concerns very small logic (e.g. the typing-extra one, that mostly boils down to a single statement). I'll open a "maintenance" issue about that.

On the other hand, I think that in this case the context is key: we don't really want/need to encourage task developers to rely on cutting-edge pydantic features (which is different from depending on e.g. scipy/numpy/sklearn/skimage/anndata/dask/cellpose cutting-edge features - see e.g. #821 or #833).
Thus I'm strongly in favor of introducing a (very mild) usage limitation, by constraining pydantic to the latest version that works with our current main (<=2.8.2), and reducing our dependency on pydantic._internal with a lower priority.

@tcompa tcompa added dependencies Pull requests that update a dependency file JSON Schemas labels Sep 9, 2024
@tcompa tcompa changed the title Adapt to pydantic v2.9.0 changes or pin to 2.8.2 Pin pydantic to <=2.8.2 Sep 9, 2024
@tcompa tcompa linked a pull request Sep 9, 2024 that will close this issue
1 task
tcompa added a commit that referenced this issue Sep 9, 2024
@jluethi
Copy link
Collaborator Author

jluethi commented Sep 9, 2024

Agree that supporting the most cutting edge pydantic features isn't a September priority for sure! Fully fine with pinning this for the time being.

Let's discuss the best way forward to ensure our manifest building is maintainable and that we eventually can support newer pydantic versions longer-term.

@tcompa
Copy link
Collaborator

tcompa commented Sep 10, 2024

I merged #836 and released fractal-tasks-core v1.3.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file JSON Schemas
Projects
Development

Successfully merging a pull request may close this issue.

2 participants