fix(python): IBA.demosaic had GIL release in wrong spot #4777
Conversation
Only symptomatic on debug builds, but I was getting exceptions from pybind11 about bad GIL release. I traced it to the recent implementations of demosaic. Apparently it's not safe to release the GIL until AFTER the call to py_to_stdvector. Which makes sense when you think about it -- we need the lock whenever we are interacting directly with Python calls or data structures (such as what py_to_stdvector is doing), but we release it when we're done with the python and only calling OIIO internals (such as IBA::demosaic() itself). Signed-off-by: Larry Gritz <[email protected]>
|
I've only seen it symptomatic on my Mac laptop. We obviously run the testsuite in debug mode in CI, not sure why it wasn't showing up. Maybe only certain combos of python version + pybind11 version catch the unsafe behavior and throw an exception for it? |
|
I'm curious why the GIL is released in the first place, it should only ever be released if you want to make the c++ function parallelizable from python (if e.g. you have c++ code to get something from a server) but seeing as demosaic will likely saturate all threads this shouldn't really be necessary. |
|
This allows any parallelization happening in the python program that's calling it to proceed in other threads and not get locked up during the long execution of the IBA function. It's not to parallelize this call, it's to allow other python threads to make progress during the expensive part of this operation. |
|
Right I see, thanks for clearing up! |
…areFoundation#4777) Only symptomatic on debug builds, but I was seeing exceptions from pybind11 about bad GIL release. I traced it to the recent implementations of demosaic. Apparently it's not safe to release the GIL until AFTER the call to py_to_stdvector. Which makes sense when you think about it -- we need the lock whenever we are interacting directly with Python calls or data structures (such as what py_to_stdvector is doing), but we release it when we're done with the python and only calling OIIO internals (such as IBA::demosaic() itself). Signed-off-by: Larry Gritz <[email protected]>
…areFoundation#4777) Only symptomatic on debug builds, but I was seeing exceptions from pybind11 about bad GIL release. I traced it to the recent implementations of demosaic. Apparently it's not safe to release the GIL until AFTER the call to py_to_stdvector. Which makes sense when you think about it -- we need the lock whenever we are interacting directly with Python calls or data structures (such as what py_to_stdvector is doing), but we release it when we're done with the python and only calling OIIO internals (such as IBA::demosaic() itself). Signed-off-by: Larry Gritz <[email protected]>
2 participants
Only symptomatic on debug builds, but I was seeing exceptions from pybind11 about bad GIL release. I traced it to the recent implementations of demosaic. Apparently it's not safe to release the GIL until AFTER the call to py_to_stdvector. Which makes sense when you think about it -- we need the lock whenever we are interacting directly with Python calls or data structures (such as what py_to_stdvector is doing), but we release it when we're done with the python and only calling OIIO internals (such as IBA::demosaic() itself).