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

move list validation to produce an associated type #1230

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Conversation

davidhewitt
Copy link
Contributor

Change Summary

Heading towards pydantic/jiter#63 still...

The goal here is to be able to split coupling between the Input trait definition and the actual types which can get produced by certain "validate" operations. This gives more flexibility in the lifetimes, because we can completely separate the Python and JSON types. I hope it might also give perf boosts by allowing code to optimize solely for the relevant types at hand.

The experiment is to do this just for validate_list first; if this works I'll do it for all uses of GenericIterable and GenericArgs.

Related issue number

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 davidhewitt mentioned this pull request Mar 18, 2024
4 tasks
Copy link

codspeed-hq bot commented Mar 18, 2024

CodSpeed Performance Report

Merging #1230 will not alter performance

Comparing dh/input-assocs (230309d) with main (352d40f)

Summary

✅ 148 untouched benchmarks

Copy link
Member

@samuelcolvin samuelcolvin 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 I understand, and I think it looks great.

Honestly I've long thought there should be ways to re-implement iterator validation that are both more performant and more elegant, maybe this is a small step in the right direction.

@davidhewitt
Copy link
Contributor Author

Ok, so the alternative to this to support #1231 would be to make the Input trait have two lifetimes, Input<'data, 'py> and change GenericIterable to have an extra lifetime too, GenericIterable<'a, 'data, 'py>. That will pollute a lot of code with an additional lifetime which is mostly irrelevant.

I think having the localized complexity of this PR is better than polluting the whole codebase with lots of arbitrary lifetimes. So I'm going to merge this PR and then proceed to convert other relevant Input types to work in the same way.

@davidhewitt davidhewitt marked this pull request as ready for review March 19, 2024 10:14
@davidhewitt davidhewitt merged commit 89fc62f into main Mar 19, 2024
28 checks passed
@davidhewitt davidhewitt deleted the dh/input-assocs branch March 19, 2024 10:14
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