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

GIL is held while marching #67

Closed
uekstrom opened this issue Apr 7, 2022 · 3 comments
Closed

GIL is held while marching #67

uekstrom opened this issue Apr 7, 2022 · 3 comments

Comments

@uekstrom
Copy link

uekstrom commented Apr 7, 2022

It would be nice if the marching could be done while the interpreter lock is released, to allow skfmm to run in its own thread. I assume it's not as simple as

diff --git a/skfmm/fmm.cpp b/skfmm/fmm.cpp
index 351a51d..3ae18cf 100644
--- a/skfmm/fmm.cpp
+++ b/skfmm/fmm.cpp
@@ -304,7 +304,9 @@ static PyObject *distance_method(PyObject *self, PyObject *args)
   }
 
   try {
+      Py_BEGIN_ALLOW_THREADS
       marcher->march();
+      Py_END_ALLOW_THREADS
       error = marcher->getError();
       delete marcher;
     } catch (const std::exception& exn) {

because the marcher works on memory owned by Python objects, but I don't know enough to write a complete solution

@jkfurtney
Copy link
Member

Thanks for the note on this, there was some discussion of this previously in issue #36 . The conclusion at the time was to leave things as they were and rely on multiprocessing for parallelism. I still tend toward this as shared memory parallelism for computational bottlenecks is tricky, c-Python is not really designed for it, and there could be unforeseen consequences.

Do you have a use case where multiprocessing would not work? If there is a big demand for this use case we could revisit this decision. As discussed in #36 we could add an argument that would make copies of all the data and release the GIL during the computational intensive part.

@uekstrom
Copy link
Author

Thanks for your reply! The use case is large 3d calculations where multiprocessing is too slow and/or uses too much memory. Multiprocessing also does not work well in combination with some other big threaded libraries we use. On Windows (or other platforms using 'spawn' instead of 'fork') these issues are more problematic. In #36 you discuss the option "Release the GIL without copying the parameters, and leave on the programmer the responsibility for thread safeness. It looks like a bad idea, and is what my previous commit does." This is exactly what I would like, maybe controlled by an allow_threading argument? I would argue that this is much easier to understand and deal with than multiprocessing.

Perhaps it is possible to guarantee that the solver still terminates even if the array content is modified during the run? Copying the input would not work for me, I would run out of memory.

@jkfurtney
Copy link
Member

I have thought more about this and I am just not comfortable releasing a version of the library that is not thread safe even if it is an option and is explained in the documentation. It has been my experience that threading and reference counting are really difficult to get correct and that just releasing the GIL would lead to Heisenbugs and unexpected consequences. Other than forcing a copy of the data scikit-fmm uses, I am not sure what the options are, do you have examples of how other Python libraries handle this?

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

No branches or pull requests

2 participants