add `PyModule::new_bound` and `PyModule::import_bound` by davidhewitt · Pull Request #3775 · PyO3/pyo3

davidhewitt · 2024-01-29T11:50:26Z

Part of #3684

This is a WIP migration of PyModule::new and PyModule::import to the new APIs.

I decided to pause on this one for a bit and leave in draft, because:

I started moving Python::import to Python::import_bound at the same time, but this got too much so I might try to do that one first.
I think we should solve #[pymodule] taking &Bound<'_, PyModule> (probably in a similar way to the draft support &Bound<PyType> for classmethod #3744) before continuing here, because a lot of the examples / docs get in a really bad interim state here otherwise.

codspeed-hq · 2024-01-29T12:09:25Z

CodSpeed Performance Report

Merging #3775 will not alter performance

_{Comparing davidhewitt:import-bound (5296636) with main (5ddcd46)}

Summary

✅ 67 untouched benchmarks

davidhewitt · 2024-02-18T22:44:22Z

Ok, I rebased this and it should be in a suitable state now. We still need to resolve #3808, but the diff here is still pretty reviewable without that cleanup.

Icxolu

Nice to see a lot of as_borrowed and as_gil_ref disappear!

wrap_pyfunction is a bit of a bound-ref-bound circle for now, but that's ok. I noticed a type error in a place or two, that the compiler/ci shout also pickup, and a few style nits.

A bit unrelated, but while reviewing I noticed that this example also needs updating. And since compile_fail examples tend to fail for the wrong reason as time passes, I might as well mention it, before I forget again 😄 .

pyo3/src/instance.rs

Lines 609 to 636 in 2bdc80a

    
           /// ```compile_fail 
        
           /// # use pyo3::prelude::*; 
        
           /// # use pyo3::types::PyDict; 
        
           /// # 
        
           /// #[pyclass] 
        
           /// struct Foo<'py> { 
        
           ///     inner: &'py PyDict, 
        
           /// } 
        
           /// 
        
           /// impl Foo { 
        
           ///     fn new() -> Foo { 
        
           ///         let foo = Python::with_gil(|py| { 
        
           ///             // `py` will only last for this scope. 
        
           /// 
        
           ///             // `&PyDict` derives its lifetime from `py` and 
        
           ///             // so won't be able to outlive this closure. 
        
           ///             let dict: &PyDict = PyDict::new(py); 
        
           /// 
        
           ///             // because `Foo` contains `dict` its lifetime 
        
           ///             // is now also tied to `py`. 
        
           ///             Foo { inner: dict } 
        
           ///         }); 
        
           ///         // Foo is no longer valid. 
        
           ///         // Returning it from this function is a 💥 compiler error 💥 
        
           ///         foo 
        
           ///     } 
        
           /// } 
        
           /// ```

examples/maturin-starter/src/lib.rs

examples/setuptools-rust-starter/src/lib.rs

src/conversions/num_complex.rs

src/marker.rs

src/types/module.rs

tests/test_module.rs

tests/test_various.rs

LilyFirefly

I think we can change some more &PyModule to &Bound<'_, PyModule>, but maybe these should all wait until we update the wrap_pyfunction! macro to support Bound.

LilyFirefly · 2024-02-20T00:16:47Z

guide/src/module.md

