⬆️ Upgrade models-library (pydantic v2) #6333

giancarloromeo · 2024-09-09T21:37:19Z

What do these changes do?

Related issue/s

resolves Major upgrade to pydantic 2.0 #4481

How to test

Dev-ops checklist

No ENV changes or I properly updated ENV (read the instruction)

codecov · 2024-09-09T21:41:59Z

Codecov Report

Attention: Patch coverage is 90.64857% with 62 lines in your changes missing coverage. Please review.

Please upload report for BASE (pydantic_v2_migration@6d6f5c5). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...ary/src/models_library/utils/json_serialization.py	65.6%	6 Missing and 5 partials ⚠️
...models_library/utils/_original_fastapi_encoders.py	57.8%	5 Missing and 3 partials ⚠️
.../models_library/api_schemas_directorv2/clusters.py	66.6%	5 Missing ⚠️
...odels-library/src/models_library/projects_nodes.py	68.7%	5 Missing ⚠️
...src/models_library/api_schemas_webserver/groups.py	66.6%	4 Missing ⚠️
...els-library/src/models_library/user_preferences.py	60.0%	4 Missing ⚠️
...-library/src/models_library/api_schemas_storage.py	81.2%	3 Missing ⚠️
...ckages/models-library/src/models_library/docker.py	70.0%	3 Missing ⚠️
...ages/models-library/src/models_library/payments.py	70.0%	3 Missing ⚠️
...rary/src/models_library/service_settings_labels.py	91.6%	2 Missing and 1 partial ⚠️
... and 10 more

Additional details and impacted files

@@                   Coverage Diff                   @@
##             pydantic_v2_migration   #6333   +/-   ##
=======================================================
  Coverage                         ?   82.3%           
=======================================================
  Files                            ?     297           
  Lines                            ?   10035           
  Branches                         ?     704           
=======================================================
  Hits                             ?    8261           
  Misses                           ?    1684           
  Partials                         ?      90

Flag	Coverage Δ
unittests	`82.3% <90.6%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...models-library/src/models_library/access_rights.py	`100.0% <100.0%> (ø)`
...models-library/src/models_library/aiodocker_api.py	`70.8% <100.0%> (ø)`
...ary/src/models_library/api_schemas__common/meta.py	`100.0% <100.0%> (ø)`
.../models_library/api_schemas_api_server/api_keys.py	`100.0% <100.0%> (ø)`
...src/models_library/api_schemas_catalog/services.py	`100.0% <100.0%> (ø)`
...dels_library/api_schemas_catalog/services_ports.py	`95.6% <100.0%> (ø)`
...ary/api_schemas_catalog/services_specifications.py	`100.0% <ø> (ø)`
...odels_library/api_schemas_directorv2/comp_tasks.py	`83.6% <100.0%> (ø)`
...library/api_schemas_directorv2/dynamic_services.py	`96.4% <100.0%> (ø)`
...rc/models_library/api_schemas_directorv2/health.py	`100.0% <100.0%> (ø)`
... and 82 more

packages/models-library/src/models_library/api_schemas_directorv2/dynamic_services_service.py

packages/models-library/src/models_library/rabbitmq_messages.py

matusdrobuliak66

Thanks a lot, looks good! 💯 I have added few questions.

sanderegg

I guess this is still in progress.
There are missing model examples and a =None that might have been added by the migration tool?

Happy to review again once these are in. thanks a lot for the good work!!!

packages/models-library/src/models_library/api_schemas_clusters_keeper/clusters.py

packages/models-library/src/models_library/api_schemas_directorv2/clusters.py

packages/models-library/src/models_library/api_schemas_directorv2/dynamic_services.py

packages/models-library/src/models_library/api_schemas_directorv2/health.py

giancarloromeo · 2024-09-10T07:20:18Z

Thanks a lot for your feedback, the PR is still in progress :) All your precious observations are already under control.

GitHK

this is my initial round. I'd like to talk about this PR in person if possible. there are some things that I would like to understand .

Also it's extreenmly hard to review

packages/models-library/src/models_library/api_schemas_catalog/services.py

packages/models-library/src/models_library/api_schemas_clusters_keeper/clusters.py

packages/models-library/src/models_library/api_schemas_directorv2/clusters.py

GitHK · 2024-09-16T13:27:01Z

packages/models-library/scripts/validate-pg-projects.py

-    class Config(ProjectAtDB.Config):
-        extra = Extra.forbid
-
    # TODO: missing in ProjectAtDB


can you please check if this comment has any relevance and just drop it if not?

packages/models-library/tests/test__pydantic_models.py

GitHK · 2024-09-16T14:59:41Z

packages/models-library/tests/test_errors_classes.py

-    with pytest.raises(KeyError, match="missing"):
-        str(MyErrorBefore(value=42))


what about this example? why was it removed?

MyErrorBefore inherited from PydanticErrorMixin that in Pydantic v2 changed completely.

See: https://github.com/pydantic/pydantic/blob/cf671c808fba1a78bc4995e8276dd9e48876f2d9/pydantic/errors.py#L72

packages/models-library/tests/test_project_nodes.py

GitHK · 2024-09-16T15:06:21Z

packages/models-library/tests/test_utils_pydantic_tools_extension.py


 def test_schema():
-    assert MyModel.schema() == {
+    assert MyModel.model_json_schema() == {


looks like your changes broke the test. Are you sure?

Actually this tests is showing that now pydantic NICELY distinguishes nullable fields. NOTE

class MyModel(BaseModel): a: int b: int | None = Field(...) c: int = 42 d: int | None = None e: int = FieldNotRequired(description="optional non-nullable")

And we should get an schema that defines:

a is a required int

b is a required either int or Nullable

c is an optional int that defaults to 42

d is an optional either int or Nullable that defaults to None

e is an optional int (if not set, it just gives you None)

@GitHK: change is correct.

The method "schema" in class "BaseModel" is deprecated
The schema method is deprecated; use model_json_schema instead.

…/services.py Co-authored-by: Andrei Neagu <[email protected]>

pcrespov

Bravo! I left some comments and questions. Also
On my side this is good enough for a first merge. We can further refine.

packages/models-library/src/models_library/api_schemas_catalog/services_specifications.py

pcrespov · 2024-09-16T15:35:07Z

packages/models-library/src/models_library/api_schemas_directorv2/clusters.py

+        if v is None:
+            cluster_type = values["type"]
+            default_thumbnails = {
+                ClusterTypeInModel.AWS.value: "https://upload.wikimedia.org/wikipedia/commons/thumb/9/93/Amazon_Web_Services_Logo.svg/250px-Amazon_Web_Services_Logo.svg.png",


THOUGHT: wouldn't it make more sense to have links here to assets that we control? they could indeed be a copy of these (if allowed). At least we make sure nobody changes the source
e.g. we have https://github.com/ITISFoundation/osparc-assets repo where we could add default assets link these

pcrespov · 2024-09-16T15:38:43Z

packages/models-library/src/models_library/api_schemas_long_running_tasks/base.py

-    @validate_arguments
+    @validate_call
    def update(
        self,


I have the impression that here self is also validated. Do we want this? I think there is a way to skip some of the parameters if so.

packages/models-library/src/models_library/api_schemas_long_running_tasks/base.py

pcrespov · 2024-09-16T15:41:34Z

packages/models-library/src/models_library/api_schemas_storage.py

-class FileMetaDataArray(BaseModel):
-    __root__: list[FileMetaDataGet] = []
+class FileMetaDataArray(RootModel[list[FileMetaDataGet]]):
+    root: list[FileMetaDataGet] = []


THOUGHT: IMO we should get rid of all these root models. They are very confusing. In this case, I would rather annotated list[FileMetaDataGet] than FileMetaDataArray

pcrespov · 2024-09-16T15:56:26Z

packages/models-library/src/models_library/basic_types.py

+AnyUrl = Annotated[str, pydantic.AnyUrl]
+
+AnyHttpUrl = Annotated[str, pydantic.AnyHttpUrl]
+
+HttpUrl = Annotated[str, pydantic.HttpUrl]



Overriding names this way is misleading ...
Either you use different names or force users to use explicit Annotated (my preferred option)

Imaging this scenario:

from models_library.basic_types import AnyUrl from pydantic import * class MyModel(BaseModel): url : Annotated[str, AnyUrl]

pcrespov · 2024-09-16T16:00:58Z

packages/models-library/src/models_library/generics.py


-class DictModel(GenericModel, Generic[DictKey, DictValue]):
-    __root__: dict[DictKey, DictValue]
+class DictModel(RootModel[dict[DictKey, DictValue]], Generic[DictKey, DictValue]):


if ListModel and DictModel are not used anywhere in the codebase, I suggest to drop them and rather use explicit list[MyModel] and dict[str, MyModel] CC @sanderegg , @GitHK @matusdrobuliak66

packages/models-library/src/models_library/progress_bar.py

pcrespov · 2024-09-16T16:10:05Z

packages/models-library/tests/test_utils_pydantic_tools_extension.py


 def test_schema():
-    assert MyModel.schema() == {
+    assert MyModel.model_json_schema() == {


Actually this tests is showing that now pydantic NICELY distinguishes nullable fields. NOTE

class MyModel(BaseModel): a: int b: int | None = Field(...) c: int = 42 d: int | None = None e: int = FieldNotRequired(description="optional non-nullable")

And we should get an schema that defines:

a is a required int

b is a required either int or Nullable

c is an optional int that defaults to 42

d is an optional either int or Nullable that defaults to None

e is an optional int (if not set, it just gives you None)

pcrespov · 2024-09-16T16:11:11Z

packages/models-library/tests/test_utils_pydantic_tools_extension.py

+                "title": "D",
+            },
            "e": {
+                "default": None,


I just wonder whether we could change FieldNotRequired to returns some NotSet -like value

giancarloromeo · 2024-09-16T19:51:47Z

From Pydantic v1 official documentation:

If you want to specify a field that can take a None value while still being required, you can use Optional with ...:

from pydantic import BaseModel, Field, ValidationError


class Model(BaseModel):
    a: int | None
    b: int | None = ...
    c: int | None = Field(...)


print(Model(b=1, c=2))
#> a=None b=1 c=2
try:
    Model(a=1, b=2)
except ValidationError as e:
    print(e)
    """
    1 validation error for Model
    c
      field required (type=value_error.missing)
    """

In this model, a, b, and c can take None as a value. But a is optional, while b and c are required. b and c require a value, even if the value is None.

Details: https://docs.pydantic.dev/1.10/usage/models/#required-optional-fields

matusdrobuliak66

🥇

giancarloromeo · 2024-09-17T20:08:26Z

packages/models-library/src/models_library/api_schemas_webserver/projects.py

-    ui: EmptyModel | StudyUI | None
+    state: ProjectState | None = None
+    ui: EmptyModel | StudyUI | None = None
    quality: dict[str, Any] = {}


@pcrespov If I revert the = None for the fields in this model, the create-project-schemas test fails because the mocked response payload doesn't contain them. Are these fields intended as optional?

@giancarloromeo IMO this case a default =None makes sense

These mocks were generated using real payload from the front-end so it is important that they pass since there is no guarantee that the front-end always follows the openapi specs in the web-api.
Also, as you can see in services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml that these fields are not required:

BTW, once all the tests in the models_library pass, you might want to try to re-generate services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml and see if the changes make sense. In principle only the new nullable fields should be update and the rest should be the same. Pay special attention to required fields for instance ...
For that you can use

cd web/server make openapi-specs

sonarqubecloud · 2024-09-18T08:22:55Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

remove version constraint

6c6bebc

giancarloromeo requested review from GitHK, matusdrobuliak66 and sanderegg September 9, 2024 21:37

giancarloromeo self-assigned this Sep 9, 2024

upgrade requirements

533bdd1

giancarloromeo added 2 commits September 9, 2024 23:44

run bump-pydantic

c1bbc52

fix bump-pydantic issues

76f0fc6

giancarloromeo added this to the Eisbock milestone Sep 9, 2024

giancarloromeo added the t:maintenance Some planned maintenance work label Sep 9, 2024

matusdrobuliak66 reviewed Sep 10, 2024

View reviewed changes

packages/models-library/src/models_library/api_schemas_directorv2/dynamic_services_service.py Outdated Show resolved Hide resolved

matusdrobuliak66 reviewed Sep 10, 2024

View reviewed changes

packages/models-library/src/models_library/rabbitmq_messages.py Outdated Show resolved Hide resolved

matusdrobuliak66 reviewed Sep 10, 2024

View reviewed changes

sanderegg reviewed Sep 10, 2024

View reviewed changes

giancarloromeo added 10 commits September 10, 2024 10:44

fix bump-pydantic issues

55b72c7

fix bump-pydantic issues

df01a19

fix bump-pydantic issues

41b8797

add pydantic-settings dependency

7c61b0e

fix bump-pydantic issues

e4c30e0

add pydantic-extra-types dependency

f25f246

fix bump-pydantic issues

ba507d7

fix bump-pydantic issues

f2c72fe

fix field names

4f2d46d

fix schemas

e62831c

giancarloromeo changed the title ~~⬆️ WIP: Upgrade models library~~ ⬆️ WIP: Upgrade models-library Sep 10, 2024

giancarloromeo added 3 commits September 10, 2024 16:22

fix model_config refs

4a2d1c3

fix basic types

66edc00

fix rabbitmq basic types

b4b8c87

giancarloromeo added 2 commits September 16, 2024 16:21

fix validation issues

3ba5029

fix model validator mode

b328087

GitHK reviewed Sep 16, 2024

View reviewed changes

Update packages/models-library/src/models_library/api_schemas_catalog…

598b9dd

…/services.py Co-authored-by: Andrei Neagu <[email protected]>

pcrespov approved these changes Sep 16, 2024

View reviewed changes

fix thumbnail type

cecf668

giancarloromeo changed the title ~~⬆️Upgrade models-library (pydantic v2)~~ ⬆️ Upgrade models-library (pydantic v2) Sep 16, 2024

giancarloromeo added 2 commits September 16, 2024 22:20

remove empty model config

a6a86e0

update format

0abee29

matusdrobuliak66 approved these changes Sep 17, 2024

View reviewed changes

giancarloromeo added 8 commits September 17, 2024 09:51

add typealias

c4104c7

revert bump-pydantic changes

bafab91

revert change

ea624d1

fix union mode

88ec517

fix default value

b62770d

fix url types

96a93ac

fix format

cbf481e

fix format

2c18b4f

giancarloromeo commented Sep 17, 2024

View reviewed changes

giancarloromeo added 4 commits September 18, 2024 09:39

fix S3BucketName validation

c97244f

remove unnecessary todos

c049903

fix codesmell

65ced45

fix codesmell

223bbcc

sanderegg approved these changes Sep 18, 2024

View reviewed changes

giancarloromeo merged commit d2d81c0 into ITISFoundation:pydantic_v2_migration Sep 18, 2024
27 of 57 checks passed

giancarloromeo deleted the is4481/upgrade-models-library branch September 18, 2024 09:50

giancarloromeo linked an issue Sep 18, 2024 that may be closed by this pull request

Major upgrade to pydantic 2.0 #4481

Closed

giancarloromeo mentioned this pull request Oct 21, 2024

Major upgrade to pydantic 2.0 #4481

Closed

		with pytest.raises(KeyError, match="missing"):
		str(MyErrorBefore(value=42))

Uh oh!

⬆️ Upgrade models-library (pydantic v2) #6333

⬆️ Upgrade models-library (pydantic v2) #6333

Uh oh!

Conversation

giancarloromeo commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related issue/s

How to test

Dev-ops checklist

Uh oh!

codecov bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

matusdrobuliak66 left a comment

Choose a reason for hiding this comment

Uh oh!

sanderegg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giancarloromeo commented Sep 10, 2024

Uh oh!

GitHK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcrespov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giancarloromeo commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matusdrobuliak66 left a comment

giancarloromeo commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

giancarloromeo commented Sep 16, 2024 •

edited

Loading

giancarloromeo Sep 17, 2024 •

edited

Loading

pcrespov Sep 18, 2024 •

edited

Loading