Remove redundant gil acquisition.

[robertwb]

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

[rth]

Looks like the previous with gil was necessary if self.func is a python function.. Were the tests passing on your local computer?

[raghavrv]

like @rth pointed out, we need a test to see if this works without hiccups when self.func is a python object.

Additionally we also need a test to see if TypeError gets raised properly

[raghavrv]

When you raise a python error, you need the gil.

[raghavrv]

Ah the function is defined with gil but isn't it the default mode. I'm not sure why we are defining it with gil.

[raghavrv]

Ah it's @robertwd from cython ;) I'll let you handle this. Could you kindly update the PR description so we understand your PR better?

[GaelVaroquaux]

Ah it's @robertwd from cython ;)

Yes. I am very very happy to see him looking at our cython codebase. Thanks @robertwd for Cython. We love it!

[robertwb]

Hmm... there's something odd here with the inline method being declared "with gil" which should make the inner one redundant. Another try.

[lesteve]

@robertwb I get a compilation error with 0.26b0 (see #9272 (comment)) which I think is exactly related to this PR. Have you managed to get to the bottom of this ?

[amueller]

Just also ran into this. Looks like we are both planning to release, we should fix this...

[glemaitre]

Just to summarize a bit the code base.

There is the base class DistanceMetric defining cdef dist(...) nogil.
PyFuncDistance is subclassing DistanceMetric but overridedist with with gil since dist get a func which is a python argument. For cython < 0.25, the gil needs to be acquired in the method dist itself as well while this leads to the redundant GIL acquisition error for version 0.26b0.

[scoder]

The final change looks right to me. If all methods in the hierarchy are declared with the same nogil signature, then the one in PyFuncDistance can happily acquire the GIL in its body if/where it needs it. That should work in all recent Cython versions.

[scoder]

Regarding the compile failure due to Python object variables being used, I guess you'd have to hack in a call to a new (ìnline) method that is not inherited and can thus be declared with gil, and then move the body of the current PyFuncDistance.dist() method over to that method, so that the inherited dist() nogil method only calls that with gil method and nothing else. Sorry for that inconvenience.

Note that this is only needed for compatibility with older Cython versions, as the latest should support the with gil signature declaration on the overridden method (i.e. wouldn't require the with gil block in the body any more). The fact that you previously needed both was a bug (if I understand this pull request correctly).

[glemaitre]

The fact that you previously needed both was a bug (if I understand this pull request correctly).

It was looking like a bug. Thanks for the hack. I'll try that.

edit: It seems to work ;)

[TomDLT]

Fixed in #9311