Don't try to reconnect client on interpreter shutdown#6120
Don't try to reconnect client on interpreter shutdown#6120
Conversation
|
I'm inclined to merge tomorrow. @crusaderky , do you have a moment to review? |
Unit Test Results 16 files ±0 16 suites ±0 7h 47m 8s ⏱️ + 27m 21s For more details on these failures, see this check. Results for commit 9917cdc. ± Comparison against base commit 19b50d7. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
The import design is making me uneasy. It works, but if, in the future, an over-eager developer will move the import from inside the function to the top of the distributed.utils module, this will break and no tests will trip.
Could you move the atexit.register function from distributed/__init__.py to distributed/utils.py? This way, any module that contains from distributed.utils import is_python_closing in its header will guarantee that the atexit is registered well ahead of shutdown time.
Tested that this PR fixes the issue.
|
I like the idea, however, for some reason this change (which I think should do the job) fails to work. diff --git a/distributed/__init__.py b/distributed/__init__.py
index 0d116516..e4a4be95 100644
--- a/distributed/__init__.py
+++ b/distributed/__init__.py
@@ -1,8 +1,6 @@
from distributed import config # isort:skip; load distributed configuration first
from distributed import widgets # isort:skip; load distributed widgets second
-import atexit
-
import dask
from dask.config import config # type: ignore
@@ -74,18 +72,3 @@ def __getattr__(name):
return __git_revision__
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
-
-
-_python_shutting_down = False
-
-
-@atexit.register
-def _():
- """Set a global when Python shuts down
-
- See Also
- --------
- distributed.utils.is_python_shutting_down
- """
- global _python_shutting_down
- _python_shutting_down = True
diff --git a/distributed/utils.py b/distributed/utils.py
index 24fab898..ce5f8ac3 100644
--- a/distributed/utils.py
+++ b/distributed/utils.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import asyncio
+import atexit
import contextvars
import functools
import importlib
@@ -1610,8 +1611,21 @@ def recursive_to_dict(
tok.var.reset(tok)
+_python_shutting_down = False
+
+
def is_python_shutting_down() -> bool:
"""Is the interpreter shutting down now?"""
- from distributed import _python_shutting_down
-
return _python_shutting_down
+
+
+@atexit.register
+def _():
+ """Set a global when Python shuts down
+
+ See Also
+ --------
+ distributed.utils.is_python_shutting_down
+ """
+ global _python_shutting_down
+ _python_shutting_down = True
|
|
I'm feeling some time pressure to get this in soonish so that we have the option to release Friday. @crusaderky are your comments blockers, or can we resolve them later? |
|
Happy to merge with the 3 suggestions above |
Co-authored-by: crusaderky <crusaderky@gmail.com> Co-authored-by: Florian Jetter <fjetter@users.noreply.github.com>
|
I saw a couple of unrelated intermittent failures on this PR. I don't think that this is likely to cause any issues, but it's strange enough that I'm rerunning everything for good measure to see if it's a pattern. |
pre-commit run --all-files