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

bindings/python: Bump PyO3 to 0.21 #4399

Closed
Xuanwo opened this issue Mar 26, 2024 · 13 comments · Fixed by #4734
Closed

bindings/python: Bump PyO3 to 0.21 #4399

Xuanwo opened this issue Mar 26, 2024 · 13 comments · Fixed by #4734
Labels
bindings/python good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Xuanwo
Copy link
Member

Xuanwo commented Mar 26, 2024

PyO3 has been released, we can bump pyo3 to 0.21 now.

Update pyo3 in https://github.com/apache/opendal/blob/main/bindings/python/Cargo.toml and address possible build failure and warnings.

@Xuanwo Xuanwo added good first issue Good for newcomers help wanted Extra attention is needed bindings/python labels Mar 26, 2024
@reswqa
Copy link
Member

reswqa commented Mar 27, 2024

I'd like to take a look.

@reswqa
Copy link
Member

reswqa commented Mar 27, 2024

error: failed to select a version for `pyo3-ffi`.
    ... required by package `pyo3 v0.20.0`
    ... which satisfies dependency `pyo3 = "^0.20"` of package `pyo3-asyncio v0.20.0`
    ... which satisfies dependency `pyo3-asyncio = "^0.20"` of package `opendal-python v0.45.1 (xxx/opendal/bindings/python)`
versions that meet the requirements `=0.20.0` are: 0.20.0

the package `pyo3-ffi` links to the native library `python`, but it conflicts with a previous package which links to `python` as well:
package `pyo3-ffi v0.21.0`
    ... which satisfies dependency `pyo3-ffi = "=0.21.0"` of package `pyo3 v0.21.0`
    ... which satisfies dependency `pyo3 = "^0.21.0"` of package `opendal-python v0.45.1 (xxx/opendal/bindings/python)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the `links = "python"` value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

failed to select a version for `pyo3-ffi` which could resolve this conflict

Get an error if update pyo3 to 0.21. It seems that pyo3-asyncio needs to bump to 0.21 first.

@Xuanwo
Copy link
Member Author

Xuanwo commented Mar 27, 2024

Seems we can write async fn directly in pyo3 now: https://pyo3.rs/v0.21.0/async-await

Would you like to give it a try? I'm guessing we can remove pyo3-asyncio entirely.

@reswqa
Copy link
Member

reswqa commented Mar 27, 2024

Would you like to give it a try? I'm guessing we can remove pyo3-asyncio entirely.

Yep, let me take a look. 😉

@reswqa
Copy link
Member

reswqa commented Mar 30, 2024

Based on the previous discussion, I took the following action:

  1. Open an issue in pyo3-asyncio, but I'm not sure how long it will take to get a response.
  1. Try remove pyo3-asyncio, use the async function provided by PyO3 0.21.0 itself.

Do we wait for the next release or just depend on the commit included this patch?

@oowl
Copy link
Member

oowl commented Mar 30, 2024

Based on the previous discussion, I took the following action:

1. Open an issue in `pyo3-asyncio`, but I'm not sure how long it will take to get a response.


* [Bump PyO3 to 0.21.0? awestlake87/pyo3-asyncio#119](https://github.com/awestlake87/pyo3-asyncio/issues/119)


2. Try remove `pyo3-asyncio`, use the async function provided by `PyO3` 0.21.0 itself.


* It seems that `PyO3` has a bug that prevents async `pymethod` from introducing additional parameters once it has a receiver(i.e. &self/&mut self), even though they are safe enough in the asynchronous context.

* I have create an issue and open a pull request to fix this, and It was quickly merged: [Async `pymethod` with receiver can not pass any other arguments PyO3/pyo3#4005](https://github.com/PyO3/pyo3/issues/4005)

Do we wait for the next release or just depend on the commit included this patch?

I think we need to wait for the next release. From the PyO3 release history timeline, we do not need to wait for the long time.

@oowl
Copy link
Member

oowl commented Apr 3, 2024

@reswqa Good news, https://github.com/PyO3/pyo3/releases/tag/v0.21.1 pyo3 minor release includes your change!

@reswqa
Copy link
Member

reswqa commented Apr 3, 2024

Yes, but my implementation seems to have some problems (which have been fixed), and 0.21.1 also has a serious blocker that won't compile in some case, 0.21.2 should be released soon, we can upgrade to it directly then.

@Xuanwo
Copy link
Member Author

Xuanwo commented Apr 23, 2024

0.21.2 has been released now. @reswqa would you like to take another look?

@reswqa
Copy link
Member

reswqa commented Apr 23, 2024

@Xuanwo Yes, but I'm getting married and will on my honeymoon recently though 💒 :) so I can't put in the time right away. 😉

NB. If others are interested, feel free to take over this. If no one beats me, I'll keep doing it when I come back.

@Xuanwo
Copy link
Member Author

Xuanwo commented Apr 23, 2024

Yes, but I'm getting married and will on my honeymoon recently though 💒

Wow, congrats! Please enjoy your time 💌

so I can't put in the time right away. 😉

Never mind, there is no hurry.

@Xuanwo
Copy link
Member Author

Xuanwo commented May 17, 2024

I tried to bump to 0.21 but met many issues...

I pushed my local changes to https://github.com/apache/opendal/tree/bump-to-0.21

error: future cannot be sent between threads safely
   --> src/file.rs:311:1
    |
311 | #[pymethods]
    | ^^^^^^^^^^^^ future created by async block is not `Send`
    |
    = help: within `pyo3::PyAny`, the trait `Sync` is not implemented for `UnsafeCell<pyo3::ffi::PyObject>`, which is required by `{async block@src/file.rs:311:1: 311:13}: std::marker::Send`
note: captured value is not `Send` because `&` references cannot be sent unless their referent is `Sync`
   --> src/file.rs:311:1
    |
311 | #[pymethods]
    | ^^^^^^^^^^^^ has type `&pyo3::PyAny` which is not `Send`, because `pyo3::PyAny` is not `Sync`
note: required by a bound in `new_coroutine`
   --> /home/xuanwo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pyo3-0.21.2/src/impl_/coroutine.rs:22:40
    |
15  | pub fn new_coroutine<F, T, E>(
    |        ------------- required by a bound in this function
...
22  |     F: Future<Output = Result<T, E>> + Send + 'static,
    |                                        ^^^^ required by this bound in `new_coroutine`
    = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `impl futures::Future<Output = Result<bool, pyo3::PyErr>>: IntoPyCallbackOutput<_>` is not satisfied
   --> src/file.rs:311:1
    |
311 | #[pymethods]
    | ^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<_>` is not implemented for `impl futures::Future<Output = Result<bool, pyo3::PyErr>>`
    |
    = help: the following other types implement trait `IntoPyCallbackOutput<Target>`:
              <bool as IntoPyCallbackOutput<bool>>
              <bool as IntoPyCallbackOutput<i32>>
              <usize as IntoPyCallbackOutput<isize>>
              <usize as IntoPyCallbackOutput<usize>>
              <HashCallbackOutput as IntoPyCallbackOutput<isize>>
              <IterNextOutput<T, U> as IntoPyCallbackOutput<*mut pyo3::ffi::PyObject>>
              <IterANextOutput<T, U> as IntoPyCallbackOutput<*mut pyo3::ffi::PyObject>>
              <Result<T, E> as IntoPyCallbackOutput<U>>
            and 3 others
note: required by a bound in `pyo3::callback::convert`
   --> /home/xuanwo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pyo3-0.21.2/src/callback.rs:170:8
    |
168 | pub fn convert<T, U>(py: Python<'_>, value: T) -> PyResult<U>
    |        ------- required by a bound in this function
169 | where
170 |     T: IntoPyCallbackOutput<U>,
    |        ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `convert`
    = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

@reswqa
Copy link
Member

reswqa commented Jun 14, 2024

Hi @Xuanwo. Yes, I feel like the pyo3's bulit-in supports for async-io is not yet mature. I have filed a PR that still use pyo3-asyncio to bump pyo3 to 0.21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings/python good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants