[Observability] Added object refs Task is dependent on to `TaskInfoEntry` #48234

alexeykudinkin · 2024-10-23T23:47:40Z

Why are these changes needed?

Currently when listing current tasks using ray get task <task-id> we're not reflecting the args the task might be dependent on and therefore waiting to become available.

That substantially complicates troubleshooting when the task is blocked on an argument passed in as an object-ref.

This change adds arguments passed in by reference to the TaskInfoEntry to subsequently make it available to our StateHead APIs.

Related issue number

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 added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
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 :(

jjyao

Thanks for doing that! Could you add some tests?

jjyao · 2024-10-23T23:49:23Z

src/ray/core_worker/core_worker.cc

@@ -637,6 +637,7 @@ CoreWorker::CoreWorker(const CoreWorkerOptions &options, const WorkerID &worker_
        [this] {
          RAY_LOG(INFO) << "Event stats:\n\n"
                        << io_service_.stats().StatsString() << "\n\n"
+                        << task_execution_service_.stats().StatsString() << "\n\n"


Haha, I have another PR that also adds this.

jjyao · 2024-10-24T07:31:29Z

src/ray/protobuf/common.proto

+  //       this task is dependent on and does NOT contain
+  //        - Args passed by value (inlined)
+  //        - ObjectRefs of the args passed by value
+  repeated ObjectReference dependent_args_refs = 27;


I think we can only record the ObjectID instead of the entire ObjectRef?

Surely, but why not entire ObjectRef?

I think we only care about ObjectID so trying to reduce memory usage: we need to store lots of events in GCS also each task can potentially have unlimited number of args.

jjyao · 2024-10-24T20:25:34Z

There is test failure.

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…ds on Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

… tasks API Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

jjyao

LGTM

jjyao · 2024-10-28T04:54:31Z

src/ray/protobuf/common.proto

+  // NOTE: This list only contains `ObjectReference`s passed in as arguments
+  //       this task is dependent on and does NOT contain
+  //        - Args passed by value (inlined)
+  //        - ObjectRefs of the args passed by value


What does this mean?

Inlined objects

python/ray/util/state/common.py

jjyao · 2024-10-28T04:58:42Z

python/ray/tests/test_state_api.py

+        "task_id": "31323334",
+        "parent_task_id": "",
+        "args_object_ids": [
+            arg_ref.hex(),


Don't we store the binary format of the object id instead of hex?

We decode in the API

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…skInfoEntry` (ray-project#48234)" This reverts commit cacb54c.

…skInfoEntry` (#48234)" (#48498) This reverts commit cacb54c. CoreWorker launches a thread to send events to GCS ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/task_event_buffer.cc?L286:49-286:60)). #48234 adds the following logic in `pb_util.h`: ```cpp // Fill in task args for (size_t i = 0; i < task_spec.NumArgs(); i++) { if (task_spec.ArgByRef(i)) { task_info->add_args_object_ids(task_spec.ArgRef(i).object_id()); } } ``` However, it is possible for another thread to call [clear_object_ref](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/transport/dependency_resolver.cc?L36-38) between `if (task_spec.ArgByRef(i)) {` and `task_spec.ArgRef(i)`. In this case, `RAY_CHECK` ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/common/task/task_spec.cc?L279)) in `ArgRef` will fail. Error message: <img width="1864" alt="image" src="https://github.com/user-attachments/assets/5401b6ef-959a-4afe-beea-daf4e1577b0d">

…try` (ray-project#48234) Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…skInfoEntry` (ray-project#48234)" (ray-project#48498) This reverts commit cacb54c. CoreWorker launches a thread to send events to GCS ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/task_event_buffer.cc?L286:49-286:60)). ray-project#48234 adds the following logic in `pb_util.h`: ```cpp // Fill in task args for (size_t i = 0; i < task_spec.NumArgs(); i++) { if (task_spec.ArgByRef(i)) { task_info->add_args_object_ids(task_spec.ArgRef(i).object_id()); } } ``` However, it is possible for another thread to call [clear_object_ref](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/transport/dependency_resolver.cc?L36-38) between `if (task_spec.ArgByRef(i)) {` and `task_spec.ArgRef(i)`. In this case, `RAY_CHECK` ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/common/task/task_spec.cc?L279)) in `ArgRef` will fail. Error message: <img width="1864" alt="image" src="https://github.com/user-attachments/assets/5401b6ef-959a-4afe-beea-daf4e1577b0d">

…try` (ray-project#48234) Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…skInfoEntry` (ray-project#48234)" (ray-project#48498) This reverts commit cacb54c. CoreWorker launches a thread to send events to GCS ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/task_event_buffer.cc?L286:49-286:60)). ray-project#48234 adds the following logic in `pb_util.h`: ```cpp // Fill in task args for (size_t i = 0; i < task_spec.NumArgs(); i++) { if (task_spec.ArgByRef(i)) { task_info->add_args_object_ids(task_spec.ArgRef(i).object_id()); } } ``` However, it is possible for another thread to call [clear_object_ref](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/transport/dependency_resolver.cc?L36-38) between `if (task_spec.ArgByRef(i)) {` and `task_spec.ArgRef(i)`. In this case, `RAY_CHECK` ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/common/task/task_spec.cc?L279)) in `ArgRef` will fail. Error message: <img width="1864" alt="image" src="https://github.com/user-attachments/assets/5401b6ef-959a-4afe-beea-daf4e1577b0d"> Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>

…try` (ray-project#48234) Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

…skInfoEntry` (ray-project#48234)" (ray-project#48498) This reverts commit cacb54c. CoreWorker launches a thread to send events to GCS ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/task_event_buffer.cc?L286:49-286:60)). ray-project#48234 adds the following logic in `pb_util.h`: ```cpp // Fill in task args for (size_t i = 0; i < task_spec.NumArgs(); i++) { if (task_spec.ArgByRef(i)) { task_info->add_args_object_ids(task_spec.ArgRef(i).object_id()); } } ``` However, it is possible for another thread to call [clear_object_ref](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/core_worker/transport/dependency_resolver.cc?L36-38) between `if (task_spec.ArgByRef(i)) {` and `task_spec.ArgRef(i)`. In this case, `RAY_CHECK` ([code](https://sourcegraph.com/github.com/ray-project/ray@d0af8622f6b5d0668d521e20539b7d76426ceb5f/-/blob/src/ray/common/task/task_spec.cc?L279)) in `ArgRef` will fail. Error message: <img width="1864" alt="image" src="https://github.com/user-attachments/assets/5401b6ef-959a-4afe-beea-daf4e1577b0d"> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

alexeykudinkin requested review from a team, pcmoritz and raulchen as code owners October 23, 2024 23:47

alexeykudinkin requested a review from jjyao October 23, 2024 23:47

alexeykudinkin assigned jjyao Oct 23, 2024

alexeykudinkin added the go add ONLY when ready to merge, run all tests label Oct 23, 2024

jjyao reviewed Oct 23, 2024

View reviewed changes

jjyao reviewed Oct 24, 2024

View reviewed changes

alexeykudinkin added 14 commits October 25, 2024 13:41

Added tasks args to TaskInfoEntry

076681f

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Fill in task args inside TaskInfoEntry

97702f0

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Fixed filling in args

c58418d

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Revisited args in TaskInfoEntry to only bear object-refs task depen…

d3b9415

…ds on Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Make core worker print task_execution_service_ stats as well

e9ec70b

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Make sure dependent_args_refs propagated appropriately into listing…

41e668c

… tasks API Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Updated tests

9293788

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

lint

7065df6

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Persist just object ids instead of whole ObjectReference object

0593974

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Updating dashboard API

1479420

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Fixing tests

b4a054b

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Decode args_object_ids

2c50844

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Fixed message_to_dict util to properly decode repeated fields

563c0e1

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

lint

2f0c056

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

alexeykudinkin force-pushed the ak/tsk-evts-ctx-fix branch from 42ff9d3 to 2f0c056 Compare October 25, 2024 22:31

Fixing compilation

8ca9832

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

jjyao reviewed Oct 28, 2024

View reviewed changes

alexeykudinkin force-pushed the ak/tsk-evts-ctx-fix branch 2 times, most recently from 0b64417 to ccd8b8c Compare October 28, 2024 18:20

Tidying up

29659a6

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

alexeykudinkin force-pushed the ak/tsk-evts-ctx-fix branch from ccd8b8c to 29659a6 Compare October 28, 2024 18:21

jjyao approved these changes Oct 28, 2024

View reviewed changes

jjyao enabled auto-merge (squash) October 28, 2024 19:07

jjyao merged commit cacb54c into master Oct 28, 2024
6 checks passed

jjyao deleted the ak/tsk-evts-ctx-fix branch October 28, 2024 21:47

kevin85421 added a commit to kevin85421/ray that referenced this pull request Nov 1, 2024

Revert "[Observability] Added object refs Task is dependent on to `Ta…

7d67a5a

…skInfoEntry` (ray-project#48234)" This reverts commit cacb54c.

kevin85421 mentioned this pull request Nov 1, 2024

Revert "[Observability] Added object refs Task is dependent on to TaskInfoEntry (#48234)" #48498

Merged

8 tasks

Jay-ju pushed a commit to Jay-ju/ray that referenced this pull request Nov 5, 2024

[Observability] Added object refs Task is dependent on to `TaskInfoEn…

db4e6ba

…try` (ray-project#48234) Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024

[Observability] Added object refs Task is dependent on to `TaskInfoEn…

3ad971b

…try` (ray-project#48234) Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Observability] Added object refs Task is dependent on to `TaskInfoEntry` #48234

[Observability] Added object refs Task is dependent on to `TaskInfoEntry` #48234

alexeykudinkin commented Oct 23, 2024

jjyao left a comment

jjyao Oct 23, 2024

jjyao Oct 24, 2024

alexeykudinkin Oct 24, 2024

jjyao Oct 24, 2024

jjyao commented Oct 24, 2024

jjyao left a comment

jjyao Oct 28, 2024

alexeykudinkin Oct 28, 2024 •

edited

Loading

jjyao Oct 28, 2024

alexeykudinkin Oct 28, 2024

[Observability] Added object refs Task is dependent on to TaskInfoEntry #48234

[Observability] Added object refs Task is dependent on to TaskInfoEntry #48234

Conversation

alexeykudinkin commented Oct 23, 2024

Why are these changes needed?

Related issue number

Checks

jjyao left a comment

Choose a reason for hiding this comment

jjyao Oct 23, 2024

Choose a reason for hiding this comment

jjyao Oct 24, 2024

Choose a reason for hiding this comment

alexeykudinkin Oct 24, 2024

Choose a reason for hiding this comment

jjyao Oct 24, 2024

Choose a reason for hiding this comment

jjyao commented Oct 24, 2024

jjyao left a comment

Choose a reason for hiding this comment

jjyao Oct 28, 2024

Choose a reason for hiding this comment

alexeykudinkin Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

jjyao Oct 28, 2024

Choose a reason for hiding this comment

alexeykudinkin Oct 28, 2024

Choose a reason for hiding this comment

[Observability] Added object refs Task is dependent on to `TaskInfoEntry` #48234

[Observability] Added object refs Task is dependent on to `TaskInfoEntry` #48234

alexeykudinkin Oct 28, 2024 •

edited

Loading