Update PyO3 to 0.23

[ngoldbaum]

Opening as a draft to see how CI feels about this change and because lock().unwrap() may swallow panics incorrectly.

[lgray]

@ngoldbaum I'm really interested in helping get this out, we use cramjam in our data analysis stack and I'd like to finally be able to use multithreading as opposed to multiprocessing. Is there any way I can help?

The current error seems a bit mysterious, but I'll take a crack at reproducing.

[lgray]

Hmm, that test passed on my laptop (osx-arm64), so either flaky test or os-dependent?

[lgray]

Oh I see these failures are possibly related to #201 and #190, as the issue is in test_variants.

[ngoldbaum]

Thanks for the help! Now that I know someone is blocked on this I'll get back to working on this library next week.

[martindurant]

I have been involved in this repo in the past, but not much recently. So I don't know the current state of the code well, but maybe I can help too.

[ngoldbaum]

I spent some time over at #201. @milesgranger do you have any opinions about how to deal with CI being broken? Pin hypothesis for now maybe?

[milesgranger]

Can't thank you enough for helping out here @ngoldbaum, pinning hypothesis seems good for now.

[ngoldbaum]

woohoo, green CI :)

[ngoldbaum]

Before merging this I should do one final pass to make sure the unwrap() calls are safe. They're problematic if the mutex might get poisoned.

[milesgranger]

If it eases things, I'm not adverse to adding parking_lot::Mutex

[lgray]

Just FYI this still complains on import about not being marked as freethreaded-safe and reenables the GIL on import. Is that what we intend to have happen?

[ngoldbaum]

Just FYI this still complains on import about not being marked as freethreaded-safe and reenables the GIL on import. Is that what we intend to have happen?

Yes, I still need to formally audit the codebase and add free-threaded support and CI. I’d also like to add some explicitly multithreaded tests. That will be a followup. After that is merged I’ll work on the wheel builds in a third PR.

[lgray]

OK, wanted to check! Sounds good to me.

[ngoldbaum]

So I think all the lock().unwrap() is fine - if there are any possible panics while waiting for the lock then they should be propagated to the user. PyO3's panic handler should then convert the panic into a Python exception. And indeed that's exactly what happens when I intentionally insert a panic into lz4::Compressor::compress:

diff --git a/src/lz4.rs b/src/lz4.rs
index f612ec9..614ee09 100644
--- a/src/lz4.rs
+++ b/src/lz4.rs
@@ -237,7 +237,9 @@ pub mod lz4 {

         /// Compress input into the current compressor's stream.
         pub fn compress(&mut self, input: &[u8]) -> PyResult<usize> {
-            crate::io::stream_compress(&mut self.inner.lock().unwrap(), input)
+            let inner = &mut self.inner.lock().unwrap();
+            panic!("Should be a python exception");
+            crate::io::stream_compress(inner, input)
         }

FAILED tests/test_variants.py::test_streams_compressor[lz4] - pyo3_runtime.PanicException: Should be a python exception

So I think we're good to go. @milesgranger go ahead and merge when ready :)

[milesgranger]

Thank you, @ngoldbaum! 🎉