Rewrite `module.md` for clarity and add tip on code organization #1693

Eric-Arellano · 2021-06-23T18:48:03Z

This adds a recommendation on how to split FFI code over multiple Rust modules, which we have found useful at Pants.

It also rewrites some of module.md and a bit of function.md:

Consistently use a simpler example function double, rather than sum_as_string.
Use #[pyfunction] instead of #[pyfn] in the module example for simplicity.
Add a code example of renaming a #[pymodule] with #[pyo3(name = "..")].
Rewrite submodule hierarchy section, including adding the disclaimer about packages.

Eric-Arellano

I hope some of these suggestions are useful :) I love that y'all have a guide, that was a big draw to PyO3 for me personally with the Pants project.

Eric-Arellano · 2021-06-23T18:48:48Z

guide/src/function.md

    m.add_function(wrap_pyfunction!(double, m)?)?;
    Ok(())
 }
-
-# fn main() {}


This isn't necessary as of version 1.34 of Rust: https://doc.rust-lang.org/rustdoc/documentation-tests.html#using--in-doc-tests.

Eric-Arellano · 2021-06-23T18:50:21Z

guide/src/module.md

@@ -1,44 +1,49 @@
 # Python Modules

-You can create a module as follows:
+You can create a module using `#[pymodule]`:


This is something we picked up on with docs for Pants: for bigger code blocks, preview what the main thing to look for is.

Eric-Arellano · 2021-06-23T18:50:41Z

guide/src/module.md


-// add bindings to the generated Python module
-// N.B: "rust2py" must be the name of the `.so` or `.pyd` file.


This is said later in the paragraph. It's repetitive and distracting to say here too.

Eric-Arellano · 2021-06-23T18:51:36Z

guide/src/module.md

-    // PyO3 aware function. All of our Python interfaces could be declared in a separate module.
-    // Note that the `#[pyfn()]` annotation automatically converts the arguments from
-    // Python objects to Rust values, and the Rust return value back into a Python object.
-    // The `_py` argument represents that we're holding the GIL.


That's explained in function.md. This page is instead focused on modules, and there's value imo in keeping this example short and focused.

Eric-Arellano · 2021-06-23T18:53:18Z

guide/src/module.md

-    // Note that the `#[pyfn()]` annotation automatically converts the arguments from
-    // Python objects to Rust values, and the Rust return value back into a Python object.
-    // The `_py` argument represents that we're holding the GIL.
-    #[pyfn(m)]


I personally prefer the #[pyfunction] approach and think it makes things easier to understand. Better separation of concerns, such that the #[pymodule] function is solely for FFI registration and has no business logic.

I think the example is easier to follow by using #[pyfunction] - function.md still demos #[pyfn].

Iirc, the plan is to get rid of #[pyfn] at some point.

That's great! I was going to open an issue proposing that very thing :) I think there's value in having a consolidate way to do things.

Would you like me to open an issue for this? I'd be happy to implement.

Yeah, I don't like having two slightly different ways to do the same thing.

As for an issue, there is one: #694

Completely agree that #[pyfn] should go. The syntax will at least be almost the same in PyO3 0.14.

Regarding #694 - the vision was that rather than remove #[pyfn], we would first create a new #[pymodule] syntax which is used on mod my_module instead of fn my_module. Then we could deprecate the #[pymodule] function syntax and #[pyfn] together.

Eric-Arellano · 2021-06-23T18:54:23Z

guide/src/module.md

+keep your code readable.
+
+You can pass references to the `PyModule` so that each 
+module registers its own FFI code, which can give your project better 


Is FFI a safe word to use here? Generally acronyms should be avoided in docs.

Yes I think FFI is fairly prevalent in PyO3's guide. TBH we could add a glossary if we really were worried, to include other terms like GIL which we also use a lot.

guide/src/module.md

Eric-Arellano · 2021-06-23T18:56:02Z

guide/src/module.md


-In Python, modules are first class objects. This means that you can store them as values or add them to
-dicts or other modules:
+You can create a module hierarchy within a single extension module by using `PyModule.add_submodule()`.


Should this cross-reference with Rustdoc magic? I'm not sure how to do that.

Rustdoc magic? What do you mean?

As in, rustdoc has a way to refer to other parts of the code base and then it will hyperlink for you. I'm not sure how to do it, but I know it exists.

I'm wondering here if you happen to know if it's possible to cross reference from the book to the API docs?

You can just hyperlink to the latest versions with e.g.:

the pyo3.rs documentation: https://pyo3.rs/main/doc/pyo3/types/struct.PyModule.html#method.add_submodule

docs.rs: https://docs.rs/pyo3/latest/pyo3/types/struct.PyModule.html#method.add_submodule

I'm actually not sure which one you should prefer...fwiw pyo3.rs reflects the current main branch, docs.rs the latest release.

The mdbook setup has a plugin to substitute in the appropriate link, the following should do it:

