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

Add to_python(..., serialize_generators=False) #1401

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yorickvP
Copy link

This disables wrapping/consuming generators while serializing, while calling the fallback function on them.

In many cases, we want to implement some custom serialization logic for generators (such as io.IOBase), instead of consuming them whole.

Change Summary

Add serialize_generators parameter to to_python, to_json, etc. This changes the serialization to handle generators as if they were 'unknown' types. By default, this calls fallback or leaves them alone in to_python, and calls fallback in to_json or errors there.

Related issue number

pydantic-core part of a fix for pydantic/pydantic#8907 . If this change gets merged, we'll submit another PR to pydantic to have model_dump forward a serialize_generators argument.

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@yorickvP yorickvP force-pushed the yorickvp/serialize_generators branch from 618a1d4 to fd0e67b Compare August 12, 2024 15:12
Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.54%. Comparing base (ab503cb) to head (deaf7d2).
Report is 145 commits behind head on main.

Files Patch % Lines
src/serializers/type_serializers/generator.rs 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1401      +/-   ##
==========================================
- Coverage   90.21%   89.54%   -0.67%     
==========================================
  Files         106      109       +3     
  Lines       16339    17387    +1048     
  Branches       36       41       +5     
==========================================
+ Hits        14740    15569     +829     
- Misses       1592     1798     +206     
- Partials        7       20      +13     
Files Coverage Δ
src/errors/validation_exception.rs 92.97% <100.00%> (+0.10%) ⬆️
src/serializers/extra.rs 96.45% <100.00%> (-3.08%) ⬇️
src/serializers/infer.rs 94.13% <100.00%> (-1.00%) ⬇️
src/serializers/mod.rs 100.00% <100.00%> (ø)
src/serializers/type_serializers/generator.rs 82.53% <95.00%> (-12.32%) ⬇️

... and 40 files with indirect coverage changes


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 863640b...deaf7d2. Read the comment docs.

Copy link

codspeed-hq bot commented Aug 12, 2024

CodSpeed Performance Report

Merging #1401 will not alter performance

Comparing yorickvP:yorickvp/serialize_generators (deaf7d2) with main (39a6b10)

Summary

✅ 155 untouched benchmarks

@yorickvP yorickvP force-pushed the yorickvp/serialize_generators branch from fd0e67b to 5210b1d Compare August 12, 2024 15:22
This disables wrapping/consuming generators while serializing,
while calling the fallback function on them.

In many cases, we want to implement some custom serialization logic
for generators (such as io.IOBase), instead of consuming them whole.
@yorickvP yorickvP force-pushed the yorickvp/serialize_generators branch from 5210b1d to d073938 Compare August 12, 2024 15:27
@yorickvP yorickvP changed the title Add model_dump(..., serialize_generators=False) Add to_python(..., serialize_generators=False) Aug 13, 2024
@sydney-runkle
Copy link
Member

Huh, thanks for the PR!

Not sure if we want to add this runtime setting. Let me circle back with the team + will get back to you later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants