Skip to content

[Core][Node Labels 3/n]Add node labels to node resources and publish to all node#36009

Merged
jjyao merged 1 commit intoray-project:masterfrom
larrylian:node_labels_3
Jun 14, 2023
Merged

[Core][Node Labels 3/n]Add node labels to node resources and publish to all node#36009
jjyao merged 1 commit intoray-project:masterfrom
larrylian:node_labels_3

Conversation

@larrylian
Copy link
Copy Markdown
Contributor

@larrylian larrylian commented Jun 2, 2023 •
edited
Loading

Why are these changes needed?

Add node labels to node resources and publish to all node

Related issue number

Enhancing node affinity scheduling feature through node labels #34894
(P1)Parse the configuration parameters for node labels and save them in the NodeInfo data structure.

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

@larrylian larrylian requested a review from a team as a code owner June 2, 2023 03:36
jjyao
jjyao reviewed Jun 2, 2023
View reviewed changes
src/ray/raylet/scheduling/cluster_resource_manager.cc Outdated
Comment on lines 303 to 302
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 2, 2023

Choose a reason for hiding this comment

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

it's a bit weird that we can have node with only labels but empty resources. We should be able to set labels and resources together when we add a node?

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 12, 2023

Choose a reason for hiding this comment

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

I have reconsidered and still think it's better to keep this interface.

  1. it's not weird because the original code logic already had cases where the total resources of a node were not set.
    image

  2. To update resources, use UpdateResourceCapacity, and to update labels, use UpdateNodeLabels. This way, the interface is more focused, and the code is simpler.

Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 12, 2023

Choose a reason for hiding this comment

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

I'm actually wondering when we may not have total resources. Should it be a check instead of warning? Let me check with the team.

Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 13, 2023

Choose a reason for hiding this comment

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

Can we actually change if (it == nodes_.end()) { to CHECK which makes code easier to reason about.

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 13, 2023

Choose a reason for hiding this comment

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

My current implementation requires me to retain this logic. Since the labels information is already available in 'GcsNodeInfo', I will first call 'ResetNodeLabels()', and then update the resources.
image

jjyao
jjyao reviewed Jun 2, 2023
View reviewed changes
src/ray/raylet/scheduling/cluster_resource_data.cc Outdated
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 2, 2023

Choose a reason for hiding this comment

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

After we have labels, the current naming (NodeResources) is no longer that accurate. We should probably have something like

class Node {
   NodeResources resources;
  map<string, string> labels;
}

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 2, 2023

Choose a reason for hiding this comment

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

  1. Labels can also be regarded as a resource of node. So it is very suitable to put it in NodeResources, and it reduces a lot of code changes.
  2. NodeResources do not need to be computable. The ResourceRequest in NodeResources is Computable. Adding labels to NodeResources does not affect the original available/tatal, etc.
    image

Copy link
Copy Markdown
Contributor

@jiwq jiwq Jun 5, 2023

Choose a reason for hiding this comment

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

Cool! Got the design.

jiwq
jiwq reviewed Jun 5, 2023
View reviewed changes
src/ray/raylet/scheduling/cluster_resource_data.cc Outdated
Copy link
Copy Markdown
Contributor

@jiwq jiwq Jun 5, 2023

Choose a reason for hiding this comment

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

Cool! Got the design.

@larrylian larrylian requested review from jiwq and jjyao June 12, 2023 09:21
jjyao
jjyao reviewed Jun 13, 2023
View reviewed changes
src/ray/raylet/scheduling/cluster_resource_manager.cc Outdated
Comment on lines 303 to 302
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 13, 2023

Choose a reason for hiding this comment

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

Can we actually change if (it == nodes_.end()) { to CHECK which makes code easier to reason about.

src/ray/raylet/scheduling/cluster_resource_manager.cc Outdated
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 13, 2023

Choose a reason for hiding this comment

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

I don't think we should early return here. If labels are empty then we should just set node labels to empty.

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Same as above.
I've thought about it, and for the scenario where nodes are added, the semantics should be 'reset' instead of 'update', since the labels are static. Therefore, I have changed the interface to 'ResetNodeLabels()'. If dynamic labels are added in the future, I will create a new interface called 'UpdateNodeLabels()'.

@larrylian larrylian force-pushed the node_labels_3 branch 2 times, most recently from 3781ee3 to 3942b14 Compare June 13, 2023 04:00
jjyao
jjyao approved these changes Jun 13, 2023
View reviewed changes
src/ray/gcs/gcs_server/gcs_resource_manager.cc Outdated
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 13, 2023

Choose a reason for hiding this comment

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

nit: SetNodeLabels()

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 14, 2023

Choose a reason for hiding this comment

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

Fixed

src/ray/raylet/node_manager.h Outdated
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 13, 2023

Choose a reason for hiding this comment

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

remove this line

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 14, 2023

Choose a reason for hiding this comment

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

Fixed

src/ray/raylet/scheduling/cluster_resource_scheduler.cc Outdated
Copy link
Copy Markdown
Contributor

@jjyao jjyao Jun 13, 2023

Choose a reason for hiding this comment

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

Where is the change that creates the ClusterResourceScheduler and pass in the labels?

Copy link
Copy Markdown
Contributor Author

@larrylian larrylian Jun 14, 2023

Choose a reason for hiding this comment

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

image

@larrylian
[Core][Node Labels 3/n]Add node labels to node resources and publish to
a114500 
all node

Signed-off-by: LarryLian <554538252@qq.com>
@larrylian larrylian mentioned this pull request Jun 14, 2023
Open
28 tasks
@jjyao jjyao merged commit 887c03f into ray-project:master Jun 14, 2023
@akshay-anyscale akshay-anyscale mentioned this pull request Jul 21, 2023
Closed
8 tasks
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
@larrylian @arvind-chandra
[Core][Node Labels 3/n]Add node labels to node resources and publish …
c003b7e 
…to all node(ray-project#36009)

Signed-off-by: LarryLian <554538252@qq.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjyao jjyao jjyao approved these changes

@scv119 scv119 Awaiting requested review from scv119

@wumuzi520 wumuzi520 Awaiting requested review from wumuzi520

@SongGuyang SongGuyang Awaiting requested review from SongGuyang

@jiwq jiwq Awaiting requested review from jiwq

Assignees

@jjyao jjyao

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@larrylian @jjyao @jiwq