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

Declarative modules next steps #3900

Open
3 of 12 tasks
Tpt opened this issue Feb 26, 2024 · 14 comments
Open
3 of 12 tasks

Declarative modules next steps #3900

Tpt opened this issue Feb 26, 2024 · 14 comments

Comments

@Tpt
Copy link
Contributor

Tpt commented Feb 26, 2024

declarative modules with the #[pymodule] mod X {} syntax is now merged behind the experimental-declarative-modules syntax (#3815).

The next steps are:

Before v0.21:

Before v0.22:

  • properly register submodules in sys.modules automatically i.e. fix ModuleNotFoundError: No module named 'supermodule.submodule'; 'supermodule' is not a package #759 by default
  • allow #[pymodule_init] function to return () or PyResult<()>
  • adding a class to a module via the PyAddToModule trait is currently relying on PyTypeInfo::type_object_bound. This function panics when module initialization fail. It would be better to throw an exception instead like in PyModuleMethods::add_class (in progress in PyAddToModule: Properly propagate initialization error #3919)
  • investigate relationship between this and "two-phase initialization" e.g. Implement PEP 489 - Multi-phase extension module initialization #2245. In particular we should look at #[pymodule_data] as a way to create a per-module data mechanism (might be postpone to later)
  • allow #[pymodule_export] const FOO = ...; to add constants to the modules
  • investigate automatically filling the module parameter of #[pyclass] usages inside of decarative modules.
  • remove the experimental-declarative-modules feature flag to allow using declarative modules by default
  • update examples, README and guide

Before v0.23 (or later?):

  • Deprecate #[pyfn]
  • Deprecate "function-based" modules
@davidhewitt
Copy link
Member

Fantastic, thanks for creating this list! 👍

@davidhewitt
Copy link
Member

Possibly worth us remembering the discussion in #3919 (comment) to decide what is allowed to export before we declare stable and remove the feature gate.

@nickdrozd
Copy link

Some user feedback: I just upgraded to 0.21.0-beta.0, and the declarative module feature is very nice. It allows for getting rid of a bunch of worthless junk like m.add_function(wrap_pyfunction!(some_func, m)?)?;. Maybe I hate repetitive boilerplate more than most, but this was a big relief for me.

Of course you have all had a lot of discussion about this, and I don't understand most of the details. But from my perspective, the best course would be to bless declarative modules as the standard way of doing things and throw all that old m.add_function stuff in the garbage ASAP.

It's not just that it's a nice feature. It's also that it might end up being on the migration path for a lot of users anyway. Moving from 20 to 21 raised several compiler warnings, and the only way I could figure out to silence all of them was to use a declarative module.

@davidhewitt
Copy link
Member

Hey @nickdrozd, thanks for the feedback! I'm pleased you like these, I agree that imperative modules should be deprecated and replaced with these modules as soon as possible. This feature is probably the thing I've wanted most for PyO3 for the longest, except for the Bound API we're finally shipping in 0.21.

We could just call these stable in 0.21, but there are a couple of corner cases of unresolved design that I think could break people so I'd like to get these at least understood before we tell people it's stable. From the guide that we'll ship in 0.21:

Some changes are planned to this feature before stabilization, like automatically filling submodules into sys.modules to allow easier imports (see issue #759) and filling the module argument of inlined #[pyclass] automatically with the proper module name. Macro names might also change.

@davidhewitt
Copy link
Member

Just to note against the alternative of 0.21 delaying until these design points are ready: so much is changing in 0.21 already that I'm quite keen to see the release finalised so that users can handle these. There's a lot of stuff beginning to pile up that I've asked to wait until 0.22 already, it would be good to unblock the development flow.

If it turns out that lots of users need to update to declarative modules to solve compile issues, then I think probably we got something wrong with the design of the Bound API migration 😢. Hopefully the additional deprecation warnings we're shipping in 0.21 final compared to 0.21 beta will make the difference.

@nickdrozd
Copy link

If it turns out that lots of users need to update to declarative modules to solve compile issues, then I think probably we got something wrong with the design of the Bound API migration 😢. Hopefully the additional deprecation warnings we're shipping in 0.21 final compared to 0.21 beta will make the difference.

I had a setup like this:

#[pymodule]
fn rust_stuff(py: Python, m: &PyModule) -> PyResult<()> {
    // ...
}

After switching to 0.21.0-beta.0, I got this compiler warning:

warning: use of deprecated method `pyo3::methods::Extractor::<T>::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument
  --> src/lib.rs:30:27
   |
30 | fn rust_stuff(py: Python, m: &PyModule) -> PyResult<()> {
   |                           ^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

I don't know anything about Bound or extract_gil_ref, so the warning left me baffled. It seemed easier just to switch to the declarative style.

(I am using PyO3 to rewrite a Python project in Rust using a bottom-up leaf-first approach. So Python calls into Rust, but the Rust code never interacts with Python except through pyfunction, pyclass, etc).

@davidhewitt
Copy link
Member

Do you think it would have helped if these deprecation messages link you to the PyO3 migration guide?

@nickdrozd
Copy link

Do you think it would have helped if these deprecation messages link you to the PyO3 migration guide?

I check the migration guide, but it doesn't say anything about how to update m: &PyModule.

@alecandido
Copy link

alecandido commented Apr 11, 2024

I'm a bit puzzled by the submodule. In the example on the docs is left empty

but as soon as I try to add something, it fails:

#[pymodule]
mod submodule {
    #[pyfunction]    // ■■ consider importing one of these items: `use crate::pyfunction;    `, `use pyo3::pyfunction;    `
    pub fn triple(x: usize) -> usize {    // ■ failed to resolve: function `triple` is not a crate or module  function `triple`
        x * 3
    }
}
error: cannot find attribute `pyfunction` in this scope
  --> src/lib.rs:24:11
   |
24 |         #[pyfunction]
   |           ^^^^^^^^^^
   |
help: consider importing one of these items
   |
24 +         use crate::pyfunction;
   |
24 +         use pyo3::pyfunction;
   |
                                                                                                                                         
error[E0433]: failed to resolve: function `triple` is not a crate or module
  --> src/lib.rs:25:16
   |
25 |         pub fn triple(x: usize) -> usize {
   |                ^^^^^^ function `triple` is not a crate or module

and without the #[pyfunction] it simply doesn't do anything...
(i.e. it's not accessible from the Python module.submodule.triple)

How is it supposed to be used?

Ok, nothing, this was at most a documentation issue, I was just missing a use super::* or use pyo3::prelude::* inside the submodule.

This works perfectly fine:

#[pymodule]
mod submodule {
    use pyo3::prelude::*;

    #[pyfunction]
    pub fn triple(x: usize) -> usize {
        x * 3
    }
}

@alecandido
Copy link

Moreover, something weird happens with classes. This is working as expected:

use pyo3::prelude::*;

mod othermod {
    use pyo3::prelude::*;

    #[pyfunction]
    pub fn triple(x: usize) -> usize {
        x * 3
    }
}

#[pymodule]
mod myext {
    #[pymodule_export]
    use super::othermod::triple;
}

but this is not:

use pyo3::prelude::*;
                                                                                           
mod othermod {
    use pyo3::prelude::*;
                                                                                           
    #[pyclass]    // ■ private associated constant defined here
    pub struct Unit;
}
                                                                                           
#[pymodule]    // ■■ associated constant `_PYO3_DEF` is private  private associated constant
mod myext {               
    #[pymodule_export]
    use super::othermod::Unit;
}
error[E0624]: associated constant `_PYO3_DEF` is private
  --> src/lib.rs:10:1
   |
6  |     #[pyclass]
   |     ---------- private associated constant defined here
...
10 | #[pymodule]
   | ^^^^^^^^^^^ private associated constant
   |
   = note: this error originates in the attribute macro `pymodule` (in Nightly builds, run with -Z macro-backtrace for more info)

@LilyFoote
Copy link
Contributor

@alecandido I think this bug is #4036.

@alecandido
Copy link

Oh, thanks. I actually searched for _PYO3_DEF in the repo, but I forgetting the leading _ (and it didn't show the issue...)

https://github.com/search?q=repo%3APyO3%2Fpyo3+PYO3_DEF&type=issues
https://github.com/search?q=repo%3APyO3%2Fpyo3+_PYO3_DEF&type=issues

Sorry...

@wyfo
Copy link
Contributor

wyfo commented Apr 16, 2024

Just a remark: exception created with create_exception! are not mentioned in this issue. The only solution I see for the moment is to declare exception outside the module and use #[pymodule_export].
I don't have any idea on how we could improve ergonomics for now.

@Tpt
Copy link
Contributor Author

Tpt commented Apr 17, 2024

Just a remark: exception created with create_exception! are not mentioned in this issue. The only solution I see for the moment is to declare exception outside the module and use #[pymodule_export].

With the current code, yes! I would tend to think the long term solution is #295 but it seems quite a bit of work. An other slighty hacky approach might be to make #[pymodule] macro automatically expose all create_exception! calls inside of the module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants