-
Notifications
You must be signed in to change notification settings - Fork 27
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
Upgrade resource usage tracker (Pydantic v2) #6517
Upgrade resource usage tracker (Pydantic v2) #6517
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pydantic_v2_migration #6517 +/- ##
=======================================================
Coverage ? 84.9%
=======================================================
Files ? 593
Lines ? 22405
Branches ? 719
=======================================================
Hits ? 19037
Misses ? 3241
Partials ? 127
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some questions about the Decimal changes.
BeforeValidator(lambda x: round(x, 2)), | ||
PlainSerializer(float, return_type=float, when_used='json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 questions here:
- why do we need to do that?
- is the BeforeValidator having the same behavior as the old Decimal ? (in which case I am wondering if the new Decimal is better? or not? @matusdrobuliak66 @pcrespov )
- PlainSerializer, is that when we output to json it creates a plain float? is this actually valid json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PlainSerializer
is used here to serialize as float
the Decimal
field that from Pydantic v2 would be returned as str
in the JSON
serialization. Without it the OpenAPI schema would have a field of type string
instead of number
, changing the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If unit tests are passing, then it should be fine (we have some tests for this Decimal checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matusdrobuliak66 I’m quite skeptical about the changes in these Decimal
... should be test it further ?:-(
services/resource-usage-tracker/src/simcore_service_resource_usage_tracker/core/application.py
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! let me know if help is needed to fix some failing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
bb61da5
into
ITISFoundation:pydantic_v2_migration
What do these changes do?
Upgrade resource usage tracker to Pydantic v2.
Related issue/s
How to test
Dev-ops checklist