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

Update to Pyo3 v0.22 #7

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Update to Pyo3 v0.22 #7

merged 4 commits into from
Oct 11, 2024

Conversation

bschoenmaeckers
Copy link
Contributor

@bschoenmaeckers bschoenmaeckers commented Oct 8, 2024

This is my first attempt at updating to pyo3 v0.22 without the py-clone feature. I've added a Python::with_gil naively everywhere a GIL token was required, so it may need some refactoring & cleanup next. Furthermore all fmt/cargo checks should be resolved.

ref #1

@davidhewitt
Copy link
Member

Thanks! Potentially rebase on main please, and then CI should run :)

@cjdsellers
Copy link
Collaborator

I'll wait for this one to be merged before I do anything else, so we're not stepping on each other 😄

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.

I think this looks good to me, thanks! Just one small correction (I'll apply it) and then let's merge.

src/async_std.rs Outdated Show resolved Hide resolved

if let Err(e) = R::spawn_local(async move {
let result = R::scope_local(
locals2.clone(),
Python::with_gil(|py| locals2.clone_ref(py)),
Copy link
Member

Choose a reason for hiding this comment

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

I think these patterns are all correct for now. It would be interesting to explore further what we can do to reduce calls to Python::with_gil, and the correct way to release the GIL across yield points too.

I think they are follow up areas of research, which probably can yield new APIs both here and upstream in PyO3.

(I guess there's potentially a lot of interesting performance benchmarking to do?)

@davidhewitt davidhewitt marked this pull request as ready for review October 11, 2024 07:42
@davidhewitt davidhewitt merged commit b780910 into PyO3:main Oct 11, 2024
3 of 6 checks passed
@cpu cpu mentioned this pull request Oct 22, 2024
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