Improving message for access to unknown objects #28209

pabloem · 2022-08-31T18:34:54Z

Signed-off-by: Pablo pabloem@users.noreply.github.com

Why are these changes needed?

Consider this scenario:


ray.init()
my_objref = ray.put('MY TEXT')
ray.shutdown()

ray.init()

@ray.remote
def my_fn(obj):
  print(obj)

my_fn.remote(my_objref)

In this scenario, Ray will throw this error and kill the Python interpreter (without erroring out!). I've tried to rephrase the error to be a little easier to figure out and fix, and changed the check so that an error will be thrown rather than ending the whole process.

Changes to Ray APIs

The best overview you can get is by checking the changes to test_ray_shutdown.py. The behavior changes in the following scenarios:

First consider the following set up:

ray.init()
my_ref = ray.put("anystring")

@ray.remote
def f(s):
    print(s)

ray.shutdown()

ray.init()

In this case, my_ref is an ObjRef that exists from a previous Ray session, and start a new ray session that does not know this object. Without this change, the behavior is:

Call	Result
`f.remote([my_ref])`	This would cause the Python interpreter to die
`f.remote(my_ref)`	This would cause the Python interpreter to die
`ray.get(my_ref, timeout=30)`	This would cause Ray to throw a Timeout exception
`ray.get(my_ref)`	This would cause Ray to hang indefinitely
`ray.wait([my_ref])`	This would cause Ray to hang indefinitely
`ray.wait([my_ref], timeout=30)`	This would cause Ray to throw a timeout
`ray.wait([ray.put("something"), my_ref])`	This would cause Ray to always return ObjRef("something")
`ray.wait([ray.put("something"), my_ref], num_returns=2)`	This would cause Ray hang indefinitely

With these changes, the behavior for each call is now:

Call	Result
`f.remote([my_ref])`	Throws `ValueError` with the expected explanation
`f.remote(my_ref)`	Throws `ValueError` with the expected explanation
`ray.get(my_ref, timeout=30)`	Throws `ValueError` with the expected explanation
`ray.get(my_ref)`	Throws `ValueError` with the expected explanation
`ray.wait([my_ref])`	Throws `ValueError` with the expected explanation
`ray.wait([my_ref], timeout=30)`	Throws `ValueError` with the expected explanation
`ray.wait([ray.put("something"), my_ref])`	This would cause Ray to always return ObjRef("something")
`ray.wait([ray.put("something"), my_ref], num_returns=2)`	Throws `ValueError` with the expected explanation

I would argue that this change is preferable from the previous behavior because:

Python interpreter dying is never an expected outcome for a Python application.
Rather than hanging indefinitely, users now can know that the object they're requesting will never be available

That's it, I think.

(FYI on above section @stephanie-wang @jjyao)

Related issue number

I have not created an issue to track this.

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
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 :(

pabloem · 2022-09-07T16:58:23Z

This is meant to fix #28211 - how can I run the test I added to verify that?

r: @jjyao

pabloem · 2022-09-07T17:01:45Z

I also added a test for #28341, but I have not added a fix

pabloem · 2022-09-07T18:30:13Z

hm I see this does not raise an error either. How can I raise a Python error from C? : )

richardliaw · 2022-09-07T19:26:04Z

@jjyao could you shepherd this?

jjyao · 2022-09-08T07:06:47Z

Sorry for the late review. I'll take a look tomorrow!

pabloem · 2022-09-08T14:55:59Z

question:
I'm running the following:

bazel build -- //:ray_pkg //cpp:ray_cpp_pkg
pip install -e python/.
python -m pytest -v -s python/ray/tests/test_ray_shutdown.py

This works for my purposes, but it won't build Ray with all the compilation checks (e.g. -Wunused-result) - do you know what I might need for that?

pabloem · 2022-09-08T21:08:31Z

ok at this point the failing tests seem like unrelated flakes. LMK if I should look into it!

(also, I'll sign the commit on the next review cycle)

jjyao · 2022-09-09T04:33:15Z

I actually think the check failure is fine here: you should never share the object ref out of band across clusters. If you do that then it's a bug in the application code and the check failure basically tells you to fix the application code. IMO, it probably doesn't worth the effort to propagate up a catchable exception (it doesn't make much sense to catch and recover from it, we should just fix the application code).

Maybe we should just improve the check failure message? WDYT @stephanie-wang ?

pcmoritz · 2022-09-09T04:58:06Z

Hmm, it seems to me that having a Python program just terminate the interpreter is always bad and raising and exception is much preferable. I haven't reviewed the PR in detail but it doesn't seem to introduce that much complexity. So we should probably merge it.

While it doesn't make sense to catch the exception and recover from it, raising an exception is always better than just terminating the interpreter. Python is a safe language, so it shouldn't crash.

pabloem · 2022-09-09T06:46:48Z

Thanks! Right, I don't think the python interpreter should ever just terminate like that, but rather throw a Python exception that will tell the application what is the problem. I've added a couple small tests that you can try in Ray without this change and see how the behavior without the change is at least unintuitive

(p.s. pushed signed-off commit)

jjyao · 2022-09-09T16:21:55Z

Ok, then I think we should check all the public apis that accept obj refs or nested obj refs and make sure they all throw the exception instead of check failure.

pabloem · 2022-09-09T20:33:42Z

I've created a GetOwnerAddressOrDie method which will preserve the existing behavior for untested paths, and used GetOwnerAddress for paths that we can test. Do you think we could review the current scope of changes and have a follow-up item to review other API paths that may cause CPython death afterwards? : )

jjyao · 2022-09-09T16:29:52Z

python/ray/_raylet.pyx

+                    c_arg, c_owner_address)
+            if not op_status.ok():
+                raise PlasmaObjectNotAvailable(
+                    'Object %r could not be found in this Ray session.' % arg)


Could we add possible reasons why this can happen to the message just like the check failure message?

jjyao · 2022-09-09T16:32:19Z

python/ray/_raylet.pyx

-        return CCoreWorkerProcess.GetCoreWorker().GetOwnerAddress(
-                c_object_id).SerializeAsString()
+            CAddress c_owner_address
+        CCoreWorkerProcess.GetCoreWorker().GetOwnerAddress(


We don't need to check status here?

I've added the check there. This should be more reasonable, huh?

jjyao · 2022-09-09T16:49:28Z

python/ray/_raylet.pyx

+            op_status = CCoreWorkerProcess.GetCoreWorker().GetOwnerAddress(
+                    c_arg, c_owner_address)
+            if not op_status.ok():
+                raise PlasmaObjectNotAvailable(


I think this exception is for a different purpose based on its comment. Should it just be ValueError given that we pass invalid objects to the functions?

that makes sense. done.

jjyao · 2022-09-09T17:07:00Z

python/ray/tests/test_ray_shutdown.py

+        appropriate_exception_thrown
+    ), "ray.get is hanging on unknown object retrieval"
+
+


Could we add a similar test for ray.wait() as well

added one. please make sure to check on the behavior and lmk if it makes sense to you.

python/ray/tests/test_ray_shutdown.py

jjyao · 2022-09-13T17:57:33Z

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

@@ -279,13 +279,30 @@ Status CoreWorkerMemoryStore::Get(const std::vector<ObjectID> &object_ids,
                 /*abort_if_any_object_is_exception=*/true);
 }

+Status CoreWorkerMemoryStore::ObjectHasOwner(const ObjectID &object_id,


I feel a bool is enough here based on the function name?

Also I feel we can expose an ObjectHasOwner method in core_worker and do the validation at the beginning of public apis like ray.get() (normally where we do function inputs validation) so the remaining code can just assume the objects are always valid and we have a single logic deciding whether an object is valid. WDYT?

jjyao · 2022-09-13T18:03:10Z

python/ray/tests/test_ray_shutdown.py

+    exception_thrown = False
+    try:
+        f.remote(my_ref)  # This would cause full CPython death.
+    except ray.exceptions.RayError:


Could we check more specifically to make sure the exact error is thrown, RayError is too broad. We can even check the exception message.

Btw, you can use with pytest.raises(...)

done. thanks!

jjyao · 2022-09-13T18:21:05Z

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

@@ -311,6 +328,10 @@ Status CoreWorkerMemoryStore::GetImpl(const std::vector<ObjectID> &object_ids,
        count += 1;
      } else {
        remaining_ids.insert(object_id);
+        Status status = ObjectHasOwner(object_id, abort_if_any_object_is_exception);


Hmm, I think abort_if_any_object_is_exception and abort_if_missing are two different things.

ok I've changed this

jjyao · 2022-09-13T18:24:02Z

Do you think we could review the current scope of changes and have a follow-up item to review other API paths that may cause CPython death afterwards? : )

sg :)

pabloem · 2022-10-10T22:00:55Z

@jjyao any pointers for me to look at reproducing any possible failures locally? : )

