Pymodule bound by LilyFirefly · Pull Request #3897 · PyO3/pyo3

LilyFirefly · 2024-02-25T00:21:11Z

Builds upon #3744.

pyo3-macros-backend/src/module.rs

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Icxolu

Nice to see this getting closer to landing. After this I think we can really start to update documentation and examples! 🚀

I have added some thoughts, but you should definitely wait for a second opinion.

src/derive_utils.rs

Icxolu · 2024-02-25T13:58:30Z

src/macros.rs

@@ -126,13 +126,42 @@ macro_rules! py_run_impl {
 macro_rules! wrap_pyfunction {


This one should now get a deprecation warning I thing

Good spot! I think maybe this makes sense to do in a separate PR since it'll cause a lot of churn in tests.

Makes sense 👍

Looks like that is #3899

src/types/module.rs

src/macros.rs

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

src/impl_/pymodule.rs

src/types/module.rs

This allows us to remove the awkward `PyFunctionArgumentsBound` enum.

src/macros.rs

davidhewitt · 2024-02-27T07:56:13Z

I pushed a final commit which removes the #[doc_hidden] fn wrap_pyfunction and enables type inference for wrap_pyfunction_bound again. Please forgive me @LilyFoote, I don't normally do that uninvited, I just thought that it might be helpful in this case as this PR seems to be one of the critical bits remaining before we can release 0.21.

I couldn't find a good way to describe what I was thinking until I'd tried it out, at which point I had the commit ready to go 🙈

Do let me know what you think of that last commit, if it seems good, maybe this is ready to merge?!

davidhewitt · 2024-02-27T08:24:11Z

Ok so I had a slightly nutty magical idea to use some trickery to automatically infer the output type of wrap_pyfunction from the input. It now continues to work as-is, but now also accepts Bound<'py, Module> (and friends) and will return a Bound<'py, PyCFunction> in those cases.

If the input argument is Python<'py> then we can't use inference and instead users have to use wrap_pyfunction_bound! just in that case.

The nice thing about this is that then for the vast majority of user code they don't need to change anything in their #[pymodule] other than change from &'py PyModule to &Bound<'py, PyModule> at the top and everything else keeps working. We wouldn't even need to deprecate wrap_pyfunction then!

I pushed that as a final commit. What do you think?

codspeed-hq · 2024-02-27T08:39:47Z

CodSpeed Performance Report

Merging #3897 will improve performances by 11.35%

_{Comparing LilyFoote:pymodule-bound (f7a4e82) with main (404161c)}

Summary

⚡ 1 improvements
✅ 66 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`LilyFoote:pymodule-bound`	Change
⚡	`extract_str_extract_success`	1,090 ns	978.9 ns	+11.35%

LilyFirefly · 2024-02-27T10:27:41Z

pyo3-macros-backend/src/pyfunction.rs

                use #krate::prelude::PyModuleMethods;
                use ::std::convert::Into;
-                module.add_function(&#krate::types::PyCFunction::internal_new(&DEF, module.as_gil_ref().into())?)
+                module.add_function(#krate::types::PyCFunction::internal_new(&DEF, module.as_gil_ref().into())?)


It's a bit sad to still need this as_gil_ref() call for now.

LilyFirefly · 2024-02-27T10:32:03Z

Looks good to me! If @Icxolu and @maffoo are happy too, I think this is ready!

maffoo

LGTM. I think it'd be good to move forward with this and we can revisit the macro import hygiene in a follow-up.

maffoo · 2024-02-27T16:53:53Z

pyo3-macros-backend/src/module.rs


+    let mut stmts: Vec<syn::Stmt> = vec![syn::parse_quote!(
+        #[allow(unknown_lints, unused_imports, redundant_imports)]
+        use #krate::{PyNativeType, types::PyModuleMethods};


I'm hesitant to add this kind of use statement, which transparently injects things into the namespace of user code and so would seem to violate hygiene (see discussion in the follow-up PR here: #3899 (comment)). An alternative would be to use fully-qualified references to trait methods in the macro-generated code. But as @Icxolu noted in that discussion, the patterns around these new *Methods traits are not very well established, so I could be persuaded :-)

Yeah, after that discussion I tend to agree, that we should not inject these traits silently. If we still want to do it we should probably at least import them anonymously e.g. as use "Trait" as _. I would not consider it a blocker here, we can change in a followup if needed. Another thing would be if we should seal these traits, but that's another discussion entirely.

Agreed. Maybe a good solution here it to write a private trait in impl_/pymodule.rs which has just the methods we need implemented for both Bound<'_, PyModule> and &'py PyModule, which we can then call directly without imports.

(This is actually probably very similar to what @LilyFoote had already done with the #[doc(hidden)] fn wrap_pyfunction, just located as a private trait inside the impl codebase.)

Icxolu

Nice solution, this look pretty elegant. I have found a minor point regarding PyFunction::internal_new_bound plus two general comments, otherwise LGTM!

Icxolu · 2024-02-27T17:56:33Z

src/types/function.rs

    }
+
+    #[doc(hidden)]
+    pub(crate) fn internal_new_bound<'py>(


I think we don't need this. We can change the signature of internal_new to match the new _bound APIs and move let (py, module) = py_or_module.into_py_and_maybe_module(); into the deprecated gil-ref APIs

Icxolu · 2024-02-27T17:58:26Z

src/types/module.rs

    /// [1]: crate::prelude::pyfunction
    /// [2]: crate::wrap_pyfunction
-    fn add_function(&self, fun: &Bound<'_, PyCFunction>) -> PyResult<()>;
+    fn add_function(&self, fun: Bound<'_, PyCFunction>) -> PyResult<()>;


I guess this is just to make it nicer, so that we don't have to use&wrap_pyfunction! instead of just wrap_pyfunction!?

Yeah I think it's probably worth making this change? It's not like we expect this to be called in a hot loop. I guess we could also have impl AsRef<Bound<'_, PyCFunction>>?

Icxolu · 2024-02-27T18:02:25Z

src/impl_/pyfunction.rs

+pub trait WrapPyFunctionArg<'py, T> {
+    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<T>;
+}


Very interesting solution! I guess T could also be an associated type given that it's not intended to be implemented more than once per type. But since this is internal API, I'm fine either way.

Yes actually I was thinking that later on today, I had generic originally because I wanted to play with type inference but given that each type now implements only once we could go for the associated type.

Icxolu · 2024-02-27T18:06:48Z

pyo3-macros-backend/src/module.rs


+    let mut stmts: Vec<syn::Stmt> = vec![syn::parse_quote!(
+        #[allow(unknown_lints, unused_imports, redundant_imports)]
+        use #krate::{PyNativeType, types::PyModuleMethods};


Yeah, after that discussion I tend to agree, that we should not inject these traits silently. If we still want to do it we should probably at least import them anonymously e.g. as use "Trait" as _. I would not consider it a blocker here, we can change in a followup if needed. Another thing would be if we should seal these traits, but that's another discussion entirely.

davidhewitt · 2024-02-27T19:19:38Z

Thanks all! I think given this PR seems to be blocking a lot of the final refinements to the codebase which we need for 0.21, and we're happy enough with this as-is, I think let's merge this as-is. There are quite a few follow-up items. Anyone got time / interest in taking them on? Otherwise I can try to do these maybe at the weekend.

(I also just made some comments on other issues specifically about the trait sealing...)

LilyFirefly commented Feb 25, 2024

View reviewed changes

pyo3-macros-backend/src/module.rs Outdated Show resolved Hide resolved

davidhewitt added the CI-skip-changelog Skip checking changelog entry label Feb 25, 2024

LilyFirefly force-pushed the pymodule-bound branch from 2f8f123 to 031f033 Compare February 25, 2024 13:26

Support Bound in pymodule and pyfunction macros

9db0ea9

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

LilyFirefly force-pushed the pymodule-bound branch from 031f033 to 9db0ea9 Compare February 25, 2024 13:27

LilyFirefly marked this pull request as ready for review February 25, 2024 13:29

Icxolu reviewed Feb 25, 2024

View reviewed changes

LilyFirefly mentioned this pull request Feb 25, 2024

Update ModuleInitializer to handle Bound API #3894

Closed

maffoo mentioned this pull request Feb 26, 2024

Use single-arg form of #[pymodule] function in docs and tests #3899

Merged

maffoo reviewed Feb 26, 2024

View reviewed changes

src/macros.rs Outdated Show resolved Hide resolved

maffoo mentioned this pull request Feb 26, 2024

Allow #[pymodule] functions to take a single module arg #3905

Merged

Remove spurious $ character

a05342e

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

davidhewitt reviewed Feb 26, 2024

View reviewed changes

src/impl_/pymodule.rs Outdated Show resolved Hide resolved

src/types/module.rs Outdated Show resolved Hide resolved

This was referenced Feb 26, 2024

support &Bound<PyType> for classmethod #3744

Closed

New Bound API implementation tracking issue #3684

Closed

LilyFirefly added 2 commits February 26, 2024 23:35

Rework PyCFunction::new_bound signatures

e310b4a

This allows us to remove the awkward `PyFunctionArgumentsBound` enum.

Use BoundRef instead of BoundModule

b67f93f

maffoo reviewed Feb 27, 2024

View reviewed changes

src/macros.rs Outdated Show resolved Hide resolved

support argument deduction for wrap_pyfunction_bound!

90a1efe

support wrap_pyfunction! with Bound input/output

4af75fe

LilyFirefly added 2 commits February 27, 2024 09:42

Fix docs link to wrap_pyfunction_bound!

df1da21

Revert back to wrap_pyfunction!

f7a4e82

LilyFirefly commented Feb 27, 2024

View reviewed changes

maffoo approved these changes Feb 27, 2024

View reviewed changes

Icxolu approved these changes Feb 27, 2024

View reviewed changes

davidhewitt mentioned this pull request Feb 27, 2024

Seal traits inside impl_ submodule #3908

Open

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

Merged via the queue into PyO3:main with commit a3ad28b Feb 27, 2024

Icxolu mentioned this pull request Feb 27, 2024

cleans up PyCFunction::internal_new #3910

Merged

LilyFirefly deleted the pymodule-bound branch February 27, 2024 20:38

davidhewitt mentioned this pull request Feb 28, 2024

Make #wrap_pyfunction return a Bound value #3808

Closed

		@@ -126,13 +126,42 @@ macro_rules! py_run_impl {
		macro_rules! wrap_pyfunction {

Conversation

LilyFirefly commented Feb 25, 2024

Uh oh!

Uh oh!

Icxolu left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidhewitt commented Feb 27, 2024

Uh oh!

davidhewitt commented Feb 27, 2024

Uh oh!

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

Uh oh!

Merging #3897 will improve performances by 11.35%

Summary

Benchmarks breakdown

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LilyFirefly commented Feb 27, 2024

Uh oh!

maffoo 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!

Icxolu 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!

davidhewitt commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codspeed-hq bot commented Feb 27, 2024 •

edited

Loading