{#PYO3_DOCS_URL}}/pyo3/prelude/struct.PyModule.html#method.add_submodule

mejrs

Hey, thanks for this :), this looks great.

Contributing.md

mejrs · 2021-06-23T20:38:25Z

guide/src/module.md

-    // Note that the `#[pyfn()]` annotation automatically converts the arguments from
-    // Python objects to Rust values, and the Rust return value back into a Python object.
-    // The `_py` argument represents that we're holding the GIL.
-    #[pyfn(m)]


Iirc, the plan is to get rid of #[pyfn] at some point.

mejrs · 2021-06-23T20:47:23Z

guide/src/module.md


-In Python, modules are first class objects. This means that you can store them as values or add them to
-dicts or other modules:
+You can create a module hierarchy within a single extension module by using `PyModule.add_submodule()`.


Rustdoc magic? What do you mean?

guide/src/module.md

mejrs · 2021-06-23T21:33:05Z

guide/src/module.md

 ```

-Which means that the above Python code will print `This module is implemented in Rust.`.
+## Organizing your module registration code


I would probably avoid this indirection and just add the classes and functions directly:

#[pymodule] fn my_extension(py: Python, m: &PyModule) -> PyResult<()> { m.add_class::<dirutil::SomeClass>()?; m.add_function(wrap_pyfunction!(osutil::determine_current_os, m)?)?; Ok(()) }

I guess this makes more sense if you have a big hierarchy of modules with register functions that each import other registers? But at that point you should probably just organize them in submodules.

I remember now why I originally came up with this pattern of passing &PyModule around:

error: no rules expected the token `::` --> src/lib.rs:591:43 | 14 | m.add_function(wrap_pyfunction!(osutil::determine_current_os, m)?)?; | ^^ no rules expected this token in macro call

Is that expected? Any known workarounds other than what I'm suggesting?

Hm my next idea didn't work either to leverage use:

// src/lib.rs use pyo3::prelude::*; use osutil::determine_current_os; #[pymodule] fn my_extension(py: Python, m: &PyModule) -> PyResult<()> { m.add_class::<dirutil::SomeClass>()?; m.add_function(wrap_pyfunction!(determine_current_os, m)?)?; Ok(()) } // src/dirutil.rs # mod dirutil { use pyo3::prelude::*; #[pyclass] pub(crate) struct SomeClass { x: usize, } # } // src/osutil.rs # mod osutil { use pyo3::prelude::*; #[pyfunction] pub(crate) fn determine_current_os() -> String { "linux".to_owned() } # }

❯ cargo test --doc -- doc_test::guide_module_md warning: profiles for the non root package will be ignored, specify profiles at the workspace root: package: /Users/eric/code/pyo3/examples/pyo3-pytests/Cargo.toml workspace: /Users/eric/code/pyo3/Cargo.toml Finished test [unoptimized + debuginfo] target(s) in 0.10s Doc-tests pyo3 running 5 tests test src/lib.rs - doc_test::guide_module_md (line 620) ... ignored test src/lib.rs - doc_test::guide_module_md (line 578) ... FAILED test src/lib.rs - doc_test::guide_module_md (line 535) ... ok test src/lib.rs - doc_test::guide_module_md (line 513) ... ok test src/lib.rs - doc_test::guide_module_md (line 670) ... ok failures: ---- src/lib.rs - doc_test::guide_module_md (line 578) stdout ---- error[E0425]: cannot find value `__pyo3_get_function_determine_current_os` in this scope --> src/lib.rs:586:20 | 11 | m.add_function(wrap_pyfunction!(determine_current_os, m)?)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in this scope | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: aborting due to previous error

Yeah, you're running into #1709

For now you can work around this by writing use osutil::*;

(As stated in the issue above, I hope we can fix it in a nicer way at the end of the year when we do a dependency update release.)

davidhewitt

Looks great, many thanks for doing this! A few quick comments on some of the conversations...

davidhewitt · 2021-06-24T06:46:20Z

guide/src/module.md

-    // Note that the `#[pyfn()]` annotation automatically converts the arguments from
-    // Python objects to Rust values, and the Rust return value back into a Python object.
-    // The `_py` argument represents that we're holding the GIL.
-    #[pyfn(m)]


Completely agree that #[pyfn] should go. The syntax will at least be almost the same in PyO3 0.14.

Regarding #694 - the vision was that rather than remove #[pyfn], we would first create a new #[pymodule] syntax which is used on mod my_module instead of fn my_module. Then we could deprecate the #[pymodule] function syntax and #[pyfn] together.

davidhewitt · 2021-06-24T06:47:45Z

guide/src/module.md

+keep your code readable.
+
+You can pass references to the `PyModule` so that each 
+module registers its own FFI code, which can give your project better 


Yes I think FFI is fairly prevalent in PyO3's guide. TBH we could add a glossary if we really were worried, to include other terms like GIL which we also use a lot.

guide/src/module.md