pabloem · 2022-11-01T16:43:11Z

@jjyao ptal? : )

pabloem · 2022-11-01T17:13:23Z

i'll try to debug why the tests are not passing as expected...

jjyao · 2022-11-01T18:43:21Z

sorry for dropping the ball here, I'll take a look at the failures as well.

Signed-off-by: Pablo E <pabloem@apache.org>

pabloem · 2022-12-15T19:37:23Z

rebased!

I think this should be ready : ) @jjyao thanks

pabloem · 2022-12-15T20:41:43Z

reopening to rernu tests.......

Signed-off-by: Pablo E <pabloem@apache.org>

While working on ray-project#28209 I hit issue google/googletest#3514, which causes a couple gtest-dependent tests to be unable to build Signed-off-by: Pablo E <pabloem@apache.org>

pabloem · 2022-12-16T01:29:05Z

ok looks good finally : )

jjyao · 2022-12-16T22:52:37Z

src/ray/core_worker/core_worker.cc

+  for (size_t i = 0; i < ids.size(); i++) {
+    rpc::Address unused_owner_address;
+    auto status = GetOwnerAddress(ids[i], &unused_owner_address);
+    if (!status.ok()) {


Instead of not ok, can we check ObjectUnknownOwner explicitly?

jjyao · 2022-12-16T22:52:50Z

src/ray/core_worker/core_worker.cc

+  for (size_t i = 0; i < ids.size(); i++) {
+    rpc::Address unused_owner_address;
+    auto status = GetOwnerAddress(ids[i], &unused_owner_address);
+    if (!status.ok()) {


jjyao · 2022-12-16T22:56:40Z

Tests look good. I manually triggered mac tests, lets see if they are passing as well.

pabloem · 2022-12-17T05:15:24Z

CI for the previous commit without the couple changes from comments: https://buildkite.com/ray-project/oss-ci-build-pr/builds/7636#0185179c-5905-4cf6-95bb-d6eea5422628

Signed-off-by: Pablo E <pabloem@apache.org>

jjyao · 2022-12-18T11:54:30Z

Thanks!!

While working on ray-project#28209 I hit issue google/googletest#3514, which causes a couple gtest-dependent tests to be unable to build Signed-off-by: Pablo E <pabloem@apache.org> Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

Signed-off-by: Pablo E <pabloem@apache.org> When accessing unknown objects, Ray will kill the Python interpreter (without erroring out!). I've tried to rephrase the error to be a little easier to figure out and fix, and changed the check so that an error will be thrown rather than ending the whole process. Signed-off-by: Capiru <gabriel_s_prado@hotmail.com>

This reverts commit d57b582.

Signed-off-by: Pablo E <pabloem@apache.org> When accessing unknown objects, Ray will kill the Python interpreter (without erroring out!). I've tried to rephrase the error to be a little easier to figure out and fix, and changed the check so that an error will be thrown rather than ending the whole process.

Signed-off-by: Pablo E <pabloem@apache.org> When accessing unknown objects, Ray will kill the Python interpreter (without erroring out!). I've tried to rephrase the error to be a little easier to figure out and fix, and changed the check so that an error will be thrown rather than ending the whole process. Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>

pabloem force-pushed the patch-1 branch from 7cb5a3c to f7b6ed5 Compare September 7, 2022 16:57

pabloem changed the title ~~[wip] Improving message for access to unknown objects~~ Improving message for access to unknown objects Sep 7, 2022

pabloem marked this pull request as ready for review September 7, 2022 17:01

pabloem force-pushed the patch-1 branch from f271d12 to 3ec7472 Compare September 7, 2022 22:46

jjyao self-assigned this Sep 7, 2022

pabloem force-pushed the patch-1 branch 2 times, most recently from d3ec300 to d45ee06 Compare September 8, 2022 05:41

pabloem force-pushed the patch-1 branch from 6b60111 to 4c4b4c0 Compare September 8, 2022 15:00

pabloem force-pushed the patch-1 branch from ef0d949 to 1da6c46 Compare September 9, 2022 06:44

pabloem force-pushed the patch-1 branch from 1da6c46 to 7c22aca Compare September 9, 2022 06:49

jjyao reviewed Sep 13, 2022

View reviewed changes

pabloem force-pushed the patch-1 branch from de776e0 to 976b2bb Compare October 10, 2022 17:06

pabloem force-pushed the patch-1 branch from 976b2bb to eacc43a Compare November 1, 2022 18:07

pabloem added 4 commits December 15, 2022 11:36

improve code

e3a6926

Signed-off-by: Pablo E <pabloem@apache.org>

Input validation at the beginning

32b0b9c

Signed-off-by: Pablo E <pabloem@apache.org>

Adding new error status: ObjectUnknownOwner

ae7a039

Signed-off-by: Pablo E <pabloem@apache.org>

fixups

4d0392c

Signed-off-by: Pablo E <pabloem@apache.org>

pabloem force-pushed the patch-1 branch from 8ee3e03 to 4d0392c Compare December 15, 2022 19:36

pabloem closed this Dec 15, 2022

pabloem reopened this Dec 15, 2022

fixup

4d2ef69

Signed-off-by: Pablo E <pabloem@apache.org>

pabloem requested a review from jjyao December 16, 2022 01:28

jjyao approved these changes Dec 16, 2022

View reviewed changes

address comments

9018945

Signed-off-by: Pablo E <pabloem@apache.org>

pabloem force-pushed the patch-1 branch from 9656e7a to 9018945 Compare December 17, 2022 05:15

jjyao merged commit d57b582 into ray-project:master Dec 18, 2022

pabloem deleted the patch-1 branch December 18, 2022 14:57

rickyyx mentioned this pull request Dec 23, 2022

[core][nightly] many_actors regression #31284

Closed

rickyyx added a commit that referenced this pull request Jan 3, 2023

Revert "Improving message for access to unknown objects (#28209)"

4cb3927

This reverts commit d57b582.

rickyyx mentioned this pull request Jan 3, 2023

Revert "Improving message for access to unknown objects" #31408

Closed

		appropriate_exception_thrown
		), "ray.get is hanging on unknown object retrieval"

Improving message for access to unknown objects #28209

Improving message for access to unknown objects #28209

Conversation

pabloem commented Aug 31, 2022 • edited Loading

Why are these changes needed?

Changes to Ray APIs

Related issue number

Checks

pabloem commented Sep 7, 2022

pabloem commented Sep 7, 2022

pabloem commented Sep 7, 2022

richardliaw commented Sep 7, 2022

jjyao commented Sep 8, 2022

pabloem commented Sep 8, 2022

pabloem commented Sep 8, 2022

jjyao commented Sep 9, 2022

pcmoritz commented Sep 9, 2022 • edited Loading

pabloem commented Sep 9, 2022

jjyao commented Sep 9, 2022 • edited Loading

pabloem commented Sep 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjyao commented Sep 13, 2022

pabloem commented Oct 10, 2022

pabloem commented Nov 1, 2022

pabloem commented Nov 1, 2022

jjyao commented Nov 1, 2022

pabloem commented Dec 15, 2022

pabloem commented Dec 15, 2022

pabloem commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjyao commented Dec 16, 2022

pabloem commented Dec 17, 2022

jjyao commented Dec 18, 2022

pabloem commented Aug 31, 2022 •

edited

Loading

pcmoritz commented Sep 9, 2022 •

edited

Loading

jjyao commented Sep 9, 2022 •

edited

Loading