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

Fully move to Pydantic v2 #793

Merged
merged 58 commits into from
Jul 24, 2024
Merged

Fully move to Pydantic v2 #793

merged 58 commits into from
Jul 24, 2024

Conversation

lorenzocerrone
Copy link
Collaborator

@lorenzocerrone lorenzocerrone commented Jul 16, 2024

Close #790
Close #791
Close #792
Close #796
Close #802

  • Transition all pydantic models to v2
  • Adapt validators
  • Correct metadata in test/data
  • Adapt tests

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

@lorenzocerrone
Copy link
Collaborator Author

@tcompa the v2 model conversion is ready. It pass all tests not related to manifest creation.
There is still a bit of work to do on my side to update some deprecated APIs like .dict

@lorenzocerrone lorenzocerrone changed the title Pydantic v2 second attempt [Do Not Merge] Pydantic v2 second attempt Jul 16, 2024
@tcompa
Copy link
Collaborator

tcompa commented Jul 22, 2024

Latest update about external packages:

  1. fractal-helper-tasks is fully under control (the diff only includes the args_schema_version field)
  2. A recent branch of APX is fully under control (all details in Switch to Pydantic V2? Apricot-Therapeutics/APx_fractal_task_collection#5)
  3. scmultiplex has the diff reported below, which does not seem to include anything breaking
diff --git a/src/scmultiplex/__FRACTAL_MANIFEST__.json b/src/scmultiplex/__FRACTAL_MANIFEST__.json
index 6fd5f0c..0b34938 100644
--- a/src/scmultiplex/__FRACTAL_MANIFEST__.json
+++ b/src/scmultiplex/__FRACTAL_MANIFEST__.json
@@ -1,5 +1,5 @@
 {
-  "args_schema_version": "pydantic_v1",
+  "args_schema_version": "pydantic_v2",
   "has_args_schemas": true,
   "manifest_version": "2",
   "task_list": [
@@ -35,8 +35,7 @@
         "type": "object"
       },
       "args_schema_parallel": {
-        "additionalProperties": false,
-        "definitions": {
+        "$defs": {
           "InitArgsRegistration": {
             "description": "Registration init args.",
             "properties": {
@@ -52,11 +51,12 @@
             "type": "object"
           }
         },
+        "additionalProperties": false,
         "properties": {
           "init_args": {
-            "$ref": "#/definitions/InitArgsRegistration",
+            "$ref": "#/$defs/InitArgsRegistration",
             "description": "Intialization arguments provided by `_image_based_registration_hcs_init`. They contain the reference_zarr_url that is used for registration. (standard argument for Fractal tasks, managed by Fractal server).",
-            "title": "Init_Args"
+            "title": "Init Args"
           },
           "iou_cutoff": {
             "default": 0.2,
@@ -140,8 +140,7 @@
         "type": "object"
       },
     {
       "args_schema_parallel": {
-        "additionalProperties": false,
-        "definitions": {
+        "$defs": {
           "ChannelInputModel": {
             "description": "A channel which is specified by either `wavelength_id` or `label`.",
             "properties": {
               "label": {
-                "description": "Name of the channel.",
+                "description": "Name of the channel. Can only be specified if wavelength_id is not set.",
                 "title": "Label",
                 "type": "string"
               },
               "wavelength_id": {
-                "description": "Unique ID for the channel wavelength, e.g. `A01_C01`.",
+                "description": "Unique ID for the channel wavelength, e.g. `A01_C01`. Can only be specified if label is not set.",
                 "title": "Wavelength Id",
                 "type": "string"
               }
@@ -1311,6 +1310,7 @@
             "type": "object"
           }
         },
+        "additionalProperties": false,
         "properties": {
           "allow_duplicate_labels": {
             "default": false,
@@ -1326,7 +1326,7 @@
           },
           "input_channels": {
             "additionalProperties": {
-              "$ref": "#/definitions/ChannelInputModel"
+              "$ref": "#/$defs/ChannelInputModel"
             },
             "description": "Dictionary of channels to measure. Keys are the names that will be added as prefixes to the measurements, values are another dictionary containing either wavelength_id or channel_label information to allow Fractal to find the correct channel (but not both). Example: {\"C01\": {\"wavelength_id\": \"A01_C01\"}. To only measure morphology, provide an empty dict",
             "title": "Input Channels",

@tcompa
Copy link
Collaborator

tcompa commented Jul 22, 2024

The last missing check on the produced manifests is the one for fractal-tasks-core, which is also under control (full details at fractal-analytics-platform/fractal-web#531).

@tcompa
Copy link
Collaborator

tcompa commented Jul 22, 2024

This PR is now mostly ready, and could be merged any time.

Minor reviews can still take place, before or after merging:

  1. With @lorenzocerrone we plan to test the JSON Schema tools on yet another package
  2. Before using the newly-generated schema, a (minor) fix and a new version are required in fractal-web - Support new pydantic_v2 manifest and JSON Schemas fractal-web#531

Once we merge this PR, it will be a bit cumbersome to switch back to Pydantic V1 (cc @jluethi)

@tcompa tcompa marked this pull request as ready for review July 22, 2024 14:05
@lorenzocerrone
Copy link
Collaborator Author

lorenzocerrone commented Jul 23, 2024

I found a minor issue in handling tuples in pydantic v2 that can be potentially problematic.

If I have the following tuple definition in a pydantic model:

patch: tuple[int, int, int] = (80, 160, 160)

The schema generated is slightly different between v2 and v1

V1:

"patch": {
                "title": "Patch",
                "default": [
                  80,
                  160,
                  160
                ],
                "type": "array",
                "minItems": 3,
                "maxItems": 3,
                "items": [
                  {
                    "type": "integer"
                  },
                  {
                    "type": "integer"
                  },
                  {
                    "type": "integer"
                  }
                ],
                "description": "The patch size."
              },

V2:

"patch": {
                ...
                "prefixItems": [
                  {
                    "type": "integer"
                  },
                  {
                    "type": "integer"
                  },
                  {
                    "type": "integer"
                  }
                ]
               ...
              },

The V2 version is not shown in the fractal web UI.

@tcompa, should this be addressed on fractal web or as a custom schema generation?

@tcompa
Copy link
Collaborator

tcompa commented Jul 23, 2024

I found a minor issue in handling tuples in pydantic v2 that can be potentially problematic.

Great, thanks for catching it!

should this be addressed on fractal web or as a custom schema generation?

This is one of the new features in JSON Schema Draft 2020-12, which pydantic2-generated JSON Schemas should comply with.

Therefore I guess it's best to adapt fractal-web, rather than sticking with some old JSON Schemas.

@tcompa
Copy link
Collaborator

tcompa commented Jul 23, 2024

Therefore I guess it's best to adapt fractal-web, rather than sticking with some old JSON Schemas.

Ref fractal-analytics-platform/fractal-web#534

@tcompa tcompa merged commit 8ab8499 into main Jul 24, 2024
17 of 19 checks passed
@tcompa tcompa deleted the pydantic-v2-second-attempt branch July 24, 2024 13:08
@tcompa tcompa mentioned this pull request Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants