⬆️ Upgrade API Server service (Pydantic v2) #6580

giancarloromeo · 2024-10-23T08:02:28Z

What do these changes do?

Upgrade the API Server service to Pydantic v2.

Related issue/s

How to test

make devenv
source .venv/bin/activate
cd services/api-server
make install-dev
make mypy
make test-dev-unit

Dev-ops checklist

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

codecov · 2024-10-23T08:08:39Z

Codecov Report

Attention: Patch coverage is 89.54248% with 16 lines in your changes missing coverage. Please review.

Project coverage is 81.9%. Comparing base (fb48eb1) to head (fd1ae8d).
Report is 3 commits behind head on pydantic_v2_migration_do_not_squash_updates.

Files with missing lines	Patch %	Lines
...simcore_service_api_server/services/director_v2.py	55.5%	4 Missing ⚠️
...src/simcore_service_api_server/services/storage.py	25.0%	3 Missing ⚠️
...src/simcore_service_api_server/api/routes/files.py	66.6%	1 Missing ⚠️
...er/exceptions/handlers/_httpx_client_exceptions.py	50.0%	1 Missing ⚠️
...erver/exceptions/handlers/_log_streaming_errors.py	50.0%	1 Missing ⚠️
...i_server/exceptions/handlers/_validation_errors.py	0.0%	1 Missing ⚠️
...simcore_service_api_server/models/schemas/files.py	87.5%	1 Missing ⚠️
...mcore_service_api_server/models/schemas/solvers.py	85.7%	1 Missing ⚠️
...mcore_service_api_server/services/log_streaming.py	80.0%	1 Missing ⚠️
...pi_server/services/solver_job_models_converters.py	88.8%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@                              Coverage Diff                              @@
##           pydantic_v2_migration_do_not_squash_updates   #6580     +/-   ##
=============================================================================
+ Coverage                                         80.9%   81.9%   +0.9%     
=============================================================================
  Files                                              289     512    +223     
  Lines                                            10605   19536   +8931     
  Branches                                           346    1039    +693     
=============================================================================
+ Hits                                              8584   16007   +7423     
- Misses                                            1975    3365   +1390     
- Partials                                            46     164    +118

Flag	Coverage Δ
unittests	`81.9% <89.5%> (+0.9%)`	⬆️

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

Files with missing lines	Coverage Δ
...c/simcore_service_api_server/api/routes/solvers.py	`63.7% <100.0%> (ø)`
...core_service_api_server/api/routes/solvers_jobs.py	`100.0% <ø> (ø)`
...vice_api_server/api/routes/solvers_jobs_getters.py	`93.3% <100.0%> (ø)`
...c/simcore_service_api_server/api/routes/studies.py	`100.0% <100.0%> (ø)`
...src/simcore_service_api_server/core/application.py	`92.9% <100.0%> (ø)`
...er/src/simcore_service_api_server/core/settings.py	`100.0% <100.0%> (ø)`
...imcore_service_api_server/db/repositories/users.py	`100.0% <100.0%> (ø)`
...src/simcore_service_api_server/exceptions/_base.py	`100.0% <100.0%> (ø)`
...e_api_server/exceptions/handlers/_custom_errors.py	`84.6% <100.0%> (ø)`
...er/exceptions/handlers/_handlers_backend_errors.py	`100.0% <100.0%> (ø)`
... and 23 more

... and 241 files with indirect coverage changes

giancarloromeo · 2024-10-23T09:45:36Z

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

-    workspace_id: WorkspaceID | None
-    folder_id: FolderID | None
+    permalink: ProjectPermalink | None = FieldNotRequired()
+    workspace_id: WorkspaceID | None = None


@matusdrobuliak66 confirmed that workspace_id and folder_id are optional

these are only for jsonschema generation right? what confuses me is when you say "optional" this does not mean with a default of None

giancarloromeo · 2024-10-23T09:46:40Z

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

    )

+    model_config = ConfigDict(
+        frozen=False


Superclass is frozen and the behaviour was inherited.

thanks! I guess these went off with the re-creation of the branch. how did you find that out? in the warnings of pytest??

In a test there was a direct assignment of a field in that model.

giancarloromeo · 2024-10-23T09:46:53Z

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

    modified: datetime

+    model_config = ConfigDict(
+        frozen=False


Superclass is frozen and the behaviour was inherited.

same question

In a test there was a direct assignment of a field in that model.

giancarloromeo · 2024-10-23T09:52:32Z

services/api-server/Makefile

 # specification of the used openapi-generator-cli (see also https://github.com/ITISFoundation/openapi-generator)
-OPENAPI_GENERATOR_NAME := itisfoundation/openapi-generator-cli-openapi-generator-v4.2.3
-OPENAPI_GENERATOR_TAG := v0
+OPENAPI_GENERATOR_NAME := openapitools/openapi-generator-cli


For OpenAPI Spec validation we use the official tool, our fork is obsolete and doesn't support OpenAPI Spec 3.1.x.

pcrespov · 2024-10-23T11:24:56Z

services/api-server/openapi.json

@bisgaard-itis can you check now this openapi.json generates the client? Probably is going to force us to go to the next version of the generator

I will give it a try

matusdrobuliak66

Thanks!

bisgaard-itis

Very cool! Thanks a lot for the effort. Just a few questions, mainly for my understanding

bisgaard-itis · 2024-10-23T12:48:23Z

packages/pytest-simcore/src/pytest_simcore/helpers/httpx_calls_capture_parameters.py

+    minimum: int | float | None = None
+    anyOf: list["CapturedParameterSchema"] | None = None
+    allOf: list["CapturedParameterSchema"] | None = None
+    oneOf: list["CapturedParameterSchema"] | None = None


Out of curiosity: Why do these have default values now? I believe this is used quite extensively for our testing, so I would try to be as strict as possible in our model.

https://docs.pydantic.dev/latest/migration/#required-optional-and-nullable-fields

but still. these were thought as "required" but can be "none" and now they are "non-required". we already had that discussion if I remember well. you showed that pydantic v1 was misbehaving. but still the intent was of having them "required". I would double check with @bisgaard-itis and @pcrespov here

bisgaard-itis · 2024-10-23T12:50:33Z

services/api-server/Makefile

-OPENAPI_GENERATOR_NAME := itisfoundation/openapi-generator-cli-openapi-generator-v4.2.3
-OPENAPI_GENERATOR_TAG := v0
+OPENAPI_GENERATOR_NAME := openapitools/openapi-generator-cli
+OPENAPI_GENERATOR_TAG := latest


Would it make sense to pin the version? I believe this is used in the tests (checking compatibility of openapi specs) and not pinning the version creates a "loose end". If suddenly a bug/change is introduced in the tool we may start seeing strange compatibility failures in our ci

bisgaard-itis · 2024-10-23T12:54:51Z

...es/api-server/src/simcore_service_api_server/exceptions/handlers/_handlers_backend_errors.py

+async def backend_error_handler(request: Request, exc: Exception) -> JSONResponse:
    assert request  # nosec
+    assert isinstance(exc, BaseBackEndError)
+


Out of curiosity: What's the motivation for this change?

mypy complained about function signature assigned to app.add_exception_handler.

sanderegg

thanks!

sanderegg · 2024-10-24T06:05:02Z

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

-    workspace_id: WorkspaceID | None
-    folder_id: FolderID | None
+    permalink: ProjectPermalink | None = FieldNotRequired()
+    workspace_id: WorkspaceID | None = None


these are only for jsonschema generation right? what confuses me is when you say "optional" this does not mean with a default of None

sanderegg · 2024-10-24T06:05:47Z

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

    )

+    model_config = ConfigDict(
+        frozen=False


thanks! I guess these went off with the re-creation of the branch. how did you find that out? in the warnings of pytest??

sanderegg · 2024-10-24T06:06:02Z

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

    modified: datetime

+    model_config = ConfigDict(
+        frozen=False


same question

sanderegg · 2024-10-24T06:07:51Z

packages/pytest-simcore/src/pytest_simcore/helpers/httpx_calls_capture_parameters.py

+    minimum: int | float | None = None
+    anyOf: list["CapturedParameterSchema"] | None = None
+    allOf: list["CapturedParameterSchema"] | None = None
+    oneOf: list["CapturedParameterSchema"] | None = None


but still. these were thought as "required" but can be "none" and now they are "non-required". we already had that discussion if I remember well. you showed that pydantic v1 was misbehaving. but still the intent was of having them "required". I would double check with @bisgaard-itis and @pcrespov here

sanderegg · 2024-10-24T06:09:09Z

packages/pytest-simcore/src/pytest_simcore/helpers/httpx_calls_capture_parameters.py

-        anyOf = values.get("anyOf")
-        allOf = values.get("allOf")
-        oneOf = values.get("oneOf")
+        type_ = values.type_


this should be a root_validator or the pydantic v2 pendant instead. instead of playing with strings, you would have the self.pattern

sanderegg · 2024-10-24T06:11:06Z

services/api-server/src/simcore_service_api_server/api/routes/solvers_jobs_getters.py

            presigned_download_link,
        )
-        return RedirectResponse(presigned_download_link)
+        return RedirectResponse(f"{presigned_download_link}")


interesting that fastapi that is using pydantic v2 would not be compatible with new style URL? are you sure?

At the end it would work, because internally there the Url is stringified... but type-checking complains.

src/simcore_service_api_server/api/routes/solvers_jobs_getters.py: note: In function "get_job_output_logfile": src/simcore_service_api_server/api/routes/solvers_jobs_getters.py:337:33: error: Argument 1 to "RedirectResponse" has incompatible type "Url"; expected "str | URL" [arg-type] Found 1 error in 1 file (checked 90 source files)

class RedirectResponse( url: str | URL, # starlette.datastructures.URL status_code: int = 307, headers: Mapping[str, str] | None = None, background: BackgroundTask | None = None )

services/api-server/src/simcore_service_api_server/core/settings.py

sanderegg · 2024-10-24T06:13:26Z

services/api-server/src/simcore_service_api_server/models/schemas/jobs.py

    # NOTE: this is different from the resource JobInput (TBD)
    values: KeywordArguments

    # TODO: gibt es platz fuer metadata?


@pcrespov @bisgaard-itis German TODO... that is a good one

bisgaard-itis · 2024-10-24T09:53:56Z

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

+

 class WalletGetWithAvailableCredits(WalletGet):
    available_credits: Decimal


N.B. as discussed offline with @giancarloromeo pydantic 2 has a breaking change concerning how it deals with Decimals which will affect the behaviour of the server (pydantic/pydantic#7120 (comment)). That means once this PR gets merged users will receive, say {"my_decimal": "1.23"} instead of {"my_decimal": 1.23}. This will probably affect users who use the osparc client.

Checking for solution to make this back-compatible with the old behavior 👌 I read that using json_encoders is discouraged, because it will change...

This change solves our problems:

ad0cf2b

OpenAPI Spec file generated again using make openapi.json command.

Very cool! Thanks a lot for the quick fix

…1/upgrade-api-server-service

sonarqubecloud · 2024-10-25T09:57:07Z

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

upgrade service

aa37a25

giancarloromeo self-assigned this Oct 23, 2024

giancarloromeo added the a:apiserver api-server service label Oct 23, 2024

giancarloromeo added this to the MartinKippenberger milestone Oct 23, 2024

fix import

e085708

fix params

d32c059

giancarloromeo mentioned this pull request Oct 23, 2024

Major upgrade to pydantic 2.0 #4481

Closed

giancarloromeo added 5 commits October 23, 2024 10:33

fix optional fields

37fcf8d

fix frozen model

3d88400

fix test

7d2cb94

fix optional field

f439884

fix frozen model

7ed9993

giancarloromeo changed the title ~~WIP: ⬆️ Upgrade API Server service~~ WIP: ⬆️ Upgrade API Server service (Pydantic v2) Oct 23, 2024

giancarloromeo added the t:maintenance Some planned maintenance work label Oct 23, 2024

giancarloromeo changed the title ~~WIP: ⬆️ Upgrade API Server service (Pydantic v2)~~ ⬆️ Upgrade API Server service (Pydantic v2) Oct 23, 2024

giancarloromeo marked this pull request as ready for review October 23, 2024 09:44

giancarloromeo requested review from matusdrobuliak66, pcrespov and sanderegg as code owners October 23, 2024 09:44

giancarloromeo requested a review from GitHK October 23, 2024 09:44

giancarloromeo commented Oct 23, 2024

View reviewed changes

giancarloromeo added 3 commits October 23, 2024 12:06

fix merge

9b9b74c

fix merge

ff99950

fix deprecated

fbf6463

pcrespov requested a review from bisgaard-itis October 23, 2024 11:23

pcrespov approved these changes Oct 23, 2024

View reviewed changes

matusdrobuliak66 approved these changes Oct 23, 2024

View reviewed changes

bisgaard-itis approved these changes Oct 23, 2024

View reviewed changes

sanderegg approved these changes Oct 24, 2024

View reviewed changes

giancarloromeo added 3 commits October 24, 2024 09:33

fix double classmethod

b2742f5

revert link to str

523e530

fix type-checking

ae3e882

bisgaard-itis reviewed Oct 24, 2024

View reviewed changes

giancarloromeo added 3 commits October 24, 2024 12:38

fix available_credits serialization

ad0cf2b

Merge branch 'pydantic_v2_migration_do_not_squash_updates' into is448…

593df29

…1/upgrade-api-server-service

Merge branch 'pydantic_v2_migration_do_not_squash_updates' into is448…

fd1ae8d

…1/upgrade-api-server-service

giancarloromeo merged commit 39aed6a into ITISFoundation:pydantic_v2_migration_do_not_squash_updates Oct 25, 2024
40 of 58 checks passed

giancarloromeo deleted the is4481/upgrade-api-server-service branch November 21, 2024 09:40



		class WalletGetWithAvailableCredits(WalletGet):
		available_credits: Decimal

⬆️ Upgrade API Server service (Pydantic v2) #6580

⬆️ Upgrade API Server service (Pydantic v2) #6580

Uh oh!

Conversation

giancarloromeo commented Oct 23, 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 Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

matusdrobuliak66 left a comment

Choose a reason for hiding this comment

Uh oh!

bisgaard-itis left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

sanderegg left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

giancarloromeo commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading

giancarloromeo Oct 23, 2024 •

edited

Loading

giancarloromeo Oct 23, 2024 •

edited

Loading

giancarloromeo Oct 24, 2024 •

edited

Loading

giancarloromeo Oct 24, 2024 •

edited

Loading

giancarloromeo Oct 24, 2024 •

edited

Loading