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

⬆️♻️ Updates pydantic repo-wide to the same version and fixes new issues in all services #6882

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Dec 2, 2024

What do these changes do?

This pull request updates the pydantic version across the entire repository (see table below) to ensure consistency and prevent serialization issues. Such issues arise when one service serializes data using a newer pydantic version, and another service with an older version attempts to deserialize it. For example, we encountered the following error:

|   'exception_details': 'NEWOBJ class argument must be a type, not '
|                        '_AnnotatedAlias',
|   'exception_type': "<class '_pickle.UnpicklingError'>",

The latest version of pydantic (2.10) introduces stricter annotations, requiring code adjustments to align with these new rules. The most significant change involves handling field annotations with default or default_factory. Below is the proper way to annotate fields with these attributes in the updated pydantic version:

class Before(BaseModel):
    a: list[A] = Field(default_factory=list) # <-- fails mypy 
    b: int = Field(default=42, description="b")
    c: int = 43 
    d: str    
from common_library.basic_types import Undefined
_Unset: Any = Undefined

class After(BaseModel):
    a: Annotated[ # OK
        list[A],
        Field(
            default_factory=list,
            json_schema_extra={"default": []}, # only if exposed to web-api
        ),
    ] = _Unset

    b: Annotated[int , Field(description="b")] = 42

    c: int = 43 

    d: str


a = After(d="foo")  # <- mypy will error if `_Unset` is missing
  • _Unset as default is used to indicate mypy that this field has a default and at the same time let pydantic know that it needs run default_factory to get it. This technique is taken and adapted from https://github.com/fastapi/fastapi/blob/master/fastapi/_compat.py#L75-L78.

  • UriSchema annotation created to preserve openapi.json for api-server. There are some inconsistencies in the json-schemas generated by fastapi and pydantic that I explore in services/api-server/tests/test_utils_pydantic.py and I cannot explain. @bisgaard-itis I would appreciate some feedback here.

Related issue/s

  • Maintenance of pydantic upgrades

Upgrades

  • #packages before ~ 4
  • #packages after ~ 4
# name before after upgrade count packages
1 faststream 0.5.30, 0.5.28 0.5.31 21 agent⬆️
api-server⬆️
autoscaling⬆️
aws-library🧪
catalog⬆️
clusters-keeper⬆️
dask-sidecar⬆️
datcore-adapter⬆️
director-v2⬆️
director⬆️
dynamic-scheduler⬆️
dynamic-sidecar⬆️
efs-guardian⬆️
invitations⬆️
payments⬆️
resource-usage-tracker⬆️
service-library🧪
simcore-sdk🧪
storage⬆️
swarm-deploy🧪
web⬆️
2 pydantic 2.9.2 2.10.2 minor 36 api-server⬆️
autoscaling⬆️🧪
aws-library🧪🧪
catalog⬆️
clusters-keeper⬆️🧪
common-library🧪🧪
dask-sidecar⬆️🧪
dask-task-models-library🧪
datcore-adapter⬆️
director-v2⬆️
director⬆️
efs-guardian⬆️🧪
environment-setup🧪
models-library🧪
notifications-library🧪
osparc-gateway-server⬆️
postgres-database🧪
public-api🧪
resource-usage-tracker⬆️🧪
service-integration🧪
service-library🧪🧪
settings-library🧪
simcore-sdk🧪🧪
storage⬆️🧪
swarm-deploy🧪
web⬆️
3 pydantic-core 2.23.4 2.27.1 minor 37 api-server⬆️
autoscaling⬆️🧪
aws-library🧪🧪
catalog⬆️
clusters-keeper⬆️🧪
common-library🧪🧪
dask-sidecar⬆️🧪
dask-task-models-library🧪
datcore-adapter⬆️
director-v2⬆️
director⬆️
e2e-playwright🧪
efs-guardian⬆️🧪
environment-setup🧪
models-library🧪
notifications-library🧪
osparc-gateway-server⬆️
postgres-database🧪
public-api🧪
resource-usage-tracker⬆️🧪
service-integration🧪
service-library🧪🧪
settings-library🧪
simcore-sdk🧪🧪
storage⬆️🧪
swarm-deploy🧪
web⬆️
4 typing-extensions 4.12.0, 4.11.0, 4.10.0 4.12.2 30 api-server⬆️🧪🔧
autoscaling⬆️🧪🔧
catalog⬆️🧪🔧
clusters-keeper⬆️🧪🔧
dask-sidecar⬆️🧪🔧
datcore-adapter⬆️🧪🔧
director-v2⬆️🧪🔧
resource-usage-tracker⬆️🧪🔧
storage⬆️🧪🔧
web⬆️🧪🔧

