To detach ot not detach

[currocam]

I was going through the code and I realised I've been using this with the SharedTableCollection with detach, but

tskit2tskit/examples/haploid_wright_fisher.rs

Lines 170 to 172 in b50c4f3

	// We will allocate the tables via Python allocator.
	// Therefore, we must run in an interpreter with the GiL
	// attached.

My understanding was one could detach "When executing Rust code which does not need to interact with the Python interpreter".

Is this to avoid something like you kill the Python process but Rust doesn't get the signal and you end up with a leak?

Thanks

[molpopgen]

That's a good question -- I don't know. Honestly, I think I assumed that you need a GiL b/c the I assumed memory allocator of a Python interpreter would need to be able to synchronize across threads. (In other words -- I think the rust code that is the constructor is interacting w/the Python interpreter via PyMem_Malloc.)

The way to know will be to make a test and run it in a python-debug interpreter.

Have you run your project in a debug interpreter?

(The other way to check is via valgrind, but that tool is very frustrating to use w/Python.)

[molpopgen]

I just attempted to change the example python module included in this repo and saw the problem: the new method requires being passed a Python interpreter! As expected, trying to allocate within a detatch block and returning the shared tables object from the closure gave errors about pyo3 * not being Send/Sync.

[currocam]

Ahh, I understand now.

Still, one can detach when calling the Rust library from a Python runtime (as that already requires the interpreter). For example, it works well with the sort of #[pyfunction] macro.

https://github.com/currocam/smcprime/blob/6099ef2f584e25dcc34203bc3e3b056bc0a4046a/src/lib.rs#L66-L130

In that sense, I think the comment could be improved to

// We will allocate the tables via Python allocator inside the Rust executable
// Therefore, we must run in an interpreter with the GiL attached. 

Edited: Sorry if this difference was already obvious to you!
Edited2: On a different note, I did try to incorporate valgrind and I gave up after a couple of attempts. Maybe later.

[molpopgen]

Yes, that is a better wording.

You can see the use of detach in the py example in this crate -- allocate first, detach and work with later.

[molpopgen]

The example is exactly what you did.

[molpopgen]

Edited2: On a different note, I did try to incorporate valgrind and I gave up after a couple of attempts. Maybe later.

You need to use a suppressions file for Python. But cargo-valgrind doesn't seem to accept such a thing, making it hard to do things like use valgrind + cargo test.

[currocam]

🤦🏽‍♂️🤦🏽‍♂️🤦🏽‍♂️
My bad. I don't think I saw that. Thanks for your time!

[currocam]

Edited2: On a different note, I did try to incorporate valgrind and I gave up after a couple of attempts. Maybe later.

You need to use a suppressions file for Python. But cargo-valgrind doesn't seem to accept such a thing, making it hard to do things like use valgrind + cargo test.

And thanks for the tip!

[molpopgen]

My bad. I don't think I saw that. Thanks for your time!

Not a worry -- you are right that some of the comments need clarification. I'll try to get to that this week.

[molpopgen]

See #42 -- I think this is a much better way to do this example.