MINOR: Enable GIL monitoring when gilknocker installed

[milesgranger]

After benchmarking with latest gilknocker (0.4.0), it appears to have negligible performance impact (see screenshot of A/B test).

This patch only changes the config to default to enable, but would still require gilknocker to be installed.

• Tests added / passed
• Passes pre-commit run --all-files

Link to full A/B run: https://github.com/coiled/coiled-runtime/actions/runs/4437855278

xref #7290

[github-actions]

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       22 files   -        4         22 suites   - 4   13h 52m 16s ⏱️ + 24m 2s
  3 527 tests  -      25    3 442 ✔️ ±       0       82 💤  -   23  3 ❌  - 2 
38 717 runs   - 6 235  37 166 ✔️  - 5 655  1 547 💤  - 579  4 ❌  - 1 

For more details on these failures, see this check.

Results for commit c942dc9. ± Comparison against base commit 76bbfaf.

This pull request removes 25 tests.

distributed.cli.tests.test_dask_scheduler
distributed.cli.tests.test_dask_ssh
distributed.dashboard.tests.test_components
distributed.dashboard.tests.test_scheduler_bokeh
distributed.dashboard.tests.test_worker_bokeh
distributed.deploy.tests.test_old_ssh
distributed.deploy.tests.test_ssh
distributed.diagnostics.tests.test_progress_stream
distributed.diagnostics.tests.test_widgets
distributed.protocol.tests.test_arrow
…

This pull request skips 1 test.

distributed.tests.test_nanny ‑ test_no_unnecessary_imports_on_worker[pandas]

♻️ This comment has been updated with latest results.

[jrbourbeau]

Thanks @milesgranger. Looks like this PR is being hit by unrelated failures that have been resolved on main. Merging main here should fix things

[milesgranger]

I think that's right for most, but also the gilknocker sampling thread is being left open since (somewhere) the SystemMonitor.close() is missing which then calls gilknocker.KnockKnock.stop(). This results in:

FATAL: exception not rethrown
Fatal Python error: Aborted

Current thread 0x00007fbae56bc640 (most recent call first):
<no Python frame>

I spent most of yesterday trying to solve it from gilknocker's perspective but it appears to be a niche and difficult if not impossible problem to solve from gilknocker's/c-extension side as the Python main thread is being sigabrt'd from underneath the sampling thread which is trying to reacquire the (non-existent) GIL at that point.

I'll try a few more things from that end, but otherwise I'll hunt to see where in the tests it's not being stopped.

[milesgranger]

Okay, I think this is ready for another look. From what I can see, the remaining errors are not related.

I tried a few different things from gilknocker's perspective but it's a tricky and rare bug to catch. Went for the easier solution for now, perhaps later I can devote some time to saving it from gilknocker's side.

---

Edit: Seems there is actually 2 that I could see which were related... :( will poke at it a bit more and ping you when it's ready.

[milesgranger]

@jrbourbeau @fjetter Apologies for the noise, turns out the MINOR wasn't so minor after all. 😅

I'm confident now the remaining failures are unrelated to this patch. And after spending more time that I'd like to admit, found a not-so-awful way to stop the GIL monitoring thread in gilknocker to avoid the niche no Python frame core dumps that were happening when the test suite exited.

For the remaining failures, I can open a PR to try out re-running failed tests using pytest-rerunfailures if you think that may be helpful? EDIT: I see now we're already using it, marked on specific tests.

[hendrikmakait]

Do you want to assert "gil_contention" in a here as well?

[milesgranger]

Thanks @hendrikmakait, after your comment I realized it could be refactored a bit since there is checking the default ON behavior at the top and then OFF behavior at the end of the test. Let me know if you think it should be changed at all.

[hendrikmakait]

Looks great!

[hendrikmakait]

Generally LGTM, thanks, @milesgranger. At a glance, the test failures appear to be unrelated. I have one minor clarification question that's still open.

[phofl]

Looks like this broke the dask testsuite. It looks like stuff breaks when it isn't installed.

https://github.com/dask/dask/actions/runs/4810571135/jobs/8563382185?pr=10227

[jrbourbeau]

Thanks for flagging @phofl. It looks like the mindeps builds here also have a bunch of failures (those builds don't include gilknocker). Will look into this...