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

Revert "Merge pull request #3578 from davidhewitt/typed-helpers" #3794

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Feb 3, 2024

This reverts #3578, setting the return type of py.None(), py.NotImplemented(), and py.Ellipsis() back to PyObject instead of GIL Ref to their typed singletons.

I still believe that returning better typed values would be a good thing, but now with the migration to the GIL Refs API also landing in 0.21 I think that the extra churn caused by this change is not helpful right at this minute. In particular because this patch changed py.None() to return &PyNone, which is a GIL Ref API, introducing that change right now feels in contradiction with the general theme of the 0.21 release.

I propose to revert this change for now and land it in a few releases time as Borrowed<PyNone> etc as the future return types, when the rest of the dust has settled a bit.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Feb 3, 2024
@davidhewitt
Copy link
Member Author

cc @messense / @adamreichold as you were original reviewers here. As this code never made it into a release, I'm going to just proceed to merge this revert immediately to make the job cleaner for @snuderl in #3793

@davidhewitt davidhewitt enabled auto-merge February 3, 2024 21:06
@davidhewitt davidhewitt added this pull request to the merge queue Feb 3, 2024
Copy link

codspeed-hq bot commented Feb 3, 2024

CodSpeed Performance Report

Merging #3794 will degrade performances by 13.51%

Comparing davidhewitt:revert-python-none (76d1b34) with main (d8c5e79)

Summary

⚡ 1 improvements
❌ 2 regressions
✅ 76 untouched benchmarks

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

Benchmarks breakdown

Benchmark main davidhewitt:revert-python-none Change
list_via_downcast 185 ns 157.2 ns +17.67%
not_a_list_via_downcast 215 ns 242.8 ns -11.44%
f64_from_pyobject 711.1 ns 822.2 ns -13.51%

Merged via the queue into PyO3:main with commit 975f182 Feb 3, 2024
31 of 40 checks passed
@davidhewitt davidhewitt deleted the revert-python-none branch February 3, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant