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

Remove is_python_shutting_down #8492

Merged

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Feb 5, 2024

This PR binds sys.is_finalizing to make sure that it's always available during interpreter shutdown and removes the (to the best of my knowledge superfluous) is_python_shutting_down.

This handles errors during shutdown like

Exception ignored in: <function Future.__del__ at 0x6f23b140ccc0>
Traceback (most recent call last):
  File "/usr/local/python/python3/std/lib64/python3.11/site-packages/distributed/client.py", line 510, in __del__
TypeError: 'NoneType' object is not callable
  • Tests added / passed
  • Passes pre-commit run --all-files

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Unit Test Results

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

    29 files  ±0      29 suites  ±0   11h 18m 15s ⏱️ + 3m 1s
 4 080 tests ±0   3 958 ✅ ±0    112 💤 ±0   9 ❌ ±0  1 🔥 ±0 
55 188 runs   - 1  52 741 ✅  - 2  2 435 💤 ±0  11 ❌ +1  1 🔥 ±0 

For more details on these failures and errors, see this check.

Results for commit b9b1678. ± Comparison against base commit 50700f3.

♻️ This comment has been updated with latest results.

@hendrikmakait hendrikmakait changed the title [WIP] Avoid is_python_shutting_down Avoid is_python_shutting_down Jun 27, 2024
@hendrikmakait hendrikmakait marked this pull request as ready for review June 27, 2024 08:54
@hendrikmakait hendrikmakait changed the title Avoid is_python_shutting_down Remove is_python_shutting_down Jun 27, 2024
@hendrikmakait
Copy link
Member Author

cc @rjzamora in case RAPIDS uses is_python_shutting_down somewhere internally.

@hendrikmakait hendrikmakait marked this pull request as draft June 27, 2024 09:34
@hendrikmakait hendrikmakait marked this pull request as ready for review June 28, 2024 10:26
if is_python_shutting_down():

# Are we shutting down the process?
if self._is_finalizing() or not threading.main_thread().is_alive():
Copy link
Member Author

@hendrikmakait hendrikmakait Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't great, but it's the safest way of checking whether the interpreter is shutting down. More concretely, whether it's safe to use threads. Threads are being shutdown before atexit hooks so we need a mechanism like is_python_shutting_down that becomes the first atexit hook in distributed and lets us know whether we have entered shutdown, or we need to propagate that knowledge during shutdown, or we need to check whether it's safe to use threads where necessary (as done here).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what a mess. I'm fine with this

@hendrikmakait hendrikmakait merged commit 147c505 into dask:main Jul 1, 2024
23 of 34 checks passed
@hendrikmakait hendrikmakait deleted the avoid-is-python-shutting-down branch July 1, 2024 13:06
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

Successfully merging this pull request may close these issues.

None yet

2 participants