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

Clone (or alternative) on GILOnceCell #4428

Closed
jakelishman opened this issue Aug 9, 2024 · 4 comments · Fixed by #4511
Closed

Clone (or alternative) on GILOnceCell #4428

jakelishman opened this issue Aug 9, 2024 · 4 comments · Fixed by #4511

Comments

@jakelishman
Copy link
Contributor

jakelishman commented Aug 9, 2024

I have a pyclass struct that wants to contain a GILOnceCell<Py<PyAny>> for caching of a lazily constructed Python object, where the initialiser yields control to the Python interpreter so runs the risk of re-entrancy problems with regular OnceCell.

I'd originally tried to derive Clone on my struct (I'm still using py-clone for the time being, but working to disentangle our data structures), which if I understand correctly must fail for GILOnceCell because it requires the GIL to mediate both gets and sets. For the time being, I think the correct course of action for me for the clone is to manually GILOnceCell::new(other_cell.get(py).map(|ob| ob.clone_ref(py))) (or similar).

edit: if there's an A/B problem here, and there's something better I could be doing with this kind of cache, I'm totally open to alternatives to the answer being "that's not the right way to do this".

Would it be possible to either:

  • add an impl<T> GILOnceCell<Py<T>> { pub fn clone_ref(&self, py: Python); } method doing what I sketched?
  • give GILOnceCell the proposed derive macro for this kind of "clone with the GIL", if that is what comes out of Add trait for cloning while the GIL is held #4133?
@davidhewitt
Copy link
Member

This is a great question. The clone_ref function you propose seems relatively fine to add, to me.

A related question is what to do about GILOnceCell in its entirety when it no longer makes sense in freethreaded Python (see #4265). It would be nice to find a solution that works for both GIL and freethreaded builds. Does that mean removing this type and replacing it with different semantics? Hopefully we will have an answer soon...

@jakelishman
Copy link
Contributor Author

Thanks for the response, and sorry for disappearing for a long while there! I made #4511 with the top bullet, but happy to change it however needed.

For GILOnceCell in free-threaded Python: naively to me, it seems like GILOnceCell simply can't exist (or at least can't be Send) in a free-threaded build because the GIL no longer does the mediation the type relies on. I know we're intending to find a new solution to holding shared Rust/Python data in our library (Qiskit) when we start worrying about supporting free-threading, but it's just one on a fairly large pile of things preventing us from enabling GIL-free Python-space threading, so for now I'm sticking my head in the sand! We haven't even managed to move off py-clone yet, and that's going to be enough of an ordeal (of our own making) for us.

@davidhewitt
Copy link
Member

I think for py-clone you can get a long way by replacing PyObject with Arc<PyObject>>, as that gives you the clone-without-the-GIL back. Maybe this should be in the migration guide...

@davidhewitt
Copy link
Member

For freethreaded GILOnceCell, I'd welcome any opinions on #4512 and #4513

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants