Unhandled exception handler based on local ref counting by ericl · Pull Request #14049 · ray-project/ray

ericl · 2021-02-10T22:47:04Z

Why are these changes needed?

Instead of suppressing remote task errors based on a leaky heuristic, use ref counting to report only the errors that are truly unhandled.

This also adds an env var RAY_IGNORE_UNHANDLED_ERRORS that can be used to suppress the messages.

Related issue number

Closes #12018

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

ericl · 2021-02-10T22:48:30Z

python/ray/_raylet.pyx

+            metadata = Buffer.make(error.GetMetadata()).to_pybytes()
+        results = worker.deserialize_objects(
+            [(data, metadata)], [ray.ObjectRef.nil()])
+        print("DESERIALIZATION DONE")


@edoakes any idea why deserialization would succeed (deserialize_objects prints "DESER RESULT..." ), but then we never get to this second print?

The output I get is

DESERIALIZATION RESULT [RayTaskError('__main__.f', 'Traceback (most recent call last):\n File "python/ray/_raylet.pyx", line 487, in ray._raylet.execute_task\n File "test.py", line 7, in f\n raise ValueError("hi")\nValueError: hi\n', <class 'ValueError'>)] terminate called without an active exception zsh: abort python test.py

I have no idea why you're not hitting the 2nd print, but the only time I've seen this error message is when a c++ thread goes out of scope without joining it or detaching it. If this is being called on a callback from the core worker there might be something weird happening with the lifetime management in c++

stephanie-wang · 2021-02-12T20:09:41Z

src/ray/core_worker/store_provider/memory_store/memory_store.cc

  auto iter = objects_.find(object_id);
  if (iter != objects_.end()) {
    auto obj = iter->second;
+    obj->SetAccessed();


Why do we need the accessed flag? I thought it would be enough to just throw the error below, if we Put the value but there's no ref to the object.

Oh I see, for cases where the ref is still in scope but never used?

Not sure I understand your comment, this is just recording the access.

stephanie-wang

Can we add a unit test for the memory store? This is long overdue but for now we could just test that the callback gets triggered correctly.

stephanie-wang · 2021-02-12T20:13:26Z

src/ray/core_worker/store_provider/memory_store/memory_store.cc

+      check_signals_(check_signals),
+      unhandled_exception_handler_(unhandled_exception_handler) {}

 void CoreWorkerMemoryStore::GetAsync(


Should we also SetAccessed here?

src/ray/core_worker/store_provider/memory_store/memory_store.cc

edoakes

Nice, really good change both from UX and design standpoint

edoakes · 2021-02-12T22:44:44Z

python/ray/_raylet.pyx

+        # We need to release the gil since object destruction may call the
+        # unhandled exception handler.


If this is the only time that the unhandled exception handler is called, should we just hold the gil through the whole call to RemoveLocalReference and not acquire it in the handler?

Sadly we need to hold it during the call to .native().

edoakes · 2021-02-12T22:45:34Z

python/ray/tests/test_failure.py

+    num_exceptions = 0
+
+    def interceptor(e):
+        nonlocal num_exceptions


i've legitimately never seen nonlocal before.... interesting

edoakes · 2021-02-12T22:47:43Z

python/ray/worker.py

+    logger.error("Unhandled error (suppress with "
+                 "RAY_IGNORE_UNHANDLED_ERRORS=1): {}".format(e))


Instead of unhandled error it might be clearer if we explicitly say something like:
Exception from ObjectRef that was never retrieved with ray.get()

Yeah it's kind of long though... I don't suppose "ungetted error" is a real word?

edoakes · 2021-02-12T22:48:31Z

python/ray/worker.py

+                # TODO(ekl) remove task push errors entirely now that we have
+                # the separate unhandled exception handler.


ericl · 2021-02-13T01:13:44Z

Comments addressed.

…project#14049)" This reverts commit 9dc671a.

…)" (#14099) This reverts commit 9dc671a.

…ng (ray-project#14049)" (ray-project#14099)" This reverts commit b45ae76.

#14113) * Revert "Revert "Unhandled exception handler based on local ref counting (#14049)" (#14099)" This reverts commit b45ae76. * reomve test * fix * fix

…14049)

…project#14049)" (ray-project#14099) This reverts commit 9dc671a.

ray-project#14113) * Revert "Revert "Unhandled exception handler based on local ref counting (ray-project#14049)" (ray-project#14099)" This reverts commit b45ae76. * reomve test * fix * fix

…ng (ray-project#14049)" (ray-project#14099)" This reverts commit 31231f1.

…project#14049)" This reverts commit 64783d3.

ericl added 3 commits February 10, 2021 13:45

on erase

9b32908

exception handler

11cc39b

wip

f509967

ericl commented Feb 10, 2021

View reviewed changes

ericl added 6 commits February 10, 2021 15:28

wip

c77a00b

update

c38e77d

update

f367bfc

typo

4676951

update

aaf542e

lint

2b7be2b

ericl mentioned this pull request Feb 11, 2021

[Core] [Error Message] Possible unhandled error from worker #12018

Closed

ericl added 3 commits February 10, 2021 16:24

add test

a92b161

lint

3d2e94e

ignore promoted objects

fb0ba95

ericl changed the title ~~[WIP] Unhandled exception handler based on local ref counting~~ Unhandled exception handler based on local ref counting Feb 11, 2021

ericl assigned rkooo567 and edoakes and unassigned rkooo567 Feb 11, 2021

ericl added 2 commits February 11, 2021 11:19

remove more

1856035

remove

561ffc4

ericl assigned stephanie-wang Feb 12, 2021

ericl added 3 commits February 11, 2021 17:40

fix

3c3e924

wip

4d6283d

Merge remote-tracking branch 'upstream/master' into error-handling

72c0cc0

stephanie-wang reviewed Feb 12, 2021

View reviewed changes

edoakes approved these changes Feb 12, 2021

View reviewed changes

ericl added 2 commits February 12, 2021 17:04

add unit test

5e52848

fix bug

84a921b

ericl merged commit 9dc671a into ray-project:master Feb 13, 2021

rkooo567 added a commit to rkooo567/ray that referenced this pull request Feb 15, 2021

Revert "Unhandled exception handler based on local ref counting (ray-…

fe9e4d7

…project#14049)" This reverts commit 9dc671a.

ericl pushed a commit that referenced this pull request Feb 15, 2021

Revert "Unhandled exception handler based on local ref counting (#14049…

b45ae76

…)" (#14099) This reverts commit 9dc671a.

ericl added a commit to ericl/ray that referenced this pull request Feb 15, 2021

Revert "Revert "Unhandled exception handler based on local ref counti…

ad9df6d

…ng (ray-project#14049)" (ray-project#14099)" This reverts commit b45ae76.

ericl mentioned this pull request Feb 15, 2021

Revert "Revert "Unhandled exception handler based on local ref counti… #14113

Merged

6 tasks

oliverhu mentioned this pull request Feb 16, 2021

[core] remove pushing RayTaskError to driver #13512

Closed

6 tasks

fishbone pushed a commit to fishbone/ray that referenced this pull request Feb 16, 2021

Unhandled exception handler based on local ref counting (ray-project#…

64783d3

…14049)

fishbone pushed a commit to fishbone/ray that referenced this pull request Feb 16, 2021

Revert "Unhandled exception handler based on local ref counting (ray-…

31231f1

…project#14049)" (ray-project#14099) This reverts commit 9dc671a.

fishbone added a commit to fishbone/ray that referenced this pull request Feb 16, 2021

Revert "Revert "Unhandled exception handler based on local ref counti…

d0f4f69

…ng (ray-project#14049)" (ray-project#14099)" This reverts commit 31231f1.

fishbone added a commit to fishbone/ray that referenced this pull request Feb 16, 2021

Revert "Unhandled exception handler based on local ref counting (ray-…

52042d1

…project#14049)" This reverts commit 64783d3.

architkulkarni mentioned this pull request Feb 16, 2021

[Tune] Disable dcgan tests #14131

Closed

6 tasks

stephanie-wang mentioned this pull request Apr 13, 2021

Unhandled error messages aren't printed until next interaction with shell #14485

Closed

		# We need to release the gil since object destruction may call the
		# unhandled exception handler.

		logger.error("Unhandled error (suppress with "
		"RAY_IGNORE_UNHANDLED_ERRORS=1): {}".format(e))

		# TODO(ekl) remove task push errors entirely now that we have
		# the separate unhandled exception handler.

Conversation

ericl commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanie-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edoakes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericl commented Feb 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericl commented Feb 10, 2021 •

edited

Loading