-
Notifications
You must be signed in to change notification settings - Fork 803
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
Support for wrapping rust closures as python functions #1901
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
#[cfg(not(Py_LIMITED_API))] | ||
use pyo3::buffer::PyBuffer; | ||
use pyo3::prelude::*; | ||
use pyo3::types::PyCFunction; | ||
use pyo3::types::{self, PyCFunction}; | ||
#[cfg(not(Py_LIMITED_API))] | ||
use pyo3::types::{PyDateTime, PyFunction}; | ||
|
||
|
@@ -213,3 +213,57 @@ fn test_conversion_error() { | |
"argument 'option_arg': 'str' object cannot be interpreted as an integer" | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_closure() { | ||
let gil = Python::acquire_gil(); | ||
let py = gil.python(); | ||
|
||
let f = |args: &types::PyTuple, _kwargs: Option<&types::PyDict>| -> PyResult<_> { | ||
let gil = Python::acquire_gil(); | ||
let py = gil.python(); | ||
let res: Vec<_> = args | ||
.iter() | ||
.map(|elem| { | ||
if let Ok(i) = elem.extract::<i64>() { | ||
(i + 1).into_py(py) | ||
} else if let Ok(f) = elem.extract::<f64>() { | ||
(2. * f).into_py(py) | ||
} else if let Ok(mut s) = elem.extract::<String>() { | ||
s.push_str("-py"); | ||
s.into_py(py) | ||
} else { | ||
panic!("unexpected argument type for {:?}", elem) | ||
} | ||
}) | ||
.collect(); | ||
Ok(res) | ||
}; | ||
let closure_py = PyCFunction::new_closure(f, py).unwrap(); | ||
|
||
py_assert!(py, closure_py, "closure_py(42) == [43]"); | ||
py_assert!( | ||
py, | ||
closure_py, | ||
"closure_py(42, 3.14, 'foo') == [43, 6.28, 'foo-py']" | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_closure_counter() { | ||
let gil = Python::acquire_gil(); | ||
let py = gil.python(); | ||
|
||
let counter = std::cell::RefCell::new(0); | ||
let counter_fn = | ||
move |_args: &types::PyTuple, _kwargs: Option<&types::PyDict>| -> PyResult<i32> { | ||
let mut counter = counter.borrow_mut(); | ||
*counter += 1; | ||
Ok(*counter) | ||
}; | ||
let counter_py = PyCFunction::new_closure(counter_fn, py).unwrap(); | ||
|
||
py_assert!(py, counter_py, "counter_py() == 1"); | ||
py_assert!(py, counter_py, "counter_py() == 2"); | ||
py_assert!(py, counter_py, "counter_py() == 3"); | ||
} | ||
Comment on lines
+252
to
+269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is interesting. I guess because of the GIL we can agree that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're right and holding the GIL should prevent the boxed function to be called in parallel so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, I'm afraid I think I spoke too quickly, sorry. I've tweaked the let mut counter = Box::new(0);
let counter_fn_obj: Arc<RwLock<Option<Py<PyCFunction>>>> = Arc::new(RwLock::new(None));
let counter_fn_obj_clone = counter_fn_obj.clone();
let counter_fn =
move |_args: &types::PyTuple, _kwargs: Option<&types::PyDict>| -> PyResult<i32> {
let counter_value = &mut *counter;
if *counter_value < 5 {
_args.py().allow_threads(|| {
let counter_fn_obj_clone = counter_fn_obj_clone.clone();
let other_thread = std::thread::spawn(move || Python::with_gil(|py| {
counter_fn_obj_clone.read().as_ref().expect("function has been made").call0(py)
}));
*counter_value += 1;
other_thread.join().unwrap()
})?;
}
Ok(*counter_value)
};
let counter_py = PyCFunction::new_closure(counter_fn, py).unwrap();
*counter_fn_obj.write() = Some(counter_py.into()); So I think to be safe let's go back to just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I think once we switch back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, thanks for checking this thoroughly, just reverted the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
use pyo3::prelude::*; | ||
use pyo3::types::{PyCFunction, PyDict, PyTuple}; | ||
|
||
fn main() { | ||
let fun: Py<PyCFunction> = Python::with_gil(|py| { | ||
let local_data = vec![0, 1, 2, 3, 4]; | ||
let ref_: &[u8] = &local_data; | ||
|
||
let closure_fn = |_args: &PyTuple, _kwargs: Option<&PyDict>| -> PyResult<()> { | ||
println!("This is five: {:?}", ref_.len()); | ||
Ok(()) | ||
}; | ||
PyCFunction::new_closure(closure_fn, py).unwrap().into() | ||
}); | ||
|
||
Python::with_gil(|py| { | ||
fun.call0(py).unwrap(); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
error[E0597]: `local_data` does not live long enough | ||
--> tests/ui/invalid_closure.rs:7:27 | ||
| | ||
7 | let ref_: &[u8] = &local_data; | ||
| ^^^^^^^^^^^ borrowed value does not live long enough | ||
... | ||
13 | PyCFunction::new_closure(closure_fn, py).unwrap().into() | ||
| ---------------------------------------- argument requires that `local_data` is borrowed for `'static` | ||
14 | }); | ||
| - `local_data` dropped here while still borrowed | ||
|
||
error[E0373]: closure may outlive the current function, but it borrows `ref_`, which is owned by the current function | ||
--> tests/ui/invalid_closure.rs:9:26 | ||
| | ||
9 | let closure_fn = |_args: &PyTuple, _kwargs: Option<&PyDict>| -> PyResult<()> { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ may outlive borrowed value `ref_` | ||
10 | println!("This is five: {:?}", ref_.len()); | ||
| ---- `ref_` is borrowed here | ||
| | ||
note: function requires argument type to outlive `'static` | ||
--> tests/ui/invalid_closure.rs:13:9 | ||
| | ||
13 | PyCFunction::new_closure(closure_fn, py).unwrap().into() | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
help: to force the closure to take ownership of `ref_` (and any other referenced variables), use the `move` keyword | ||
| | ||
9 | let closure_fn = move |_args: &PyTuple, _kwargs: Option<&PyDict>| -> PyResult<()> { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so in the closure case the
mod_ptr
is set to the capsule, which makes the capsule object appear as the first argument torun_closure
. That's interesting!