make `GILOnceCell::get_or_try_init_type_ref` public #4542

glevco · 2024-09-10T18:48:18Z

Resolves #4516.

Change GILOnceCell::get_or_try_init_type_ref to GILOnceCell::import and make it public API, adding an example for it.
Update "Executing existing Python code" section of the guide:
- Add mention to GILOnceCell::import.
- Change Python::eval_bound (which is deprecated) to Python::eval.
- Change Python::run_bound (which is deprecated) to Python::run, also adding missing link.

mejrs

Change PyModule::import suggestion to Python::import instead

Why? I don't have a strong opinion either way, but slightly prefer PyModule::import.

mejrs · 2024-09-11T11:24:51Z

src/sync.rs

+    /// # });
+    /// ```
+    ///
+    pub fn get_or_try_init_type_ref<'py>(


This can use a better name. Maybe just "import"? 🤔

I agree, but then maybe it should be implemented for GILOnceCell<Py<T>> where T: PyTypeInfo instead of GILOnceCell<Py<PyType>>? Otherwise, I would call it import_type.

I'd be fine with that. I think you want T: PyTypeCheck rather than T: PyTypeInfo, though.

Right, done in 79568f6!

glevco · 2024-09-11T16:34:58Z

@mejrs

Why? I don't have a strong opinion either way, but slightly prefer PyModule::import.

Since Python::import was used in the implementation of get_or_try_init_type_ref, I assumed it was preferable. Also, it pairs nicely with the other entries in this section of the guide, as they recommend other "global" functions such as Python::eval and Python::run. What do you think?

davidhewitt · 2024-09-14T05:40:26Z

I prefer PyModule::import too, I think it's nicer to think of that as a "constructor" for the PyModule type.

mejrs · 2024-09-14T15:51:54Z

What do you think?

From a documentation perspective I would prefer that people visit PyModule::import over Python::import. Python has a lot of not module related api, and its page is a lot bigger.

Also, using "constructor" syntax also makes it clear that when they use it, they're getting a module (rather than something arbitrary like from foo import bar might do).

glevco · 2024-09-14T19:50:06Z

@davidhewitt @mejrs Fair enough, I reverted the guide back to PyModule::import in 79568f6. I also changed the method name to GILOnceCell::import and made its impl generic for PyTypeCheck, as discussed above. I updated the PR description accordingly.

mejrs

Sorry for the late review. I have a couple of nits about the documentation.

mejrs · 2024-09-20T14:51:16Z

src/sync.rs

