Skip to content

[core] replace node_manager_client with raylet_client_lib#56261

Merged
dayshah merged 8 commits intoray-project:masterfrom
rueian:remove-node-manager-client
Sep 7, 2025
Merged

[core] replace node_manager_client with raylet_client_lib#56261
dayshah merged 8 commits intoray-project:masterfrom
rueian:remove-node-manager-client

Conversation

@rueian
Copy link
Copy Markdown
Contributor

@rueian rueian commented Sep 4, 2025 •
edited
Loading

Why are these changes needed?

This is the follow-up for #56101.

  1. Removing NodeManagerClient by merging it into RayletClient.
  2. Move raylet_client to ray/rpc/raylet
  3. Specifying visibilities explicitly for the remaining build targets in the BUILD file.

No behavior changes in this PR.

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

@rueian rueian force-pushed the remove-node-manager-client branch 3 times, most recently from 2e8a95a to e2cbb7c Compare September 4, 2025 22:58
@rueian
[core] replace node_manager_client with raylet_client_lib
486c719 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian rueian force-pushed the remove-node-manager-client branch from e2cbb7c to 486c719 Compare September 4, 2025 23:44
@rueian
update comments
cc06982 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian rueian changed the title Remove node manager client [core] replace node_manager_client with raylet_client_lib Sep 5, 2025
@rueian rueian force-pushed the remove-node-manager-client branch 3 times, most recently from 88c94a0 to 3bb3049 Compare September 5, 2025 18:29
@rueian
update
27726a4 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian rueian force-pushed the remove-node-manager-client branch from 3bb3049 to 27726a4 Compare September 5, 2025 18:34
@rueian rueian added the go add ONLY when ready to merge, run all tests label Sep 5, 2025
rueian
rueian commented Sep 6, 2025
View reviewed changes
src/ray/raylet_client/raylet_client.cc
retryable_grpc_client_,
ray::rpc,
NodeManagerService,
ReturnWorkerLease,
Copy link
Copy Markdown
Contributor Author

@rueian rueian Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReturnWorkerLease should be called with retryable_grpc_client_ according to the original node_manager_client.

src/ray/raylet_client/raylet_client.cc Outdated
INVOKE_RETRYABLE_RPC_CALL_FULL(retryable_grpc_client_,
ray::rpc,
NodeManagerService,
CancelWorkerLease,
Copy link
Copy Markdown
Contributor Author

@rueian rueian Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CancelWorkerLease should be called with retryable_grpc_client_ according to the original node_manager_client.

src/ray/rpc/grpc_client.h Outdated
&SERVICE_NAMESPACE::SERVICE::Stub::PrepareAsync##METHOD, \
request, \
callback, \
#SERVICE ".grpc_client." #METHOD, \
Copy link
Copy Markdown
Contributor Author

@rueian rueian Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new INVOKE_RPC_CALL_FULL macro is created to keep this call_name (#SERVICE ".grpc_client." #METHOD) argument unchanged, but allow specifying SERVICE_NAMESPACE from the caller side.

src/ray/rpc/retryable_grpc_client.h Outdated
SERVICE_NAMESPACE::METHOD##Reply>( \
&SERVICE_NAMESPACE::SERVICE::Stub::PrepareAsync##METHOD, \
rpc_client, \
#SERVICE ".grpc_client." #METHOD, \
Copy link
Copy Markdown
Contributor Author

@rueian rueian Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as INVOKE_RETRYABLE_RPC_CALL_FULL macro. This is created to keep this call_name (#SERVICE ".grpc_client." #METHOD) argument unchanged, but allow specifying SERVICE_NAMESPACE from the caller side.

@rueian rueian marked this pull request as ready for review September 6, 2025 06:23
@rueian rueian requested a review from a team as a code owner September 6, 2025 06:23
@ray-gardener ray-gardener bot added the core Issues that should be addressed in Ray Core label Sep 6, 2025
dayshah
dayshah reviewed Sep 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@dayshah dayshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, this has been on the wishlist and closes this issue that I opened before

#54816

src/ray/raylet_client/raylet_client.cc Outdated
request,
callback,
grpc_client_,
/*method_timeout_ms*/ -1);
Copy link
Copy Markdown
Contributor

@dayshah dayshah Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just move raylet_client and raylet_client_pool into a raylet folder inside rpc and put it inside the ray::rpc namespace so you don't need this special rpc naming macro and it stays consistent with the core_worker_client

Copy link
Copy Markdown
Contributor Author

@rueian rueian Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I have moved the entire raylet_client to under ray/rpc/raylet.

src/ray/raylet_client/BUILD.bazel Outdated
":raylet_client_interface",
"//src/ray/common:task_common",
"//src/ray/flatbuffers:node_manager_generated",
"//src/ray/gcs/gcs_client:gcs_client_lib",
Copy link
Copy Markdown
Contributor

@dayshah dayshah Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is gcs_client_lib needed here for?

Copy link
Copy Markdown
Contributor Author

@rueian rueian Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for pointing that out. I removed all the unnecessary deps.

@rueian
remove gcs_client_lib from raylet_client_lib
cc6e7d8 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian
move raylet_client
2ec2357 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian
update references
f3ab0e5 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian
update macros and lint
259a422 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian rueian force-pushed the remove-node-manager-client branch 2 times, most recently from a5f5c93 to db921c1 Compare September 7, 2025 02:30
@rueian
remove unnecessary deps
f24bc5d 
Signed-off-by: Rueian <rueian@anyscale.com>
@rueian rueian force-pushed the remove-node-manager-client branch from db921c1 to f24bc5d Compare September 7, 2025 02:50
@rueian rueian requested a review from dayshah September 7, 2025 17:06
dayshah
dayshah approved these changes Sep 7, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@dayshah dayshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

@dayshah dayshah merged commit 7a3da11 into ray-project:master Sep 7, 2025
5 checks passed
@dayshah dayshah mentioned this pull request Sep 7, 2025
Closed
@dayshah dayshah mentioned this pull request Sep 7, 2025
Merged
8 tasks
sampan-s-nayak pushed a commit to sampan-s-nayak/ray that referenced this pull request Sep 8, 2025
@rueian
[core] replace node_manager_client with raylet_client_lib (ray-projec…
c95d462 
…t#56261)

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: sampan <sampan@anyscale.com>
@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Sep 8, 2025

great stuff @rueian!!

jugalshah291 pushed a commit to jugalshah291/ray_fork that referenced this pull request Sep 11, 2025
@rueian @jugalshah291
[core] replace node_manager_client with raylet_client_lib (ray-projec…
2fe5730 
…t#56261)

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: jugalshah291 <shah.jugal291@gmail.com>
wyhong3103 pushed a commit to wyhong3103/ray that referenced this pull request Sep 12, 2025
@rueian
[core] replace node_manager_client with raylet_client_lib (ray-projec…
8ff74b8 
…t#56261)

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: yenhong.wong <yenhong.wong@grabtaxi.com>
ZacAttack pushed a commit to ZacAttack/ray that referenced this pull request Sep 24, 2025
@rueian @ZacAttack
[core] replace node_manager_client with raylet_client_lib (ray-projec…
5882d88 
…t#56261)

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: zac <zac@anyscale.com>
dstrodtman pushed a commit that referenced this pull request Oct 6, 2025
@rueian @dstrodtman
[core] replace node_manager_client with raylet_client_lib (#56261)
f37eb50 
Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
justinyeh1995 pushed a commit to justinyeh1995/ray that referenced this pull request Oct 20, 2025
@rueian @justinyeh1995
[core] replace node_manager_client with raylet_client_lib (ray-projec…
750c94f 
…t#56261)

Signed-off-by: Rueian <rueian@anyscale.com>
landscapepainter pushed a commit to landscapepainter/ray that referenced this pull request Nov 17, 2025
@rueian @landscapepainter
[core] replace node_manager_client with raylet_client_lib (ray-projec…
5070b06 
…t#56261)

Signed-off-by: Rueian <rueian@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dayshah dayshah dayshah approved these changes

Assignees

No one assigned

Labels

core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rueian @edoakes @dayshah