Fixes #629 - python GetSubstructureMatch thread safety

[bp-kelley]

Never/ever call the Python API when the GIL is released.

Also releases the GIL during GetSubstructMatches.

Any ideas how to add a test case for this? Do we know when to test threading?

[bp-kelley]

If RDK_THREADSAFE_SSS is not set, should we release the GIL at all? The default is to always release the GIL which seems... optimistic.

[bp-kelley]

I checked the rest of the GIL releases, it appears this one was the only borked one, I'll write some MT tests over the next few days.

[greglandrum]

I've never done enough multi-threading in python to really have much of a grasp of this stuff. When I've wanted to do parallel work, I've tended to use things like multiprocessing.

I believe it only makes sense to release the GIL around substructure matching if RDK_THREADSAFE_SSS is set. If that flag is not set and there is a recursive SMARTS component of the query, it will generate incorrect results if that query is being applied to multiple molecules at the same time.

[greglandrum]

Re: testing
I was not able to come up with a reliable testing scenario that exposed the bug across platforms. We could fix the GIL code, try putting in something basic in terms of testing, and hope for the best?

[greglandrum]

How about modifying the NOGIL definition so that it only does anything if the thread safety flag (RDK_BUILD_THREADSAFE_SSS) is set?

[bp-kelley]

That's my plan.

---

Brian Kelley

On Oct 14, 2015, at 12:25 AM, Greg Landrum notifications@github.com wrote:

In Code/GraphMol/Wrap/Mol.cpp:

 MatchVectType matches;

• SubstructMatch(mol,query,matches,true,useChirality,useQueryQueryMatches);
• {

•  NOGIL gil;
  

  How about modifying the NOGIL definition so that it only does anything if the thread safety flag (RDK_BUILD_THREADSAFE_SSS) is set?

—
Reply to this email directly or view it on GitHub.