Skip to content

Conversation

sydney-runkle
Copy link
Contributor

@sydney-runkle sydney-runkle commented Jul 25, 2024

Along the same lines as #9975 and #9976.

I've marked one test as xfail - we're no longer checking for the appropriate decimal constraints here, but this isn't a reflection of a problem with decimal, but rather our known annotated metadata application process. I've explained this reason in the xfail comment, and plan to write up a report discussing how we apply annotated metadata and the current shortcomings.

Upon codspeed completion, I'm seeing a 7% improvement on this function: https://codspeed.io/pydantic/pydantic/branches/move-dec. We're slowly chipping away!

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Jul 25, 2024
@sydney-runkle sydney-runkle added relnotes-performance Used for performance improvements. and removed relnotes-fix Used for bugfixes. labels Jul 25, 2024
@sydney-runkle sydney-runkle requested a review from adriangb July 25, 2024 17:08
Copy link

codspeed-hq bot commented Jul 25, 2024

CodSpeed Performance Report

Merging #9977 will not alter performance

Comparing move-dec (b3eb92e) with main (90e263f)

Summary

✅ 14 untouched benchmarks

Copy link
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pydantic
  config.py
  pydantic/_internal
  _generate_schema.py
Project Total  

This report was generated by python-coverage-comment-action

Comment on lines +1809 to +1815
@pytest.mark.xfail(
reason=(
"We allow python validation functions to wrap the type if the constraint isn't valid for the type. "
"An error isn't raised for an int or float annotated with this same invalid constraint, "
"so it's ok to mark this as xfail for now, but we should improve all of them in the future."
)
)
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I do think we could special case something like Annotated[float, Field(max_len=10)] to raise an error at schema building time. If we're 100% sure the constraint makes no sense we can raise an error. But in the case of Annoated[float, AfterValidator(...), Field(max_len=10)] or whatever that we can't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think that should be addressed in a different PR though 👍

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

Successfully merging this pull request may close these issues.

2 participants