PyO3 / pyo3

Rust bindings for the Python interpreter
https://pyo3.rs
Apache License 2.0
12.12k stars 745 forks source link

Feature request: Support for allowing easily emitting `DeprecationWarning`s from Rust to Python #4316

Open miikkas opened 3 months ago

miikkas commented 3 months ago

Hi!

It would be great if PyO3 supported marking items in the compiled API as deprecated when used from Python.

My main use for PyO3 involves writing library code in Rust and exposing its interface to Python, essentially speeding up Python scripts by several orders of magnitude. Marking API functionality as deprecated is a useful concept, but currently there doesn't seem to be a straightforward way to do it with PyO3 for the functions callable from Python.

If possible, using Rust's existing #[deprecated] attribute for this would be neat. Currently, marking #[pyfunction]s with it causes a Rust-side compile-time deprecation warning, e.g.:

warning: use of deprecated function `pyo3_deprecated::deprecated_with_rust_attribute`
  --> src/lib.rs:10:8
   |
10 |     fn deprecated_with_rust_attribute() -> PyResult<()> {
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default
Example code `pyo3_deprecated/src/lib.rs`: ```rust use pyo3::prelude::*; use pyo3::exceptions::PyDeprecationWarning; #[pymodule] mod pyo3_deprecated { use super::*; #[pyfunction] #[deprecated] fn deprecated_with_rust_attribute() -> PyResult<()> { println!("fn: deprecated_with_rust_attribute"); Ok(()) } #[pyfunction] fn deprecated_by_warning() -> PyResult<()> { println!("fn: deprecated_by_warning"); Python::with_gil(|py| { let builtins = py.import_bound("builtins").unwrap(); let deprecation_warning = builtins.getattr("DeprecationWarning").unwrap(); let warnings = py.import_bound("warnings").unwrap(); warnings.getattr("warn") .unwrap() .call1(("This function is deprecated", deprecation_warning)) .unwrap(); }); Ok(()) } #[pyfunction] fn deprecated_by_raising() -> PyResult<()> { println!("fn: deprecated_by_raising"); Err(PyDeprecationWarning::new_err("This function is deprecated")) } } ``` `pyo3_deprecated/script.py`: ```python import pyo3_deprecated pyo3_deprecated.deprecated_with_rust_attribute() pyo3_deprecated.deprecated_by_warning() pyo3_deprecated.deprecated_by_raising() ``` Output of command `python3 script.py`: ``` $ python script.py fn: deprecated_with_rust_attribute fn: deprecated_by_warning //pyo3_deprecated/script.py:4: DeprecationWarning: This function is deprecated pyo3_deprecated.deprecated_by_warning() fn: deprecated_by_raising Traceback (most recent call last): File "//pyo3_deprecated/script.py", line 5, in pyo3_deprecated.deprecated_by_raising() DeprecationWarning: This function is deprecated ```

I'm not very familiar with PyO3's code base or CPython's C API, but I guess these resources could be useful:

davidhewitt commented 3 months ago

I think this is a great idea and would gladly accept a PR for this! (I might get to this one day myself, but I have other priorities for 0.23 which I don't want to get distracted from.)

miikkas commented 3 months ago

Nice to hear. :) I could actually try implementing a proof-of-concept leveraging Rust's #[deprecated] for marking functions, at least, but would likely need some tips to finalize it into a production grade PR.

mejrs commented 3 months ago

I also think it's a great idea but to me it feels wrong to use #[deprecated] for this. I'd prefer if the syntax is #[pyo3(deprecated)] (or something else clearly pyo3 related) instead.

miikkas commented 3 months ago

I also think it's a great idea but to me it feels wrong to use #[deprecated] for this. I'd prefer if the syntax is #[pyo3(deprecated)] (or something else clearly pyo3 related) instead.

Having a separate, PyO3-specific syntax would have benefits from the point of view of consistency with some of the existing #[pyo3(...)] attributes. However, the problem I foresee with this approach is that from the point of view of familiarity with Rust, the PyO3-specific syntax would, in spirit, duplicate the already existing #[deprecated] syntax – which a bit confusingly wouldn't then actually work. That might then require actively steering users away from it, while the non-functional attribute would still exist. (FWIW, I actually attempted using it before noticing it doesn't help and creating the issue. :))

If you have some specific concerns related to using #[deprecated], I'd be interested in reading about them. Thanks!

mejrs commented 3 months ago

If you have some specific concerns related to using #[deprecated], I'd be interested in reading about them. Thanks!

FlickerSoul commented 2 months ago

I would be happy to assist this in the next couple of days if you @miikkas haven't started yet :)

Additionally, because Python differentiates [DeprecationWarning](https://docs.python.org/3/library/exceptions.html#DeprecationWarning) and [FutureWarning](https://docs.python.org/3/library/exceptions.html#FutureWarning), would a attribute like #[pyo3(warning="deprecation")] and [pyo3(warning="future")] make more sense? In this way, we can support other possible warnings and future warnings if there would be.

Another question I have on top of my mind is that do we want to support user defined warning too? But I can't think of an non-trivial example that can be don't by attributes only. 🤔

mejrs commented 2 months ago

users should be able to create their own Warning subclasses and use them, so i think it should mention the class explicitly. For example #[pyo3(deprecated(message ="oh no", category=PyDeprecationWarning))] where the category defaults to UserWarning.

FlickerSoul commented 2 months ago

That sounds good. I'll work towards that idea :) Thanks!

miikkas commented 2 months ago

Thanks for elaborating, mejrs! I'm not entirely convinced that enabling this with a separate #[pyo3(...)] attribute is the right way to go, but I'll be happy to get to use the feature however it's implemented. :)

the user might have a crate that is usable both as a python library and a rust library, and intend the deprecation only for the rust users of the library

I wasn't really aware that someone may want to do this. After all, the functions exposed to Python are quite specifically marked so. Is this really a use case that should have much weight on the issue?

personally, I am not a fan of doing "clever" things. I'm wary of them because they tend to result in doing things that are surprising or not what the user intended.

In which way would you say using #[deprecated] for this is "clever", as you say? I reached out to it since I found it intuitive, after all the documentation strings, for example, are already included in the __doc__ property of functions.

we might want to add configuration options, like what kind of Warning subclass should be used, or what any of the values in https://docs.python.org/3/library/warnings.html#warning-categories should have. We cannot have this if we piggyback on #[deprecated].

I agree.

miikkas commented 2 months ago

I would be happy to assist this in the next couple of days if you @miikkas haven't started yet :)

Please go ahead. :) I only made an unpolished proof-of-concept based on the #[deprecated] attribute, which I don't really have the time to change and publish for now due to some hurries at $DAYJOB.

miikkas commented 2 months ago

I think one issue remains with the route of using a new #[pyo3(...)] attribute for this. Should something be done with #[deprecated], since currently marking functions (exposed to Python) with it causes the Rust-side compile-time deprecation warning. From a user's perspective it's a bit weird, since the user themselves doesn't call it from Rust.

Maybe a compile-time message to point users towards the appropriate attribute would be helpful? Another option I'd prefer would be to just have it do the same thing as the new #[pyo3(...)] attribute but hardcoded for DeprecationWarning.

FlickerSoul commented 2 months ago

~Maybe it would make sense to add #[deprecated] attribute to the function generated by macro with the #[pyo3(deprecated(..., deprecated_rust))]? In this way, we won't have duplication in syntax and programmers would have a choice to specify which behavior they would like. What do you think?~

~(deprecated_rust is a terrible name but you get the idea :))~