davidhewitt · 2021-06-24T06:50:56Z

guide/src/module.md


-In Python, modules are first class objects. This means that you can store them as values or add them to
-dicts or other modules:
+You can create a module hierarchy within a single extension module by using `PyModule.add_submodule()`.


The mdbook setup has a plugin to substitute in the appropriate link, the following should do it:

{#PYO3_DOCS_URL}}/pyo3/prelude/struct.PyModule.html#method.add_submodule

Eric-Arellano · 2021-06-26T17:00:09Z

FYI I'm traveling next few days on a road trip, so I likely won't get to this till I'm back.

Thanks for the review though!

Eric-Arellano · 2021-07-20T07:21:11Z

FYI I'm traveling next few days on a road trip, so I likely won't get to this till I'm back.

Took me longer to get back to this than I hoped, but updating based on feedback now :) Congrats on 0.14 release btw!

# Conflicts: # guide/src/function.md # guide/src/module.md

davidhewitt · 2021-07-20T07:36:54Z

Thanks and no worries, hope you had a great trip! Feel free to ping when it's ready for re-review.

Eric-Arellano · 2021-07-20T07:52:13Z

Thanks and no worries, hope you had a great trip!

Thanks! Washington was magical, my first time spending more than a day there and I loved the outdoors :)

Feel free to ping when it's ready for re-review.

All ready! I previewed it using the instructions you sent on Gitter, and it looks good from my POV. I'm not sure I got the {#PYO3_DOCS_URL}} thing right though - the link didn't work when I was previewing locally.

davidhewitt

Looks really great! Just a few final suggestions.

Contributing.md

guide/src/module.md

davidhewitt · 2021-07-22T07:10:23Z

Perfect! Thanks again 🚀

Rewrite module.md for clarity and add tip on code organization

7018c3f

Eric-Arellano commented Jun 23, 2021

View reviewed changes

mejrs reviewed Jun 23, 2021

View reviewed changes

davidhewitt reviewed Jun 24, 2021

View reviewed changes

Merge branch 'main' + review feedback

1d48c90

# Conflicts: # guide/src/function.md # guide/src/module.md

Add section on how to build the guide + add workaround proposed by David

fdbd0f9

davidhewitt approved these changes Jul 20, 2021

View reviewed changes

Contributing.md Show resolved Hide resolved

guide/src/module.md Outdated Show resolved Hide resolved

guide/src/module.md Outdated Show resolved Hide resolved

Make more clear references to PyO3#1709

5d900f4

davidhewitt merged commit 9ab7b1f into PyO3:main Jul 22, 2021

Eric-Arellano deleted the module-rewrite branch July 22, 2021 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite `module.md` for clarity and add tip on code organization #1693

Rewrite `module.md` for clarity and add tip on code organization #1693

Eric-Arellano commented Jun 23, 2021

Eric-Arellano left a comment

Eric-Arellano Jun 23, 2021

Eric-Arellano Jun 23, 2021

Eric-Arellano Jun 23, 2021

Eric-Arellano Jun 23, 2021

Eric-Arellano Jun 23, 2021

mejrs Jun 23, 2021

Eric-Arellano Jun 23, 2021

mejrs Jun 24, 2021

davidhewitt Jun 24, 2021

Eric-Arellano Jun 23, 2021

davidhewitt Jun 24, 2021

Eric-Arellano Jun 23, 2021

mejrs Jun 23, 2021

Eric-Arellano Jun 23, 2021

mejrs Jun 24, 2021 •

edited

Loading

davidhewitt Jun 24, 2021

mejrs left a comment

mejrs Jun 23, 2021

mejrs Jun 23, 2021

mejrs Jun 23, 2021

Eric-Arellano Jul 20, 2021

Eric-Arellano Jul 20, 2021

davidhewitt Jul 20, 2021

davidhewitt left a comment

davidhewitt Jun 24, 2021

davidhewitt Jun 24, 2021

davidhewitt Jun 24, 2021

Eric-Arellano commented Jun 26, 2021

Eric-Arellano commented Jul 20, 2021

davidhewitt commented Jul 20, 2021

Eric-Arellano commented Jul 20, 2021

davidhewitt left a comment

davidhewitt commented Jul 22, 2021


		// add bindings to the generated Python module
		// N.B: "rust2py" must be the name of the `.so` or `.pyd` file.

Rewrite module.md for clarity and add tip on code organization #1693

Rewrite module.md for clarity and add tip on code organization #1693

Conversation

Eric-Arellano commented Jun 23, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Jun 26, 2021

Eric-Arellano commented Jul 20, 2021

davidhewitt commented Jul 20, 2021

Eric-Arellano commented Jul 20, 2021

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt commented Jul 22, 2021

Rewrite `module.md` for clarity and add tip on code organization #1693

Rewrite `module.md` for clarity and add tip on code organization #1693

mejrs Jun 24, 2021 •

edited

Loading