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

Update Pyo3 bounds #472

Merged
merged 5 commits into from
Jun 8, 2024
Merged

Update Pyo3 bounds #472

merged 5 commits into from
Jun 8, 2024

Conversation

Michael-J-Ward
Copy link
Contributor

@Michael-J-Ward Michael-J-Ward commented Apr 16, 2024

pyo3 0.21 is in arrow-rs master (see apache/arrow-rs#5566), but not yet released.

Notes

After updating the deps, this was completely a compiler / clippy driven refactor where clippy highlights deprecation warnings, and I would update using the migration guide.

I did not go looking for any other opportunities to use the new Bounds api.

Copy link
Collaborator

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot! Let's hope that the arrow project publishes a new release soon.

@Michael-J-Ward
Copy link
Contributor Author

@haixuanTao the last deprecation warning was here.

// We need to create a new scoped `GILPool` because the dora-runtime
// is currently started through a `start_runtime` wrapper function,
// which is annotated with `#[pyfunction]`. This attribute creates an
// initial `GILPool` that lasts for the entire lifetime of the `dora-runtime`.
// However, we want the `PyBytes` created below to be freed earlier.
// creating a new scoped `GILPool` tied to this closure, will free `PyBytes`
// at the end of the closure.
// See https://github.com/PyO3/pyo3/pull/2864 and
// https://github.com/PyO3/pyo3/issues/2853 for more details.
let pool = unsafe { py.new_pool() };
let py = pool.python();

I think that the new bounds API makes this unnecessary, and the deprecation warning states "code not using the GIL Refs API can safely remove the use of Py::new_pool".

I included that change as the last commit, so if you find that it is still necessary it'll be an easy rollback.

@haixuanTao
Copy link
Collaborator

@haixuanTao the last deprecation warning was here.

// We need to create a new scoped `GILPool` because the dora-runtime
// is currently started through a `start_runtime` wrapper function,
// which is annotated with `#[pyfunction]`. This attribute creates an
// initial `GILPool` that lasts for the entire lifetime of the `dora-runtime`.
// However, we want the `PyBytes` created below to be freed earlier.
// creating a new scoped `GILPool` tied to this closure, will free `PyBytes`
// at the end of the closure.
// See https://github.com/PyO3/pyo3/pull/2864 and
// https://github.com/PyO3/pyo3/issues/2853 for more details.
let pool = unsafe { py.new_pool() };
let py = pool.python();

I think that the new bounds API makes this unnecessary, and the deprecation warning states "code not using the GIL Refs API can safely remove the use of Py::new_pool".

I included that change as the last commit, so if you find that it is still necessary it'll be an easy rollback.

That makes sense, thanks!

@haixuanTao
Copy link
Collaborator

This fix an issue that I had about stopping a dataflow that has been present for a bit!

Thanks a lot!

Can't wait to merge it.

@Michael-J-Ward
Copy link
Contributor Author

@haixuanTao - It could be a few months before arrow-rs gets released.

If you don't mind pointing to arrow-rs's master, then I'll rebase this so you can merge now, and I'll update the deps once it does get released.

@haixuanTao
Copy link
Collaborator

If we merge it, we will not be able to publish on cargo, which will makes our release stuck. So I wouldn´t do it.

What we could do, is to only merge arrow version within dora-node-api-python that is distributed using pip which does not check for git packages and distribute dora-node-api-python with the latest arrow.

But not sure if we can handle multi arrow versions.

Arrow needs to be upgraded alongside pyo3 because they both link to python.

NOTE: Arrow marked `ffi::from_ffi` unsafe.
apache/arrow-rs#5080
The deprecation warning states:

> code not using the `GIL Refs` API can safely remove the use of `Py::new_pool`

All GIL Refs usage have been removed, so this should be fine.
@Michael-J-Ward Michael-J-Ward marked this pull request as ready for review June 7, 2024 19:29
@Michael-J-Ward Michael-J-Ward changed the title (draft): Pyo3 bounds Update Pyo3 bounds Jun 7, 2024
@Michael-J-Ward
Copy link
Contributor Author

@haixuanTao This is rebased and ready for review.

Copy link
Collaborator

@haixuanTao haixuanTao left a comment

Choose a reason for hiding this comment

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

Looks awesome!

Thanks a million!

@haixuanTao haixuanTao merged commit d7be6a4 into dora-rs:main Jun 8, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use new Bound API of pyO3
3 participants