8000 fix serialization of subclasses by samuelcolvin · Pull Request #860 · pydantic/pydantic-core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix serialization of subclasses #860

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

Jump to bottom
Merged
merged 6 commits into from
Aug 15, 2023
Merged

fix serialization of subclasses #860

merged 6 commits into from
Aug 15, 2023

Conversation

samuelcolvin
Copy link
Member
@samuelcolvin samuelcolvin commented Aug 7, 2023 •
edited
Loading

Change Summary

added a fallback method to look for subclasses of types we know how to serialize.

This should work for both custom __isinstancecheck__ logic, and cases where types are used as mixins, e.g. numpy.float64.

Related issue number

fix pydantic/pydantic#6963

Selected Reviewer: @davidhewitt

@samuelcolvin samuelcolvin marked this pull request as ready for review August 7, 2023 11:25
@samuelcolvin samuelcolvin mentioned this pull request Aug 7, 2023
Closed
1 task
@codecov
Copy link
codecov bot commented Aug 7, 2023 •
edited
Loading

Codecov Report

Merging #860 (946710c) into main (09f0acf) will decrease coverage by 0.13%.
The diff coverage is 72.72%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
- Coverage   93.91%   93.78%   -0.13%     
==========================================
  Files         104      104              
  Lines       15414    15485      +71     
  Branches       25       25              
==========================================
+ Hits        14476    14523      +47     
- Misses        932      956      +24     
  Partials        6        6
Files Changed Coverage Δ
src/serializers/type_serializers/dataclass.rs 90.90% <0.00%> (ø)
src/serializers/type_serializers/model.rs 95.10% <0.00%> (ø)
src/serializers/type_serializers/nullable.rs 74.46% <0.00%> (ø)
src/serializers/ob_type.rs 80.87% <70.65%> (-6.01%) ⬇️
src/serializers/infer.rs 95.73% <100.00%> (+0.03%) ⬆️
src/serializers/type_serializers/decimal.rs 93.02% <100.00%> (ø)

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 09f0acf...946710c. Read the comment docs.

@codspeed-hq
Copy link
codspeed-hq bot commented Aug 7, 2023 •
edited
Loading

CodSpeed Performance Report

Merging #860 will degrade performances by 29.42%

Comparing subclass-serialization (946710c) with main (09f0acf)

Summary

🔥 1 improvements
❌ 1 regressions
✅ 136 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main subclass-serialization Change
❌ test_complete_core_lax 1.3 ms 1.8 ms -29.42%
🔥 test_dont_raise_error 40.6 µs 29.4 µs +38.22%

@samuelcolvin
Copy link
Member Author

please review.

@samuelcolvin samuelcolvin assigned davidhewitt and unassigned dmontagu Aug 7, 2023
@adriangb
Copy link
Member
adriangb commented Aug 7, 2023

Does this apply only if the type is Any? If there is a field like x: int and someone passes in an int subclass I don't think we should serialize it without error.

@samuelcolvin
Copy link
Member Author

This should work on both Any and int or float.

Personally I think we should serialize subclasses without an error, but the Any behaviour alone will fix the original problem.

8000
@xmatthias
Copy link

If i think about this from a API / fastapi perspecitve, then the schema will want to define the expected output format (float).

If the original structure is creating a float64, or an actual float (explicitly casting to float) shouldn't really matter?

It should fail if the types are incompatible - but i don't really see that it should fail if a np.float64 is used instead of a float - as the 2 are actually compatible types.

davidhewitt
davidhewitt reviewed Aug 9, 2023
View reviewed changes
@samuelcolvin
Copy link
Member Author

sp @xmatthias are you saying this PR is right or wrong?

@xmatthias
Copy link

@samuelcolvin sorry for being imprecise
My comment refers to the following:

Does this apply only if the type is Any? If there is a field like x: int and someone passes in an int subclass I don't think we should serialize it without error.

#860 (comment)

I think it's correct to serialize int64 as int - as it's a subclass of int - and my intend for the output is to have it as an integer.
When i tested this, it did work that way (tested for float64, with models as both float and ANY).

@davidhewitt davidhewitt force-pushed the subclass-serialization branch from 6e29966 to 946710c Compare August 15, 2023 15:15
@davidhewitt
Copy link
Contributor

The integration test failure is trivial; the test in Pydantic is using is when it probably should have always just used ==.

@davidhewitt davidhewitt merged commit 26453d5 into main Aug 15, 2023
@davidhewitt davidhewitt deleted the subclass-serialization branch August 15, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt left review comments

Assignees

@dav 55CC idhewitt davidhewitt

Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

numpy's float64 is treated wrong
5 participants
@samuelcolvin @adriangb @xmatthias @davidhewitt @dmontagu
0