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

ability to pass context to serialization (pydantic#7143) #1215

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

ornariece
Copy link
Contributor

@ornariece ornariece commented Mar 4, 2024

Change Summary

As described and discussed in pydantic#7143, it would make sense to be able to pass a context object to .model_dump()/.model_dump_json() in order to dynamically update the serialization behavior during runtime.
This PR is my attempt at implementing this feature. This is my first time with Rust, so you should be wary about it :).
I tested this implementation with (an accordingly-modified) pydantic and everything seemed to work.

Related issue number

pydantic#7143
The pydantic side of the change is very light, I have a commit ready for it in case this PR gets merged.

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

Selected Reviewer: @adriangb

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Merging #1215 (617d086) into main (ab503cb) will decrease coverage by 0.36%.
Report is 28 commits behind head on main.
The diff coverage is 28.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1215      +/-   ##
==========================================
- Coverage   90.21%   89.86%   -0.36%     
==========================================
  Files         106      106              
  Lines       16339    16474     +135     
  Branches       36       40       +4     
==========================================
+ Hits        14740    14804      +64     
- Misses       1592     1651      +59     
- Partials        7       19      +12     
Files Coverage Δ
python/pydantic_core/core_schema.py 94.70% <100.00%> (-0.07%) ⬇️
src/errors/validation_exception.rs 92.87% <100.00%> (ø)
src/serializers/extra.rs 99.54% <100.00%> (+0.01%) ⬆️
src/serializers/infer.rs 95.14% <100.00%> (+0.02%) ⬆️
src/serializers/mod.rs 100.00% <100.00%> (ø)
src/validators/function.rs 94.35% <0.00%> (-3.25%) ⬇️
src/serializers/type_serializers/generator.rs 80.12% <0.00%> (-14.73%) ⬇️
src/serializers/type_serializers/function.rs 86.58% <17.64%> (-9.81%) ⬇️

... and 6 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 a98b8b0...617d086. Read the comment docs.

Copy link

codspeed-hq bot commented Mar 4, 2024

CodSpeed Performance Report

Merging #1215 will not alter performance

Comparing ornariece:serialization-context (617d086) with main (a98b8b0)

Summary

✅ 148 untouched benchmarks

@ornariece
Copy link
Contributor Author

CodSpeed Performance Report

Merging #1215 will degrade performances by 22.47%

Comparing ornariece:serialization-context (e1e6bc7) with main (a98b8b0)

Summary

❌ 2 regressions ✅ 146 untouched benchmarks

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

Benchmarks breakdown

Benchmark main ornariece:serialization-context Change
test_core_future_str 31.8 µs 39.4 µs -19.38%
test_core_future 29.2 µs 37.6 µs -22.47%

I think this is an artefact? Those tests only involve validation, and I only touched the serialization... or so I think.

@ornariece
Copy link
Contributor Author

as for the pypy3.9 test failing, i honestly have too little experience with Rust to have a clue as to why

@ornariece
Copy link
Contributor Author

please review

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks! Overall this looks great, clean implementation given first use of Rust! Hopefully you enjoyed the experience of working with Rust & PyO3 :)

I just have a few small thoughts which would nice to tidy up...

python/pydantic_core/_pydantic_core.pyi Outdated Show resolved Hide resolved
Comment on lines +491 to +492
#[pyo3(get)]
context: Option<PyObject>,
Copy link
Contributor

Choose a reason for hiding this comment

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

As context is arbitrary user contents which might end up creating a circular reference, it's probably a good idea to add a __traverse__ implementation so that the Python GC can see into these fields. (Probably we should have already done this with include / exclude, though with proper use circular references seem less likely for those.)

Would you be willing to help add this please?

https://pyo3.rs/v0.20.3/class/protocols#garbage-collector-integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

latest commit should resolve this :)

@davidhewitt
Copy link
Contributor

as for the pypy3.9 test failing, i honestly have too little experience with Rust to have a clue as to why

This is sadly just PyPy flakiness which nobody can get to the bottom of, see it all over this place in PyO3/pyo3#3766

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great, thanks very much!

@davidhewitt davidhewitt merged commit 1083986 into pydantic:main Mar 6, 2024
28 checks passed
@ornariece
Copy link
Contributor Author

awesome, thank you! i'm gonna submit a PR for pydantic now 👍

@ornariece
Copy link
Contributor Author

the PR is ready, i'll wait until the new pydantic-core release to submit it to pydantic @davidhewitt

@davidhewitt
Copy link
Contributor

It might be worth opening the PR anyway, even if it doesn't build yet, so that we can remember this needs to go into pydantic 2.7.

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

Successfully merging this pull request may close these issues.

3 participants