[core] Minor cpp changes around core worker #48262

dayshah · 2024-10-24T21:59:05Z

Why are these changes needed?

Minor cpp changes around core worker, was part of #48661, but factored out those changes.

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 :(

Signed-off-by: dayshah <[email protected]>

python/ray/tests/test_cancel.py

rynewang · 2024-10-28T23:43:56Z

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

  auto remaining_timeout_ms = timeout_ms;
  auto timeout_timestamp = current_time_ms() + timeout_ms;
  while (!is_ready_) {
+    // TODO (dayshah): see if using cv condition function instead of busy while helps.


is this still relevant?

ya i think it could still be relevant, pretty sure using cv.wait_for(lock, timeout, []() { return !is_ready; }); could lead to less cpu usage vs the busy while loop but want to look more into how it would affect performance

ok, let's do it in another PR, if needed

src/ray/core_worker/task_manager.cc

Signed-off-by: dayshah <[email protected]>

rynewang · 2024-10-29T04:26:13Z

test failures in cpp and python

Signed-off-by: dayshah <[email protected]>

rynewang · 2024-11-01T20:58:51Z

src/ray/core_worker/transport/normal_task_submitter.cc

  RAY_LOG(INFO) << "Cancelling a task: " << task_spec.TaskId()
                << " force_kill: " << force_kill << " recursive: " << recursive;
-  const SchedulingKey scheduling_key(
+  SchedulingKey scheduling_key(


why removed const

so it can be moved into function capture below

rynewang · 2024-11-01T21:00:43Z

src/ray/core_worker/transport/normal_task_submitter.cc

-    if (!keep_executing) {
-      RAY_UNUSED(task_finisher_->FailOrRetryPendingTask(
-          task_spec.TaskId(), rpc::ErrorType::TASK_CANCELLED, nullptr));
+      RequestNewWorkerIfNeeded(scheduling_key);


we removed a call to FailOrRetryPendingTask here. is this expected?

ya this is the call we removed for when the task is removed and the task dependencies have just now been resolved, since now we're failing task before when cancel is actually called

Signed-off-by: dayshah <[email protected]>

jjyao

In the PR description, could you add details on how the hang happens. Something like #47861

python/ray/tests/test_cancel.py

jjyao · 2024-11-07T19:07:12Z

src/ray/core_worker/core_worker.cc

        << "Cancel an actor task";
    CancelActorTaskOnExecutor(
-        caller_worker_id, task_id, force_kill, recursive, on_cancel_callback);
+        caller_worker_id, task_id, force_kill, recursive, std::move(on_cancel_callback));


Do we need to move since the parameter is const &?

ya, my bad left it in even after changing param

wait actually CancelActorTaskOnExecucutor takes by value, CancelTaskOnExecutor takes by const ref

jjyao · 2024-11-07T19:15:35Z

src/ray/core_worker/task_manager.cc

    auto it = submissible_tasks_.find(task_id);
    RAY_CHECK(it != submissible_tasks_.end())
        << "Tried to fail task that was not pending " << task_id;
-    RAY_CHECK(it->second.IsPending())


why changing this? The function name indicating that it's a pending task.

So the issue was there was a ray data backpressure test and a train test that failed here, it seems that the task cancel would happen after the task finishes and by the time it acquires that mutex in FailPendingTask it's already at a point where the task status is finished. So change here is basically just to no-op on if the task is finished at this point.

and IsPending checks for status != fail and finish, only want to check for fail here to make sure we're not double failing

Signed-off-by: dayshah <[email protected]>

dayshah · 2024-11-07T22:09:03Z

In the PR description, could you add details on how the hang happens. Something like #47861

Updated pr description to list the 3 main changes and why they're needed

Signed-off-by: dayshah <[email protected]>

dentiny · 2024-11-08T01:03:59Z

src/ray/rpc/worker/core_worker_client.h


 /// The maximum number of requests in flight per client.
-const int64_t kMaxBytesInFlight = 16 * 1024 * 1024;
+constexpr int64_t kMaxBytesInFlight = 16L * 1024 * 1024;


Randomly find this PR, two things:

We should have a human-readable unit library, something like 16_MiB;

inline is needed if we declare constants at header file

C++ spec doesn't guide the behavior on constexpr, when included by multiple translation units, whether the symbol appears once or multiple times, so it's a compiler-based UB

Do you think it looks ok to you? #48638

yes but let's not block this pr. we can always update later

inline constexpr is needed, yes

ya good point inlined, can integrate into library after, there's other instances of stuff like this too can go at once

Signed-off-by: dayshah <[email protected]>

Signed-off-by: Dhyey Shah <[email protected]>

Signed-off-by: dayshah <[email protected]>

jjyao · 2024-11-08T22:01:41Z

src/ray/core_worker/transport/normal_task_submitter.cc

        if (spec->TaskId() == task_spec.TaskId()) {
          scheduling_tasks.erase(spec);
-
-          if (scheduling_tasks.empty()) {


We shouldn't remove this if check, we should only cancel worker lease if there is no scheduling task.

Actually removed this because CancelWorkerLeaseIfNeeded already checks if its empty and early returns if it's not empty

As titled, I think having `MB` explicitly called out is more readable than `1024 * 1024` or `1<<20` Intended use case: #48262 (comment) Signed-off-by: dentiny <[email protected]>

Signed-off-by: dayshah <[email protected]>

As titled, I think having `MB` explicitly called out is more readable than `1024 * 1024` or `1<<20` Intended use case: ray-project#48262 (comment) Signed-off-by: dentiny <[email protected]>

Signed-off-by: dayshah <[email protected]> Signed-off-by: mohitjain2504 <[email protected]>

As titled, I think having `MB` explicitly called out is more readable than `1024 * 1024` or `1<<20` Intended use case: ray-project#48262 (comment) Signed-off-by: dentiny <[email protected]> Signed-off-by: mohitjain2504 <[email protected]>

dayshah added 6 commits October 17, 2024 01:28

general cleanup on related files

0048901

Signed-off-by: dayshah <[email protected]>

minor task submitter header changes

34c8be3

Signed-off-by: dayshah <[email protected]>

Merge branch 'master' into cancel-task-with-deps

67ff6fc

Signed-off-by: dayshah <[email protected]>

remove const cast

bf649c9

Signed-off-by: dayshah <[email protected]>

fail task with unresolved dependencies

b88c105

Signed-off-by: dayshah <[email protected]>

general cleanup linting on some files

7ff72eb

Signed-off-by: dayshah <[email protected]>

dayshah marked this pull request as ready for review October 28, 2024 22:26

dayshah requested a review from a team as a code owner October 28, 2024 22:26

dayshah changed the title ~~[WIP][core] Fix hang when getting cancelled task with dependencies in progress~~ [core] Fix hang when getting cancelled task with dependencies in progress Oct 28, 2024

adding test and moving cancelled callback

1293f86

Signed-off-by: dayshah <[email protected]>

rynewang reviewed Oct 28, 2024

View reviewed changes

python/ray/tests/test_cancel.py Outdated Show resolved Hide resolved

rynewang approved these changes Oct 28, 2024

View reviewed changes

dayshah added 2 commits October 28, 2024 19:47

address test comments

24f0635

Signed-off-by: dayshah <[email protected]>

store in plasma equals

877242b

Signed-off-by: dayshah <[email protected]>

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

dayshah added 3 commits October 29, 2024 16:27

reorder cancel worker lease for test

34f8199

Signed-off-by: dayshah <[email protected]>

update resolver accordingly

b667628

Signed-off-by: dayshah <[email protected]>

fix cpp test

d2c3259

Signed-off-by: dayshah <[email protected]>

rynewang reviewed Nov 1, 2024

View reviewed changes

dayshah and others added 3 commits November 1, 2024 17:36

Merge remote-tracking branch 'origin/master' into cancel-task-with-deps

5314b2f

Signed-off-by: dayshah <[email protected]>

Merge branch 'master' into cancel-task-with-deps

a863ecc

allow failpendingtask call on finished task

869327e

Signed-off-by: dayshah <[email protected]>

jjyao reviewed Nov 7, 2024

View reviewed changes

dayshah added 3 commits November 7, 2024 15:08

dont move on cancel callback

28a8f25

Signed-off-by: dayshah <[email protected]>

use force in test

5e089b9

Signed-off-by: dayshah <[email protected]>

move on cancel callback

9f8655a

Signed-off-by: dayshah <[email protected]>

dayshah requested a review from jjyao November 7, 2024 22:09

dayshah assigned rynewang Nov 7, 2024

remove cancelled tasks in task submitter

0bcb639

Signed-off-by: dayshah <[email protected]>

dentiny reviewed Nov 8, 2024

View reviewed changes

dentiny mentioned this pull request Nov 8, 2024

[core] Add a util for size literals #48638

Merged

dayshah added 4 commits November 8, 2024 00:15

reorder and don't cancel worker lease

f459cf6

Signed-off-by: dayshah <[email protected]>

inline constexpr

085621c

Signed-off-by: dayshah <[email protected]>

Merge branch 'master' into cancel-task-with-deps

15afd22

Signed-off-by: Dhyey Shah <[email protected]>

factor out actual functional changes for issue

aae4aa9

Signed-off-by: dayshah <[email protected]>

dayshah changed the title ~~[core] Fix hang when getting cancelled task with dependencies in progress~~ [core] Minor cpp changes around core worker Nov 8, 2024

dayshah assigned jjyao Nov 8, 2024

dentiny mentioned this pull request Nov 8, 2024

[core] Optimize ray check branch prediction #48663

Merged

jjyao reviewed Nov 8, 2024

View reviewed changes

jjyao merged commit e2dfcc8 into ray-project:master Nov 9, 2024

dayshah deleted the cancel-task-with-deps branch November 9, 2024 18:56

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

[core] Minor cpp changes around core worker (ray-project#48262)

b1c0980

Signed-off-by: dayshah <[email protected]>

mohitjain2504 pushed a commit to mohitjain2504/ray that referenced this pull request Nov 15, 2024

[core] Minor cpp changes around core worker (ray-project#48262)

3017ed9

Signed-off-by: dayshah <[email protected]> Signed-off-by: mohitjain2504 <[email protected]>

[core] Minor cpp changes around core worker #48262

[core] Minor cpp changes around core worker #48262

Uh oh!

Conversation

dayshah commented Oct 24, 2024 • 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!

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!

Uh oh!

rynewang commented Oct 29, 2024

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!

jjyao left a comment

Choose a reason for hiding this comment

Uh oh!

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!

dayshah commented Nov 7, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dayshah commented Oct 24, 2024 •

edited

Loading