@@ -78,9 +78,9 @@ fn parent_module(py: Python<'_>, m: &PyModule) -> PyResult<()> {
 }

 fn register_child_module(py: Python<'_>, parent_module: &PyModule) -> PyResult<()> {


Can we do:

Suggested change

fn register_child_module(py: Python<'_>, parent_module: &PyModule) -> PyResult<()> {

fn register_child_module(py: Python<'_>, parent_module: &Bound<'_, PyModule>) -> PyResult<()> {

LilyFirefly · 2024-02-20T00:17:02Z

guide/src/module.md

-    parent_module.add_submodule(child_module)?;
+    let child_module = PyModule::new_bound(py, "child_module")?;
+    child_module.add_function(&wrap_pyfunction!(func, child_module.as_gil_ref())?.as_borrowed())?;
+    parent_module.add_submodule(child_module.as_gil_ref())?;


Suggested change

parent_module.add_submodule(child_module.as_gil_ref())?;

parent_module.add_submodule(child_module)?;

LilyFirefly · 2024-02-20T00:25:59Z

src/types/module.rs

@@ -286,10 +336,10 @@ impl PyModule {
    ///
    /// #[pymodule]
    /// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {


Suggested change

/// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {

/// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> {

LilyFirefly · 2024-02-20T00:26:09Z

src/types/module.rs

    ///     submodule.add("super_useful_constant", "important")?;
    ///
-    ///     module.add_submodule(submodule)?;
+    ///     module.add_submodule(submodule.as_gil_ref())?;


Suggested change

/// module.add_submodule(submodule.as_gil_ref())?;

/// module.add_submodule(submodule)?;

LilyFirefly · 2024-02-20T00:26:54Z

src/types/module.rs

@@ -480,10 +530,10 @@ pub trait PyModuleMethods<'py> {
    ///
    /// #[pymodule]
    /// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {


Suggested change

/// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {

/// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> {

LilyFirefly · 2024-02-20T00:27:06Z

src/types/module.rs

    ///     submodule.add("super_useful_constant", "important")?;
    ///
-    ///     module.add_submodule(submodule)?;
+    ///     module.add_submodule(submodule.as_gil_ref())?;


Suggested change

/// module.add_submodule(submodule.as_gil_ref())?;

/// module.add_submodule(submodule)?;

LilyFirefly · 2024-02-20T00:28:11Z

tests/test_module.rs

@@ -258,12 +258,12 @@ fn superfunction() -> String {
 #[pymodule]
 fn supermodule(py: Python<'_>, module: &PyModule) -> PyResult<()> {


Suggested change

fn supermodule(py: Python<'_>, module: &PyModule) -> PyResult<()> {

fn supermodule(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> {

LilyFirefly · 2024-02-20T00:28:41Z

tests/test_module.rs

-    module.add_submodule(module_to_add)?;
+    let module_to_add = PyModule::new_bound(py, "submodule")?;
+    submodule(&module_to_add)?;
+    module.add_submodule(module_to_add.as_gil_ref())?;


Suggested change

module.add_submodule(module_to_add.as_gil_ref())?;

module.add_submodule(module_to_add)?;

LilyFirefly · 2024-02-20T00:29:00Z

tests/test_module.rs

+    module.add_submodule(module_to_add.as_gil_ref())?;
+    let module_to_add = PyModule::new_bound(py, "submodule_with_init_fn")?;
+    submodule_with_init_fn(py, module_to_add.as_gil_ref())?;
+    module.add_submodule(module_to_add.as_gil_ref())?;


Suggested change

module.add_submodule(module_to_add.as_gil_ref())?;

module.add_submodule(module_to_add)?;

davidhewitt · 2024-02-20T07:49:04Z

Indeed, #[pymodule] doesn't yet support a &Bound<PyModule> argument yet either. It might be possible to achieve that in one go based on what I've got remaining in #3744. I might try that after this is merged.

davidhewitt · 2024-02-20T08:20:15Z

@LilyFoote regarding the suggestions above, the blocker is the support for #[pymodule], otherwise those suggestions would compile. I'll try to include those in the follow-up.

davidhewitt · 2024-02-20T09:42:06Z

Ooof can't keep up with the merge conflicts! I'll try to fixup this again this evening and hopefully see it across the line.

davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 29, 2024

davidhewitt mentioned this pull request Feb 4, 2024

New Bound API implementation tracking issue #3684

Closed

63 tasks

Icxolu mentioned this pull request Feb 13, 2024

port Python::import to Bound API #3832

Merged

davidhewitt force-pushed the import-bound branch from 575e0bf to 3eb45fb Compare February 14, 2024 22:39

davidhewitt changed the title ~~add PyModule::new and PyModule::import_bound~~ add PyModule::new_bound and PyModule::import_bound Feb 18, 2024

add PyModule::new and PyModule::import_bound

2bdc80a

davidhewitt force-pushed the import-bound branch from 3eb45fb to 2bdc80a Compare February 18, 2024 22:43

davidhewitt marked this pull request as ready for review February 18, 2024 22:44

Icxolu reviewed Feb 19, 2024

View reviewed changes

davidhewitt mentioned this pull request Feb 19, 2024

docs: update example for storing Py<T> in structs #3876

Merged

davidhewitt added 2 commits February 19, 2024 23:11

review: Icxolu feedback

1f5a16e

Merge branch 'main' into import-bound

13b6c79

LilyFirefly approved these changes Feb 20, 2024

View reviewed changes

Merge branch 'main' into import-bound

9196fbb

davidhewitt enabled auto-merge February 20, 2024 08:20

davidhewitt added 2 commits February 22, 2024 00:43

Merge branch 'main' into import-bound

fdbac16

Merge branch 'main' into import-bound

5296636

davidhewitt added this pull request to the merge queue Feb 22, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 22, 2024

davidhewitt added this pull request to the merge queue Feb 22, 2024

Merged via the queue into PyO3:main with commit 9e74c85 Feb 22, 2024

davidhewitt deleted the import-bound branch February 22, 2024 10:17

	/// ```compile_fail
	/// # use pyo3::prelude::*;
	/// # use pyo3::types::PyDict;
	/// #
	/// #[pyclass]
	/// struct Foo<'py> {
	/// inner: &'py PyDict,
	/// }
	///
	/// impl Foo {
	/// fn new() -> Foo {
	/// let foo = Python::with_gil(\|py\| {
	/// // `py` will only last for this scope.
	///
	/// // `&PyDict` derives its lifetime from `py` and
	/// // so won't be able to outlive this closure.
	/// let dict: &PyDict = PyDict::new(py);
	///
	/// // because `Foo` contains `dict` its lifetime
	/// // is now also tied to `py`.
	/// Foo { inner: dict }
	/// });
	/// // Foo is no longer valid.
	/// // Returning it from this function is a 💥 compiler error 💥
	/// foo
	/// }
	/// }
	/// ```

		@@ -78,9 +78,9 @@ fn parent_module(py: Python<'_>, m: &PyModule) -> PyResult<()> {
		}

		fn register_child_module(py: Python<'_>, parent_module: &PyModule) -> PyResult<()> {

	parent_module.add_submodule(child_module.as_gil_ref())?;
	parent_module.add_submodule(child_module)?;

	/// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {
	/// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> {

	/// module.add_submodule(submodule.as_gil_ref())?;
	/// module.add_submodule(submodule)?;

		@@ -258,12 +258,12 @@ fn superfunction() -> String {
		#[pymodule]
		fn supermodule(py: Python<'_>, module: &PyModule) -> PyResult<()> {

	module.add_submodule(module_to_add.as_gil_ref())?;
	module.add_submodule(module_to_add)?;

Conversation

davidhewitt commented Jan 29, 2024

Uh oh!

codspeed-hq bot commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #3775 will not alter performance

Summary

Uh oh!

davidhewitt commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Icxolu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LilyFirefly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt commented Feb 20, 2024

Uh oh!

davidhewitt commented Feb 20, 2024

Uh oh!

davidhewitt commented Feb 20, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq bot commented Jan 29, 2024 •

edited

Loading

davidhewitt commented Feb 18, 2024 •

edited

Loading