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

Prevent panicking when __dict__ changes during iteration #1196

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Conversation

alexmojaki
Copy link
Contributor

Change Summary

Gather the items of __dict__ into a SmallVec before iterating during serialization, so that modifications to the __dict__ in the process (e.g. from a @cached_property called by a field serializer) don't cause iteration to panic.

Replaces #1093

Related issue number

FIxes pydantic/pydantic#7832

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

Copy link

codspeed-hq bot commented Feb 17, 2024

CodSpeed Performance Report

Merging #1196 will degrade performances by 16.76%

Comparing dict-size2 (e560eef) with main (cf42511)

Summary

❌ 1 regressions
✅ 147 untouched benchmarks

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

Benchmarks breakdown

Benchmark main dict-size2 Change
test_small_class_core_dict 32.2 µs 38.7 µs -16.76%

@samuelcolvin
Copy link
Member

I agree panicking isn't good, but I don't like the performance regression, and also worried about hampering future performance improvements.

@davidhewitt could we get this to raise an exception instead of panicking? Would that be a better way forward?

@samuelcolvin
Copy link
Member

Sorry, I've reviewed the issue now.

Maybe this is the right idea, then we work on FastModel to improve performance significantly.

@samuelcolvin
Copy link
Member

Oh, also the benchmark that's changed is unrelated.

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.

I think this is the right approach too.

In PyO3 upstream the initial reaction is that this sort of panic is acceptable because it is a bug in user code (in this case, the bug was here in Pydantic) so I think for now this is the path forward.

@alexmojaki alexmojaki merged commit ea443ba into main Feb 19, 2024
26 of 27 checks passed
@alexmojaki alexmojaki deleted the dict-size2 branch February 19, 2024 11:36
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.

PanicException with cached_property in field_serializer.
3 participants