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.

Sign up for GitHub

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

Jump to bottom

chore(python): More PyO3 0.21 Bound<> APIs, and finally disable gil-refs backwards compat feature on pyo3 crate #16143

Merged
merged 9 commits into from May 14, 2024
Merged

chore(python): More PyO3 0.21 Bound<> APIs, and finally disable gil-refs backwards compat feature on pyo3 crate #16143

merged 9 commits into from May 14, 2024
+116 −124

Conversation

itamarst
Copy link
Contributor

@itamarst itamarst commented May 9, 2024

Fixes #16142

Note that I am not convinced we've caught all usages of the old reference-style code (&PyAny and friends instead of Bound<PyAny>). Not everything is quite deprecated yet, is my impression. I have supplemented explicit warnings with some grepping, but still. But, presumably this will get caught as newer PyO3 releases come out.

@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars labels May 9, 2024
@itamarst itamarst marked this pull request as ready for review May 9, 2024 21:13
@codecov Codecov
Copy link

codecov bot commented May 9, 2024 •
edited

Codecov Report

Attention: Patch coverage is 86.25000% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 81.00%. Comparing base (c4e8678) to head (95dbf53).
Report is 33 commits behind head on main.

Files Patch % Lines
py-polars/src/map/dataframe.rs 0.00% 5 Missing ⚠️
py-polars/src/map/lazy.rs 66.66% 3 Missing ⚠️
py-polars/src/conversion/mod.rs 95.55% 2 Missing ⚠️
py-polars/src/datatypes.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #16143      +/-   ##
==========================================
+ Coverage   80.96%   81.00%   +0.04%     
==========================================
  Files        1386     1392       +6     
  Lines      178437   178916     +479     
  Branches     2882     2901      +19     
==========================================
+ Hits       144463   144923     +460     
- Misses      33480    33488       +8     
- Partials      494      505      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ritchie46
Copy link
Member

I have supplemented explicit warnings with some grepping, but still. But, presumably this will get caught as newer PyO3 releases come out.

Isn't there a feature flag that prohibits the old style usage?

@itamarst
Copy link
Contributor Author

Docs just say "As a final step of migration, deactivating the gil-refs feature will set up code for best performance and is intended to set up a forward-compatible API for PyO3 0.22." and there's nothing at https://docs.rs/crate/pyo3/latest/features. So no extra flag we can set.

However, most even if not all usages of the old API will have a deprecation warning message now that gil-refs feature is disabled.

@itamarst
Copy link
Contributor Author

@ritchie46 back to you.

@ritchie46
Copy link
Member

Alright! thanks for this one @itamarst. Great to be up to date and hopefully we'll have some perf benefits out of it.

It is at least much clearer where the cost is now, so once we can drop py38, we'll do another pass.

@ritchie46 ritchie46 merged commit 5b77f01 into pola-rs:main May 14, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritchie46 ritchie46 Awaiting requested review from ritchie46 ritchie46 is a code owner

@stinodego stinodego Awaiting requested review from stinodego stinodego is a code owner

@c-peters c-peters Awaiting requested review from c-peters c-peters is a code owner

@alexander-beedie alexander-beedie Awaiting requested review from alexander-beedie alexander-beedie is a code owner

@MarcoGorelli MarcoGorelli Awaiting requested review from MarcoGorelli MarcoGorelli is a code owner

@reswqa reswqa Awaiting requested review from reswqa reswqa is a code owner

@orlp orlp Awaiting requested review from orlp orlp is a code owner

Assignees
No one assigned
Labels
internal An internal refactor or improvement python Related to Python Polars
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Continue PyO3 0.21 Bound<> updates, and remove "gil-refs" pyo3 crate feature
3 participants
@itamarst @ritchie46 @pythonspeed