Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Oct 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
aa37a25
upgrade service
giancarloromeo Oct 23, 2024
e085708
fix import
giancarloromeo Oct 23, 2024
d32c059
fix params
giancarloromeo Oct 23, 2024
37fcf8d
fix optional fields
giancarloromeo Oct 23, 2024
3d88400
fix frozen model
giancarloromeo Oct 23, 2024
7d2cb94
fix test
giancarloromeo Oct 23, 2024
f439884
fix optional field
giancarloromeo Oct 23, 2024
7ed9993
fix frozen model
giancarloromeo Oct 23, 2024
9b9b74c
fix merge
giancarloromeo Oct 23, 2024
ff99950
fix merge
giancarloromeo Oct 23, 2024
fbf6463
fix deprecated
giancarloromeo Oct 23, 2024
b2742f5
fix double classmethod
giancarloromeo Oct 24, 2024
523e530
revert link to str
giancarloromeo Oct 24, 2024
ae3e882
fix type-checking
giancarloromeo Oct 24, 2024
ad0cf2b
fix available_credits serialization
giancarloromeo Oct 24, 2024
593df29
Merge branch 'pydantic_v2_migration_do_not_squash_updates' into is448…
giancarloromeo Oct 24, 2024
fd1ae8d
Merge branch 'pydantic_v2_migration_do_not_squash_updates' into is448…
giancarloromeo Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/models-library/src/models_library/api_schemas_webserver/projects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from models_library.folders import FolderID
from models_library.workspaces import WorkspaceID
from pydantic import Field, HttpUrl, field_validator
from pydantic import ConfigDict, Field, HttpUrl, field_validator

from ..api_schemas_long_running_tasks.tasks import TaskGet
from ..basic_types import LongTruncatedStr, ShortTruncatedStr
Expand Down Expand Up @@ -78,14 +78,18 @@ class ProjectGet(OutputSchema):
ui: EmptyModel | StudyUI | None = None
quality: dict[str, Any] = {}
dev: dict | None = None
permalink: ProjectPermalink = FieldNotRequired()
workspace_id: WorkspaceID | None
folder_id: FolderID | None
permalink: ProjectPermalink | None = FieldNotRequired()
workspace_id: WorkspaceID | None = None
Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matusdrobuliak66 confirmed that workspace_id and folder_id are optional

Copy link
Member

@sanderegg sanderegg Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

folder_id: FolderID | None = None

_empty_description = field_validator("description", mode="before")(
none_to_empty_str_pre_validator
)

model_config = ConfigDict(
frozen=False
Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superclass is frozen and the behaviour was inherited.

Copy link
Member

@sanderegg sanderegg Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)


TaskProjectGet: TypeAlias = TaskGet

Expand Down
4 changes: 4 additions & 0 deletions packages/models-library/src/models_library/api_schemas_webserver/wallets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ class WalletGet(OutputSchema):
created: datetime
modified: datetime

model_config = ConfigDict(
frozen=False
Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superclass is frozen and the behaviour was inherited.

Copy link
Member

@sanderegg sanderegg Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question

Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)


class WalletGetWithAvailableCredits(WalletGet):
available_credits: Decimal
Copy link
Contributor

@bisgaard-itis bisgaard-itis Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change solves our problems:

ad0cf2b

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

Copy link
Contributor

@bisgaard-itis bisgaard-itis Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! Thanks a lot for the quick fix

Expand Down
26 changes: 13 additions & 13 deletions packages/pytest-simcore/src/pytest_simcore/helpers/httpx_calls_capture_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
class CapturedParameterSchema(BaseModel):
title: str | None = None
type_: Literal["str", "int", "float", "bool"] | None = Field(None, alias="type")
pattern: str | None
pattern: str | None = None
format_: Literal["uuid"] | None = Field(None, alias="format")
exclusiveMinimum: bool | None
minimum: int | None
anyOf: list["CapturedParameterSchema"] | None
allOf: list["CapturedParameterSchema"] | None
oneOf: list["CapturedParameterSchema"] | None
exclusiveMinimum: bool | None = None
minimum: int | float | None = None
anyOf: list["CapturedParameterSchema"] | None = None
allOf: list["CapturedParameterSchema"] | None = None
oneOf: list["CapturedParameterSchema"] | None = None
Copy link
Contributor

@bisgaard-itis bisgaard-itis Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sanderegg sanderegg Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


class Config:
validate_always = True
Expand All @@ -34,15 +34,15 @@ def preprocess_type_(cls, val):
@model_validator(mode="after")
@classmethod
def check_compatibility(cls, values):
type_ = values.get("type_")
pattern = values.get("pattern")
format_ = values.get("format_")
anyOf = values.get("anyOf")
allOf = values.get("allOf")
oneOf = values.get("oneOf")
type_ = values.type_
Copy link
Member

@sanderegg sanderegg Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pattern = values.pattern
format_ = values.format_
anyOf = values.anyOf
allOf = values.allOf
oneOf = values.oneOf
if not any([type_, oneOf, anyOf, allOf]):
type_ = "str" # this default is introduced because we have started using json query params in the webserver
values["type_"] = type_
values.type_ = type_
if type_ != "str" and any([pattern, format_]):
msg = f"For {type_=} both {pattern=} and {format_=} must be None"
raise ValueError(msg)
Expand Down
2 changes: 1 addition & 1 deletion packages/service-library/src/servicelib/logging_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
from pprint import pformat
from typing import Any, TypedDict

from common_library.errors_classes import OsparcErrorMixin
from models_library.error_codes import ErrorCodeStr
from models_library.errors_classes import OsparcErrorMixin

from .logging_utils import LogExtra, get_log_record_extra

Expand Down
8 changes: 2 additions & 6 deletions services/api-server/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ reqs: ## compiles pip requirements (.in -> .txt)


# 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
Copy link
Contributor Author

@giancarloromeo giancarloromeo Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OPENAPI_GENERATOR_TAG := latest
Copy link
Contributor

@bisgaard-itis bisgaard-itis Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OPENAPI_GENERATOR_IMAGE := $(OPENAPI_GENERATOR_NAME):$(OPENAPI_GENERATOR_TAG)

define _create_and_validate_openapi
Expand All @@ -25,10 +25,6 @@ define _create_and_validate_openapi
export API_SERVER_DEV_FEATURES_ENABLED=$1; \
python3 -c "import json; from $(APP_PACKAGE_NAME).main import *; print( json.dumps(the_app.openapi(), indent=2) )" > $@

# patching version until tools adapted
@sed -i 's/"openapi": "3.1.0",/"openapi": "3.0.2",/g' $@


# validates OAS file: $@
docker run --rm \
--volume "$(CURDIR):/local" \
Expand Down
Loading
Loading