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

refactor: Split py-polars crate #18204

Merged
merged 15 commits into from
Aug 16, 2024
Merged

refactor: Split py-polars crate #18204

merged 15 commits into from
Aug 16, 2024

Conversation

stinodego
Copy link
Member

This PR splits the py-polars crate into two crates:

  • A 'regular' crate with re-usable functionality
  • A cdylib crate for exporting the dynamic library which enables Python Polars

The motivation behind this split is that we will need to re-use some of the Python functionality for use on Polars Cloud workers, e.g. executing Python UDFs.

In practice, almost all functionality has moved to the 'regular' crate under crates/polars-python. The py-polars crate is just the #[pymodule] definition. We initially wanted to define the classes (e.g. PyDataFrame) in the reusable crate and implement their methods in the library crate, but this does not seem possible due to how the PyO3 proc macro interacts with the Rust orphan rule.

So we're just going to have pretty much everything under crates/polars-python.

Open question:

  • Where should the allocator logic be? It's currently in the crates/polars-python crate but I'm not sure that will work correctly now for the Python Polars wheels.

@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars labels Aug 15, 2024
@ritchie46
Copy link
Member

Where should the allocator logic be? It's currently in the crates/polars-python crate but I'm not sure that will work correctly now for the Python Polars wheels.

In the cdylib crate as that one is the binary that is compiled. The other crate is a library for other rust crates and libraries should not determine which allocator is used for the consumers.

@ritchie46
Copy link
Member

So we're just going to have pretty much everything under crates/polars-python.

Can we put them in a separate folder and feature gate them? As a consumer of the regular crate I am not interested in compiling the pymethods.

Copy link

codspeed-hq bot commented Aug 15, 2024

CodSpeed Performance Report

Merging #18204 will degrade performances by 31.95%

Comparing python-crate-part (7f71c77) with main (f6b19ad)

Summary

❌ 1 regressions
✅ 36 untouched benchmarks

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

Benchmarks breakdown

Benchmark main python-crate-part Change
test_to_numpy_series_with_nulls 300.7 µs 441.9 µs -31.95%

@stinodego
Copy link
Member Author

So we're just going to have pretty much everything under crates/polars-python.

Can we put them in a separate folder and feature gate them? As a consumer of the regular crate I am not interested in compiling the pymethods.

Will do!

@stinodego
Copy link
Member Author

stinodego commented Aug 15, 2024

@ritchie46 Updated to keep the allocator logic in py-polars. Feature flagging the pymethods I'm doing in a followup PR - it's a bit finnicky and requires some more refactoring I'd rather do separately.

CI failures unrelated (see #18211)

@ritchie46
Copy link
Member

Ai.. conflicts are already happening. :( Can you rebase?

Copy link

codecov bot commented Aug 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.27%. Comparing base (bd328a0) to head (7f71c77).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18204      +/-   ##
==========================================
- Coverage   80.29%   80.27%   -0.02%     
==========================================
  Files        1497     1499       +2     
  Lines      198741   198741              
  Branches     2837     2837              
==========================================
- Hits       159570   159549      -21     
- Misses      38644    38665      +21     
  Partials      527      527              

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

@stinodego
Copy link
Member Author

@ritchie46 Done! I also have the feature gates done, will make a PR after this one.

@ritchie46
Copy link
Member

@ritchie46 Done! I also have the feature gates done, will make a PR after this one.

Nice! Then I can remove a lot of code in pyo3-polars. 😎

@ritchie46 ritchie46 merged commit d8b617e into main Aug 16, 2024
26 checks passed
@ritchie46 ritchie46 deleted the python-crate-part branch August 16, 2024 09:19
@c-peters c-peters added the accepted Ready for implementation label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants