Suppress automatic GIL acquire to avoid deadlock

[wphicks]

Explicitly disable Cython GIL acquire on entry into logging callbacks to avoid deadlock in multi-threaded C code which calls callback

Close #3027

[GPUtester]

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

[wphicks]

The one thing this PR is missing is a good unit test for the problem being fixed. It's tricky because it would require us to set up a multi-threaded C function that calls the logging, compile that, and then call that function from Python. Any thoughts on a clean way to test this would be very welcome.

[dantegd]

change lgtm

[dantegd]

rerun tests

[wphicks]

rerun tests

[codecov-io]

Codecov Report

Merging #3037 into branch-0.17 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           branch-0.17    #3037   +/-   ##
============================================
  Coverage        59.26%   59.26%           
============================================
  Files              142      142           
  Lines             8962     8962           
============================================
  Hits              5311     5311           
  Misses            3651     3651           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3647695...8ade1c6. Read the comment docs.

[wphicks]

Affected by same issue as 3038. Looks like the extra xgboost package is gone, so I'll rerun this one more time.

[wphicks]

rerun tests

[vinaydes]

The one thing this PR is missing is a good unit test for the problem being fixed. It's tricky because it would require us to set up a multi-threaded C function that calls the logging, compile that, and then call that function from Python. Any thoughts on a clean way to test this would be very welcome.

As you said, it is difficult to write test for this scenario. However I think we should add multi-threaded logging statements to logger unit test file. Something like:

#pragma omp parallel for n_threads(5)
for(int i = 0; i < 5; i++) {
  CUML_LOG_INFO("Testing multi-threaded logging from thread: %d", omp_get_thread_num());
}

This will ensure that we capture hang in logging if there is one.

[wphicks]

@vinaydes I don't think that's a bad idea, but unfortunately it wouldn't have caught this particular bug. CUML_LOG_INFO and its brethren are a relatively thin wrapper on spdlog. Is there enough complexity in the logic of those wrappers that this would test something non-trivial beyond what spdlog already guarantees with its own unit tests? Certainly not trying to argue against it, just making sure I understand the value-add.

Either way, if it's all right by you, I think let's add that in a separate PR, since it's not a test of this particular change.

[vinaydes]

@wphicks Yeah, you are right. It would only work if we also write a Python test which calls the C++ test code. We can leave it for another PR.