Skip to content

Conversation

@Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Jun 23, 2025

Part of #3987

This renames Python::with_gil to Python::attach to detangle our naming from the GIL which is not always relevant anymore. The old name is deprecated. (Big diff, but pretty much just a mechanical change)

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, I cannot think of a better name and with_gil needs to go, so I think it's time we commit to this 👍

I wonder if it's worth doing a documentation pass (and maybe mentioning this in the migration guide) as a follow up PR to make sure documentation reads well and concepts come across ok following the rename.

@mejrs
Copy link
Member

mejrs commented Jun 24, 2025

Thanks, I cannot think of a better name

What about with? Like, Python::with(|py| {});

@Icxolu
Copy link
Contributor Author

Icxolu commented Jun 24, 2025

What about with? Like, Python::with(|py| {});

That would work too, but it also kind of reads backwards to me (or like something is missing "with what"). The advantages I see with attach are

  • CPython is traversing in this direction, so maybe this makes cross-referencing at bit easier
  • It would work really well with detach if we rename allow_threads as well

I wonder if it's worth doing a documentation pass (and maybe mentioning this in the migration guide) as a follow up PR to make sure documentation reads well and concepts come across ok following the rename.

Yes, I think that would make sense, but probably after detach/allow_threads, if we decide to do this as well.

@davidhewitt
Copy link
Member

What examples can we think of from across the ecosystem? tokio has Runtime::enter which works as a RAII guard.

We couldn't use a RAII guard because the PyGILState_Ensure return values need to be released in the opposite.

Is something like with_attach or attach_and any better?

One drawback of Python::with to me is that it conflicts a bit with the Python with keyword for context managers. I also agree that it reads a little backward, so I'd prefer choose another option.

@davidhewitt
Copy link
Member

Also looks like PEP 788's proposed API for PyThreadState_Ensure wouldn't have the same constraints, so if that's accepted perhaps we could use a RAII guard in the future? 🤔

@Icxolu
Copy link
Contributor Author

Icxolu commented Jun 24, 2025

There's std::thread::scope which is kind of similar. Not sure if I like Python::scope better tho ... I think personally I would prefer an associated method unter Python rather that an freestanding function.

@Icxolu
Copy link
Contributor Author

Icxolu commented Jun 24, 2025

Maybe it's also noteworthy that we already have some APIs that use this terminology like
OnceExt::call_once_py_attached, OnceLockExt::get_or_init_py_attached, and MutexExt::lock_py_attached

@davidhewitt
Copy link
Member

Yep I continue to feel Python::attach is totally fine

@davidhewitt
Copy link
Member

Given the nature of this PR is going to be conflict-heavy and I feel that with_gil needs to go and attach is still the best option we've come up with, can we agree to send this to merge start of next week if nobody comes up with a better alternative before then?

@Icxolu
Copy link
Contributor Author

Icxolu commented Jun 26, 2025

Sounds good to me. Should we also proceed with detach then? I can prepare a followup if we want to do them together.

@davidhewitt
Copy link
Member

Ungh, allow_threads / detach is still in a bit of a mess with #3646. I still haven't found time to really research the implication of breaking all TLS, but it seems painful and a bit sad.

I guess how soon do we expect that to get resolved? It hasn't been for years+ at this point, so while it would be nice to fix, we don't need to rush that. So renaming to detach now seems fine.

Just as a last attempt to spitball and check we've tested out all options, what about Python::call_attached and py.call_detached ?

Given we already have Once::call_once_py_attached, maybe call_attached has a nice connection to this.

@mejrs
Copy link
Member

mejrs commented Jun 27, 2025

I'm fine with attach and detach.

@Icxolu Icxolu added this pull request to the merge queue Jul 1, 2025
Merged via the queue into PyO3:main with commit 2af040b Jul 1, 2025
44 of 45 checks passed
@Icxolu Icxolu deleted the attach branch July 1, 2025 20:16
HerringtonDarkholme added a commit to ast-grep/ast-grep that referenced this pull request Sep 8, 2025
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