__richcmp__ API docs could be expanded

[Eric-Arellano]

Thanks for PyO3! This is an awesome project, I talked about you all in my Pycon US talk last week https://us.pycon.org/2021/schedule/presentation/17/. I'm starting to look into porting Pants (https://github.com/pantsbuild/pants) from rust-cpython to PyO3.

I'd be happy to fix this issue if you'd like.

🐛 Bug Report

Using CompareOp in fn __richcmp__ results in this error:

❯ cargo check
    Checking pyo3-richcmp v0.1.0 (/Users/eric/code/debug/pyo3-richcmp)
error[E0277]: the trait bound `Demo: Clone` is not satisfied
 --> src/lib.rs:9:1
  |
9 | #[pymethods]
  | ^^^^^^^^^^^^ the trait `Clone` is not implemented for `Demo`
  |
  = note: required because of the requirements on the impl of `pyo3::FromPyObject<'_>` for `Demo`
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `CompareOp: PyClass` is not satisfied
 --> src/lib.rs:9:1
  |
9 | #[pymethods]
  | ^^^^^^^^^^^^ the trait `PyClass` is not implemented for `CompareOp`
  |
  = note: required because of the requirements on the impl of `pyo3::FromPyObject<'_>` for `CompareOp`
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `CompareOp: Clone` is not satisfied
 --> src/lib.rs:9:1
  |
9 | #[pymethods]
  | ^^^^^^^^^^^^ the trait `Clone` is not implemented for `CompareOp`
  |
  = note: required because of the requirements on the impl of `pyo3::FromPyObject<'_>` for `CompareOp`
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

It's plausible I'm writing the code wrong - if so, I'd be happy to contribute a docs change to make more clear how to implement __richcmp__.

Somewhat related, I don't see how to return NotImplemented from __richcmp__ - I'd be happy to contribute a docs change for that!

🌍 Environment

• Your operating system and version: macOS 11.0.1, M1 / Apple Silicon
• Your python version: 3.9.1
• How did you install python (e.g. apt or pyenv)? Did you use a virtualenv?: Pyenv, no virtualenv for this repro
• Your Rust version (rustc --version): 1.49.0
• Your PyO3 version: 0.13.2
• Have you tried using latest PyO3 main (replace version = "0.x.y" with git = "https://github.com/PyO3/pyo3")?: Yes, but it fails because of the rename from master to main:

error: failed to get `pyo3` as a dependency of package `pyo3-richcmp v0.1.0 (/Users/eric/code/debug/pyo3-richcmp)`

Caused by:
  failed to load source for dependency `pyo3`

Caused by:
  Unable to update https://github.com/PyO3/pyo3

Caused by:
  failed to find branch `master`

Caused by:
  cannot locate remote-tracking branch 'origin/master'; class=Reference (4); code=NotFound (-3)

💥 Reproducing

Minimal repro created at https://github.com/Eric-Arellano/pyo3-richcmp. Run cargo check.

[mejrs]

Hey 👋

This must be implemented on the PyObjectProtocol trait - not in a #[pymethods] block.

Instead, do this:

use pyo3::prelude::*;
use pyo3::basic::CompareOp;
use pyo3::class::basic::PyObjectProtocol;

#[pyclass]
#[derive(Clone)]
struct Demo {
    x: usize,
}

#[pyproto]
impl PyObjectProtocol for Demo {
    fn __richcmp__(&self, other: Demo, op: CompareOp) -> bool {
        true
    }
}

You also cannot use Demo as an argument as you do now - either derive Clone for it or use PyRef<Demo> (or similar, depending on what you want to do).

Finally; this protocol as traits stuff is a noob trap that everyone seems to fall for - which is why the plan is to do away with it.

[mejrs]

Also regarding returning NotImplemented, None..etc - you need Python::NotImplemented.

You can get a Python<'py> in a few ways:

• just put it as a function argument (which doesn't work for this - another reason for getting rid of these traits)
• explicitly acquire the gil
• get it from something else that has a gil lifetime

Using the last of those:

#[pyproto]
impl PyObjectProtocol for Demo {
    fn __richcmp__(&self, other: PyRef<Demo>, op: CompareOp) -> Py<PyAny> {	
        other.py().NotImplemented()
    }
}

[davidhewitt]

I talked about you all in my Pycon US talk last week https://us.pycon.org/2021/schedule/presentation/17/. I'm starting to look into porting Pants (https://github.com/pantsbuild/pants) from rust-cpython to PyO3.

👋 !

Very cool to see you're interested in PyO3. Thanks for sharing the word about us. I've been aware of pants for a while, I'd been wondering about using it for a large monorepo (30+ libraries + microservices) at work. We couldn't make use of it at the time but I'm keeping an eye on it and going to have another think next time I evaluate our build system.

The information by @mejrs is all correct, I just thought I'd add that if you don't want to implement Demo: Clone you can always accept &Demo arguments, e.g. the below should also work fine:

#[pyproto]
impl PyObjectProtocol for Demo {
    fn __richcmp__(&self, other: &Demo, op: CompareOp) -> Py<PyAny> {	
        other.py().NotImplemented()
    }
}

Please let me know if there's any features or information you need to evaluate porting pants to PyO3. Very happy to help.

[Eric-Arellano]

Ah, excellent. Thank you both!

Thanks for sharing the word about us.

You're welcome! It was exciting that the talk was really well received! The link will be on YouTube in a few weeks, but slides are here (in Spanish): https://speakerdeck.com/ericarellano/cuando-usar-extensiones-nativas-en-rust-rendimiento-accesible-y-seguro

I think Rust + Python is a phenomenal intersection and I appreciate all the work you do to build that ecosystem.

I've been aware of pants for a while, I'd been wondering about using it for a large monorepo

Ah, cool! FYI we released Pants 2 last year, which is a ground-up redesign after 10 years of experience with Pants 1: https://blog.pantsbuild.org/introducing-pants-v2/. (One of those changes is using Rust and Tokio for the scheduler.)

Please let me know if there's any features or information you need to evaluate porting pants to PyO3. Very happy to help.

Thank you! I am quite happy with this first proof of concept: pantsbuild/pants#12110. Among other benefits of PyO3, it's great to have IDE autocompletion and rustfmt working again on our FFI code :D

Likewise, if you would like, I'd be happy to contribute back to your project based on my experience migrating, e.g. tweaking the docs for __richcmp__.

[davidhewitt]

I think Rust + Python is a phenomenal intersection

Heh, curiously I feel the same way 😉

Likewise, if you would like, I'd be happy to contribute back to your project based on my experience migrating, e.g. tweaking the docs for __richcmp__.

Docs contributions are always very much appreciated! Because we plan to migrate #[pyproto] to #[pymethods] (hopefully in PyO3 0.15) don't spend your time writing too much for this particular one. I imagine we might need to write a lot of documentation once that change lands.

[Eric-Arellano]

Ah, a couple weeks later...I'm rereading https://pyo3.rs/v0.14.1/class/protocols.html now. I'm willing to contribute a docs improvement to that page. I'm thinking channge the bullet approach into instead subheaders. Then, there's more room to expand each section, like including code examples and how to use NotImplemented.

Because we plan to migrate #[pyproto] to #[pymethods] (hopefully in PyO3 0.15) don't spend your time writing too much for this particular one.

Would that be helpful given this?

I'm also interested in trying to port one or two of these to #[pymethods], especially if you have the time to write a starter ticket for how to help out :)

[Eric-Arellano]

Also for posterity, this is how I got this working:

#[pyclass]
struct PyDigest(Digest);

#[pyproto]
impl PyObjectProtocol for PyDigest {
  fn __richcmp__(&self, other: PyRef<PyDigest>, op: CompareOp) -> Py<PyAny> {
    let py = other.py();
    match op {
      CompareOp::Eq => (self.0 == other.0).into_py(py),
      CompareOp::Ne => (self.0 != other.0).into_py(py),
      _ => py.NotImplemented(),
    }
  }
}

Thanks @davidhewitt and @mejrs!

[davidhewitt]

Would that be helpful given this?

I'm also interested in trying to port one or two of these to #[pymethods], especially if you have the time to write a starter ticket for how to help out :)

The offer is really appreciated, I'll try and figure out a direction for the port within in the next few weeks and write a plan. It's either going to turn out to be really straightforward, or really hard ;)

[Eric-Arellano]

Sounds good! In the meantime, I'll try to contribute this weekend some improvements to that guide's page. Will helpfully help out other users over the next few weeks + maybe help with API design (i.e. docs driven development)

Feel free to ping me for feedback on anything! Happy to weigh in on different designs.

[davidhewitt]

I think this is now covered as part of #1884

[Eric-Arellano]

Just got around to upgrading to PyO3 0.15...sorry for the radio silence! This is awesome, amazing work!

Btw, have y'all thought about doing an episode on the Rustacean Station podcast? They've been turning out a lot of content recently and I think folks would love to hear about PyO3! https://rustacean-station.org (We're thinking of reaching out for Pants to discuss how we're using Rust ecosystem like Tokio and PyO3!)

[davidhewitt]

👍 I would love to go on a podcast to talk about PyO3, but unfortunately now is not the right time for me to do that personally.