[core][aDAG ]Fix unhandled error related issues. #47689
Conversation
| @@ -661,7 +661,7 @@ def __init__( | |||
| # We conservatively set num_shm_buffers to _max_inflight_executions. | |||
| # It means that the DAG can be underutilized, but it guarantees there's | |||
| # no false positive timeouts. | |||
| num_shm_buffers=self._max_inflight_executions, | |||
| num_shm_buffers=1, | |||
There was a problem hiding this comment.
this is unrelated to this PR, but will be merged separately
| from ray.tests.conftest import * # noqa | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
currently I cannot capture logs. wip
| RAY_IGNORE_UNHANDLED_ERRORS_ENV_VAR = "RAY_IGNORE_UNHANDLED_ERRORS" | ||
| RAY_IGNORE_UNHANDLED_ERRORS = env_bool(RAY_IGNORE_UNHANDLED_ERRORS_ENV_VAR, False) |
There was a problem hiding this comment.
these should be prefixed with RAY_ADAG as they are only relevant to adag
There was a problem hiding this comment.
actually this is the same env var as what we have in Ray itself
ray/python/ray/_private/worker.py
Line 799 in ee320aa
imo it is better unifying. I will use the common constant for the env var instead
There was a problem hiding this comment.
Got it. Can you add a comment here: people tend to change this to make names consistent if they are not aware of the reuse.
| try: | ||
| self.get() | ||
| except Exception as e: | ||
| if RAY_IGNORE_UNHANDLED_ERRORS: | ||
| return | ||
|
|
||
| logger.error( | ||
| "Unhandled error (suppress with " | ||
| f"{RAY_IGNORE_UNHANDLED_ERRORS_ENV_VAR}=1'): {e}" | ||
| ) |
There was a problem hiding this comment.
I think python by default ignores the exceptions happen in __del__ and logs to stderr.
What's the difference with this change? Can you post the log before and after the change?
There was a problem hiding this comment.
that will print unnecessary stacktrace (to a destructor) and and an additional message the "exception raised in destructor is ignored". This will have clean error messages
| @@ -2143,6 +2149,7 @@ def teardown(self): | |||
| monitor = getattr(self, "_monitor", None) | |||
| if monitor is not None: | |||
| monitor.teardown(wait=True) | |||
| self._is_teardown = True | |||
Why are these changes needed?
wip tests
Related issue number
Closes #47687
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.