+impl<T> GILOnceCell<Py<T>>
+where
+    T: PyTypeCheck,
+{
    /// Get a reference to the contained Python type, initializing it if needed.


Just to clarify that "it" refers to the GILOnceCell, not the class in it

Suggested change

/// Get a reference to the contained Python type, initializing it if needed.

/// Get a reference to the contained Python type, initializing the cell if needed.

Done! bcb883c

mejrs · 2024-09-20T14:56:27Z

guide/src/python-from-rust/calling-existing-code.md

+[`PyModule::import`] introduces an overhead each time it's called. To avoid this in functions that are called multiple times,
+using a [`GILOnceCell`]({{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html) is recommended. Specifically, for importing types,
+[`GILOnceCell::import`]({{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html#method.import) is provided
+(check out the [example]({{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html#example-using-giloncecell-to-avoid-the-overhead-of-importing-a-class-multiple-times)).


IMO this is not the right place for this.

I would like to keep this section of the guide as concise as possible.

GILOnceCell::import does not work in the context of the example because sum is not a class, which would be confusing.

Please avoid terminology like "overhead" or "is recommended". Just say what it does, for example, "if you want to import a class, you can store a reference to it with GILOnceCell::import".

I think such a footnote would fit better on https://pyo3.rs/main/doc/pyo3/types/struct.pymodule#method.import

Sure, just made those changes in 46b18cc.

However, since I changed the implementation from PyType to T: PyTypeCheck, this does work:

#[pyfunction] fn sum<'py>(py: Python<'py>, l: Py<PyList>) -> PyResult<Bound<'py, PyAny>> { static SUM: GILOnceCell<Py<PyAny>> = GILOnceCell::new(); SUM.import(py, "builtins", "sum")? .call1((l,)) }

I guess this raises some questions. Is there any benefit in storing functions such as sum in the GILOnceCell, too? If not, should we prevent it (maybe by changing it back to PyType)? And if yes, should we be explicit in the docs that this is not only for importing classes?

mejrs · 2024-09-20T15:05:19Z

src/sync.rs

+    /// # Example: Using `GILOnceCell` to avoid the overhead of importing a class multiple times
+    ///


It is better to just say what the example does, this makes for nicer URLs as well.

Suggested change

/// # Example: Using `GILOnceCell` to avoid the overhead of importing a class multiple times

///

/// # Example: Using `GILOnceCell` to store a class in a static variable.

///

/// `GILOnceCell` can be used to avoid importing a class multiple times

(this also explicitly mentions "static", which is important. We don't want people to have local (or worse, const) GILOnceCells).

Done: 5d95fba

Co-authored-by: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com>

mejrs

Thanks!

make GILOnceCell::get_or_try_init_type_ref public

5b16662

glevco force-pushed the pub_get_or_try_init_type_ref branch from 1e65734 to 5b16662 Compare September 10, 2024 18:49

glevco marked this pull request as ready for review September 10, 2024 18:55

glevco mentioned this pull request Sep 10, 2024

Make GILOnceCell<Py<PyType>>'s get_or_try_init_type_ref public #4516

Closed

mejrs reviewed Sep 11, 2024

View reviewed changes

change GILOnceCell::get_or_try_init_type_ref to GILOnceCell::import

79568f6

glevco force-pushed the pub_get_or_try_init_type_ref branch from 245dae4 to 79568f6 Compare September 14, 2024 19:45

glevco requested review from mejrs and davidhewitt September 19, 2024 00:54

mejrs reviewed Sep 20, 2024

View reviewed changes

glevco and others added 3 commits September 20, 2024 12:37

update doc

bcb883c

Co-authored-by: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com>

update doc

5d95fba

Co-authored-by: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com>

update docs

46b18cc

glevco requested a review from mejrs September 20, 2024 16:23

mejrs approved these changes Sep 21, 2024

View reviewed changes

mejrs added this pull request to the merge queue Sep 21, 2024

Merged via the queue into PyO3:main with commit 6b3f887 Sep 21, 2024
42 of 43 checks passed

glevco deleted the pub_get_or_try_init_type_ref branch September 23, 2024 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make `GILOnceCell::get_or_try_init_type_ref` public #4542

make `GILOnceCell::get_or_try_init_type_ref` public #4542

glevco commented Sep 10, 2024 •

edited

Loading

mejrs left a comment

mejrs Sep 11, 2024

glevco Sep 11, 2024

davidhewitt Sep 14, 2024

glevco Sep 14, 2024

glevco commented Sep 11, 2024

davidhewitt commented Sep 14, 2024

mejrs commented Sep 14, 2024

glevco commented Sep 14, 2024 •

edited

Loading

mejrs left a comment

mejrs Sep 20, 2024

glevco Sep 20, 2024

mejrs Sep 20, 2024

glevco Sep 20, 2024

mejrs Sep 20, 2024

glevco Sep 20, 2024

mejrs left a comment

	/// Get a reference to the contained Python type, initializing it if needed.
	/// Get a reference to the contained Python type, initializing the cell if needed.

		/// # Example: Using `GILOnceCell` to avoid the overhead of importing a class multiple times
		///

-    /// # Example: Using `GILOnceCell` to avoid the overhead of importing a class multiple times
-    ///
+    /// # Example: Using `GILOnceCell` to store a class in a static variable.
+    ///
+    /// `GILOnceCell` can be used to avoid importing a class multiple times

make GILOnceCell::get_or_try_init_type_ref public #4542

make GILOnceCell::get_or_try_init_type_ref public #4542

Conversation

glevco commented Sep 10, 2024 • edited Loading

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

glevco commented Sep 11, 2024

davidhewitt commented Sep 14, 2024

mejrs commented Sep 14, 2024

glevco commented Sep 14, 2024 • edited Loading

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

mejrs left a comment

Choose a reason for hiding this comment

make `GILOnceCell::get_or_try_init_type_ref` public #4542

make `GILOnceCell::get_or_try_init_type_ref` public #4542

glevco commented Sep 10, 2024 •

edited

Loading

glevco commented Sep 14, 2024 •

edited

Loading