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

simplify ser-as-any mechanism #1478

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

simplify ser-as-any mechanism #1478

wants to merge 10 commits into from

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Oct 7, 2024

Change Summary

Built atop #1359

The concern I had with that PR is that the "fix" seemed to be a strange special case, which to me implied that there was probably a simplification which would decouple the problem.

Indeed, in this PR I instead propose that when serialize_as_any=True then the schema is immediately dropped in favour of an AnySerializer, and the extra records serialize_as_any so that if any serializers are re-entered (via inference) then they immediately discard their schema too.

Related issue number

Fix pydantic/pydantic#9670

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

@davidhewitt
Copy link
Contributor Author

Hmm, looks like this needs another pass. Will investigate later.

@davidhewitt davidhewitt marked this pull request as draft October 7, 2024 12:20
Copy link

codspeed-hq bot commented Oct 7, 2024

CodSpeed Performance Report

Merging #1478 will not alter performance

Comparing dh/simpler-ser-as-any (c58b54f) with main (dc4846e)

Summary

✅ 155 untouched benchmarks

@davidhewitt
Copy link
Contributor Author

This now works, and it also fixes the bug, but I'm not sure it's really any simpler. To make things hang together I had to rework all serializer calls inside fields.rs to dispatch on serialize_as_any. At least it means that the logic is more concentrated than it was before, which helps avoid the original recursive-ref issue.

@sydney-runkle happy to take your call on whether this PR is a good idea.

(The pydantic-integration test probably needs running, so this might want to wait until we've dealt with the invalid-schema changes.)

@davidhewitt davidhewitt marked this pull request as ready for review October 8, 2024 09:11
@sydney-runkle
Copy link
Member

(The pydantic-integration test probably needs running, so this might want to wait until we've dealt with the invalid-schema changes.)

I think we could probably just do a patch release with that change + this one to make CI happy :).

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

This LGTM, definitely feels like a simplification here and is much easier to understand if we just use the boolean pattern 👍

src/serializers/type_serializers/dataclass.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Contributor Author

Maybe merging #1480 will give us all we need to measure here.

@davidhewitt
Copy link
Contributor Author

Ungh, will investigate the test failures when I get a chance.

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