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

Use from __future__ import annotations instead of string type hints in python/pydantic_core/_pydantic_core.pyi #710

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Jun 28, 2023

Selected Reviewer: @davidhewitt

@adriangb
Copy link
Member Author

please review

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 28, 2023

CodSpeed Performance Report

Merging #710 unstringify-stubs (8c7f0be) will improve performances by 11.61%.

Summary

🔥 1 improvements
❌ 0 regressions
✅ 124 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

Benchmarks breakdown

Benchmark main unstringify-stubs Change
🔥 test_set_of_ints_core_json_duplicates 5.4 ms 4.8 ms +11.61%

@adriangb
Copy link
Member Author

@samuelcolvin @davidhewitt something seems to be wrong with the benchmarks now, this PR should not be changing any performance. I've rebased it onto 703b7b2 so it shouldn't be a base issue.

@davidhewitt
Copy link
Contributor

This particular benchmark has been flipping up and down for a while. I'm not sure of the exact reason for it, whether the optimizer or LTO is introducing some randomness or codspeed's measurement has some volatility.

@davidhewitt
Copy link
Contributor

One thought I have is that using PGO i.e. #678 might help the optimizer always hit the good code path and thus avoid the randomness.

@davidhewitt
Copy link
Contributor

The lint error should be fixed by #711

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 double-checked the docs build after this change, looks good 👍

@adriangb adriangb enabled auto-merge (squash) June 28, 2023 19:12
@adriangb
Copy link
Member Author

Seems like your approval won’t cut it, I guess we need to add you to CODEOWNERS or something

@adriangb
Copy link
Member Author

Nope it was the lint error

… in `python/pydantic_core/_pydantic_core.pyi`
@davidhewitt
Copy link
Contributor

Rebased, let's see if it goes in now.

@codecov-commenter
Copy link

Codecov Report

Merging #710 (8c7f0be) into main (54daab7) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #710   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files          99       99           
  Lines       13838    13838           
  Branches       25       25           
=======================================
  Hits        12968    12968           
  Misses        864      864           
  Partials        6        6           

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 54daab7...8c7f0be. Read the comment docs.

@adriangb adriangb merged commit 081cf31 into main Jun 28, 2023
@adriangb adriangb deleted the unstringify-stubs branch June 28, 2023 19:51
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