Skip to content
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

Add bindings for PyImport_AddModuleRef and use it in Python::run_code #4529

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

ngoldbaum
Copy link
Contributor

Refs #4265.

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/marker.rs Outdated
@@ -634,14 +634,15 @@ impl<'py> Python<'py> {
locals: Option<&Bound<'py, PyDict>>,
) -> PyResult<Bound<'py, PyAny>> {
unsafe {
let mptr = ffi::PyImport_AddModule(ffi::c_str!("__main__").as_ptr());
let mptr = ffi::compat::PyImport_AddModuleRef(ffi::c_str!("__main__").as_ptr());
Copy link
Member

@mejrs mejrs Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be better to use Bound here (same for the other raw pointers in this function...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on avoiding the manual reference counting. This can change to

Suggested change
let mptr = ffi::compat::PyImport_AddModuleRef(ffi::c_str!("__main__").as_ptr());
let mptr = ffi::compat::PyImport_AddModuleRef(ffi::c_str!("__main__").as_ptr()).assume_owned_or_err(py)?;

It looks like PyModule_GetDict below returns a borrowed reference. Is that problematic under freethreading? I assume so, do we need to call and incref under a critical section? It doesn't look like there is a PyModule_GetDictRef. Should we just do .getattr("__dict__")?

Similarly it looks like PyEval_GetBuiltins is deprecated in 3.13, so I guess we need to update that in a follow up...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call about PyModule_GetDict. I wonder if we don't actually need that on free-threaded builds since we don't need to update globals on Python 3.10 and newer. I'll see if we can avoid it with conditional compilation.

I think we could also probably use PyModule_AddObjectRef or a compat definition based on PyModule_AddObject instead.

@ngoldbaum
Copy link
Contributor Author

Last push refactors the run_code implementation to use smart pointers and only insert __builtins__ into globals on Python <3.10

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's brilliant 👍

@davidhewitt davidhewitt added this pull request to the merge queue Sep 6, 2024
Merged via the queue into PyO3:main with commit 4935033 Sep 6, 2024
41 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants