-
Notifications
You must be signed in to change notification settings - Fork 887
rename pyo3::prepare_freethreaded_python
to Python::initialize
#5247
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
Conversation
Thanks for the contribution! Other than needing a newsfragment, what makes this WIP? I'd love to merge for the upcoming release. |
@davidhewitt Some of the tests were failing for me locally on the |
@davidhewitt
The tests are now passing in the However, the tests on the
|
pyo3::prepare_freethreaded_python
to pyo3::initialize_python
pyo3::prepare_freethreaded_python
to pyo3::initialize_python
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.
Did we rule out Python::initialize
? That would sounds intuitive to me and it would be near to the other functions that interact with the runtime like Python::attach
and Python::detach
. Might be nice for discoveribility.
src/interpreter_lifecycle.rs
Outdated
/// ``` | ||
#[cfg(not(any(PyPy, GraalPy)))] | ||
pub fn prepare_freethreaded_python() { | ||
pub fn initialize_python() { |
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 think we should keep the old name with a deprecation added to offer a smoother transition to users.
I agree, it makes sense. I'll need to look into the implementation. |
An issue would be helpful, yes please. |
Nicely done @olp-cs! You may find this helpful: https://rustwiki.org/en/reference/attributes/diagnostics.html#the-deprecated-attribute An example of it in PyO3 can be found here: pyo3/pyo3-ffi/src/intrcheck.rs Line 13 in 7836f15
|
pyo3::prepare_freethreaded_python
to pyo3::initialize_python
pyo3::prepare_freethreaded_python
Thank you! I'll look into this.
I've created it here: #5254 |
@olp-cs just checking to see when you expect to have a chance to return to this? I think the agreement is that Reason why I ask - I'd love to get this into the 0.26 release, which I'd like to publish in the next week or two. I prefer to leave time for contributors to cycle back when they can and fully understand everyone has different availability and resources. In this case this rename really fits being released alongside the rename of If you don't have availability to finish this in the next couple weeks, I totally understand. I can push to this branch and finish this off if you will forgive me 🙏 . |
@davidhewitt Thank you for the explanations! If you don't hear back from me by 30 July, feel free to take it over. Apologies for the delay, I was at another conference right after EuroPython, and I've just came back from Prague. I'll try to get back to this issue within the next couple of days. |
pyo3::prepare_freethreaded_python
pyo3::prepare_freethreaded_python
to Python::initialize
I've updated the PR. Main changes to be reviewed:
|
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.
Thank you very much for coming back round to this! This looks great, with just one tiny nit which I'd prefer adjusted before merge please 🙏
src/interpreter_lifecycle.rs
Outdated
impl Python<'_> { | ||
/// Prepares the use of Python. | ||
/// | ||
/// If the Python interpreter is not already initialized, this function will initialize it with | ||
/// signal handling disabled (Python will not raise the `KeyboardInterrupt` exception). Python | ||
/// signal handling depends on the notion of a 'main thread', which must be the thread that | ||
/// initializes the Python interpreter. | ||
/// | ||
/// If the Python interpreter is already initialized, this function has no effect. | ||
/// | ||
/// This function is unavailable under PyPy because PyPy cannot be embedded in Rust (or any other | ||
/// software). Support for this is tracked on the | ||
/// [PyPy issue tracker](https://github.com/pypy/pypy/issues/3836). | ||
/// | ||
/// # Examples | ||
/// ```rust | ||
/// use pyo3::prelude::*; | ||
/// | ||
/// # fn main() -> PyResult<()> { | ||
/// Python::initialize(); | ||
/// Python::attach(|py| py.run(pyo3::ffi::c_str!("print('Hello World')"), None, None)) | ||
/// # } | ||
/// ``` | ||
pub fn initialize() { |
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.
It's a handy feature of Rust to be able to split impl Type
methods across multiple files, though I generally prefer to avoid it if I can. I think I'd rather we made the function here just an internal free function pub(crate) fn initialize
and then we can add this new Python::initialize
method with all the documentation in src/marker.rs
The Python::initialize
method can then just call crate::interpreter_lifecycle::initialize()
, exactly like the deprecated prepare_freethreaded_python
does just below.
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 think we just raced here 😁
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.
Thanks! This looks pretty good to me, just some minor things from my side
src/interpreter_lifecycle.rs
Outdated
/// Python::attach(|py| py.run(pyo3::ffi::c_str!("print('Hello World')"), None, None)) | ||
/// # } | ||
/// ``` | ||
pub fn initialize() { |
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 think I would prefer if we move this into marker.rs
(where Python
is defined), probably next to attach
would make sense to me. What do you think @davidhewitt
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.
Yep I was thinking the same 😂
@davidhewitt @Icxolu Thank you for your detailed feedback and suggestions! I really appreciate it. I think it’s better structured now, but please let me know if there’s anything else I should change. |
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.
Brilliant, thank you!
Related to #5061
At EuroPython Sprints with @Cheukting