Legend:

  • ⬆️ base dependency (only services because packages are floating)
  • 🧪 test dependency
  • 🔧 tool dependency

@pcrespov pcrespov self-assigned this Dec 2, 2024
@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 92.50000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 86.45%. Comparing base (3642829) to head (7148313).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6882      +/-   ##
==========================================
- Coverage   88.16%   86.45%   -1.72%     
==========================================
  Files        1517     1519       +2     
  Lines       59413    59144     -269     
  Branches     2095     2125      +30     
==========================================
- Hits        52384    51134    -1250     
- Misses       6698     7666     +968     
- Partials      331      344      +13     
Flag Coverage Δ
integrationtests 65.04% <80.95%> (+1.09%) ⬆️
unittests 84.36% <90.00%> (-2.06%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library 93.49% <ø> (ø)
pkg_dask_task_models_library 97.09% <ø> (ø)
pkg_models_library 91.28% <98.11%> (+0.07%) ⬆️
pkg_notifications_library 84.57% <ø> (ø)
pkg_postgres_database 87.76% <ø> (ø)
pkg_service_integration 70.02% <100.00%> (+0.02%) ⬆️
pkg_service_library 76.06% <ø> (-0.07%) ⬇️
pkg_settings_library 90.60% <85.71%> (ø)
pkg_simcore_sdk 85.38% <ø> (ø)
agent 97.00% <ø> (ø)
api_server 89.86% <100.00%> (+0.05%) ⬆️
autoscaling ∅ <ø> (∅)
catalog 90.57% <100.00%> (ø)
clusters_keeper 98.72% <ø> (ø)
dask_sidecar 91.26% <ø> (ø)
datcore_adapter 93.18% <100.00%> (+0.01%) ⬆️
director 76.40% <ø> (-0.09%) ⬇️
director_v2 91.33% <100.00%> (-0.04%) ⬇️
dynamic_scheduler 97.07% <ø> (ø)
dynamic_sidecar 89.75% <ø> (ø)
efs_guardian 90.12% <ø> (ø)
invitations 93.44% <ø> (ø)
osparc_gateway_server ∅ <ø> (∅)
payments 92.65% <50.00%> (-0.13%) ⬇️
resource_usage_tracker 88.77% <ø> (ø)
storage 89.66% <100.00%> (∅)
webclient ∅ <ø> (∅)
webserver 83.35% <80.00%> (-4.54%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3642829...7148313. Read the comment docs.

@pcrespov pcrespov force-pushed the mai/pydantic-upgrade-repo-wide branch from cdf445c to fade298 Compare December 3, 2024 09:03
@pcrespov pcrespov changed the title WIP: Mai/pydantic upgrade repo wide ⬆️♻️ Updates pydantic repo-wide to the same version and fixes new issues in all services Dec 3, 2024
@pcrespov pcrespov added this to the Event Horizon milestone Dec 3, 2024
@pcrespov
Copy link
Member Author

pcrespov commented Dec 4, 2024

I don't see why this kind of error should occur when (de)serializing. Do you maybe have a link to the logs where this happened?

@bisgaard-itis You can currently reproduce it in master and staging when you try to buy credits via billing center.

@bisgaard-itis let me know if you find a different explanation. Check with @giancarloromeo . He found the class that was causing troubles.

@pcrespov pcrespov requested review from GitHK and odeimaiz December 4, 2024 14:23
@pcrespov pcrespov enabled auto-merge (squash) December 4, 2024 14:53
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

@pcrespov pcrespov disabled auto-merge December 5, 2024 08:32
@pcrespov pcrespov merged commit 3ade7ef into ITISFoundation:master Dec 5, 2024
33 of 35 checks passed
@pcrespov pcrespov deleted the mai/pydantic-upgrade-repo-wide branch December 5, 2024 08:33
Copy link

sonarqubecloud bot commented Dec 5, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants