Add top-level Labels and Resources Structed fields to HeadGroupSpec and WorkerGroupSpec

[ryanaoleary]

Why are these changes needed?

This PR lifts both resources and labels into explicit structured fields for the HeadGroupSpec and WorkerGroupSpec. When these optional fields are specified, they override their respective values in the rayStartCommand for Pods created by KubeRay for that group. Additionally, labels specified at the top-level Labels field are merged with the K8s labels on the Pod for observability.

The discussion and rationale for this change is discussed more in #3699. The labels part of this change will help enable the autoscaling use case with label_selectors in Ray core.

Related issue number

Contributes to ray-project/ray#51564

Checks

• I've made sure the tests are passing.
• Testing Strategy
  • Unit tests
  • Manual tests
  • This PR is not tested :(

[ryanaoleary]

cc: @Future-Outlier @edoakes @MengjinYan

[ryanaoleary]

Running make api-docs fails to add the Resources field to the api.md file. I think this is because I use:

Resources corev1.ResourceList

which isn't a top-level API object. I'm wondering if there's a preference for changing it to a map[string]string and then converting it to a ResourceList internally.

[andrewsykim]

This should mention that labels are ignored if already specifeid in rayStartParams?

[ryanaoleary]

The way I implemented it, we ignore --labels in rayStartParams if they exist and instead override it with the values set in the group Labels field. Should it actually be the opposite?

My thinking was that since the top-level Labels and Resources fields are the most explicit, they should take precedence.

[kevin85421]

We should try to avoid handling the logic of overriding or merging user configurations. It’s hard to ensure correct behavior and makes Ray Autoscaler more complex. My suggestions:

Add validations in ValidateRayClusterSpec:

• Resources
  • If users specify both (1) num-cpus / num-gpus / memory / resources in rayStartParams and (2) {Head|Worker}GroupSpec.Resources, we should fail validation and avoid reconciling anything. Users should only use (1) or (2).
• Labels
  • If users specify labels in rayStartParams, we should fail the validation because we plan not to handle the string parsing in Ray Autoscaler as @edoakes said. Only {Head|Worker}GroupSpec.Labels is allowed.

cc @Future-Outlier @rueian Could one of you open an issue to track updating the compatible Ray versions (because of Ray Autoscaler)? And @rueian, could you work on adding support in Ray Autoscaler for Resources / Labels?

[rueian]

Do we need to validate that there is no , in the k and groupLabels[k]?

[Future-Outlier]

opened here, thank you!
#4113

[ryanaoleary]

rsions (because of Ray Autoscaler)? And @rueian, could you work on adding support in Ray Autoscaler for Resources / Labels?

Added the validation logic in f7f85dd

[kevin85421]

To summarize my points in today's community sync,

• Resources: It can improve UX by avoiding the need to specify something like resources: '"{\"Custom1\": 1, \"Custom2\": 5}"' in rayStartParams (example). Therefore, this is safe to add to CRD.

• Labels:

  • The primary goal of adding Labels to the CRD is to avoid parsing the string rayStartParams["labels"] into Ray labels in the Ray Autoscaler. The Ray Autoscaler already has logic to parse the string. Therefore, this benefit would not be realized if we decide not to remove the string parsing logic from the Ray Autoscaler.
  • The other benefit is to verify the label values at the OpenAPI level, but this is not significant enough to justify handling both rayStartParams["labels"] and the new field Labels at the same time.

My proposals for Labels:

• Proposal 1: Not adding Labels to the CRD and keeping the Ray Autoscaler parsing the string is fine with me, but Edward didn't like it.
• Proposal 2: Add Labels to the CRD and remove the string parsing logic in Ray Autoscaler so that Ray Autoscaler doesn't need to handle both rayStartParams["labels"] and Labels.

cc @rueian @Future-Outlier @ryanaoleary @andrewsykim

[Future-Outlier]

To summarize my points in today's community sync,

• Resources: It can improve UX by avoiding the need to specify something like resources: '"{\"Custom1\": 1, \"Custom2\": 5}"' in rayStartParams (example). Therefore, this is safe to add to CRD.

• Labels:

  • The primary goal of adding Labels to the CRD is to avoid parsing the string rayStartParams["labels"] into Ray labels in the Ray Autoscaler. The Ray Autoscaler already has logic to parse the string. Therefore, this benefit would not be realized if we decide not to remove the string parsing logic from the Ray Autoscaler.
  • The other benefit is to verify the label values at the OpenAPI level, but this is not significant enough to justify handling both rayStartParams["labels"] and the new field Labels at the same time.

My proposals for Labels:

• Proposal 1: Not adding Labels to the CRD and keeping the Ray Autoscaler parsing the string is fine with me, but Edward didn't like it.
• Proposal 2: Add Labels to the CRD and remove the string parsing logic in Ray Autoscaler so that Ray Autoscaler doesn't need to handle both rayStartParams["labels"] and Labels.

cc @rueian @Future-Outlier @ryanaoleary @andrewsykim

Based on my memory, we should also remove labels from the env variables in Ray if we want to go with proposal 2.

cc @rueian @ryanaoleary @andrewsykim @kevin85421

[kevin85421]

Based on my memory, we should also remove labels from the env variables in Ray if we want to go with proposal 2.

We should avoid Ray reading env vars to set Ray labels no matter which proposals.

[rueian]

Do we really want to merge labels from pod template metadata with the new top-level field? This will also complicate the autoscaler, making it need to read pod template metadata.

[andrewsykim]

if I'm understanding correctly, the merged labels here are not read by autoscaler. These are labels to write back as Pod labels to provide better visibility of Raylet labels as Pod labels. Autoscaler should still only look at labels field or labels in rayStartParams

[rueian]

Oh thanks. I read it wrongly.

[andrewsykim]

Add Labels to the CRD and remove the string parsing logic in Ray Autoscaler so that Ray Autoscaler doesn't need to handle both rayStartParams["labels"] and Labels.

Would it be so bad for Ray autoscaler to check labels in both places (API field and rayStartParams) for some period of time? I think even if we agree that Labels field is the ideal approach, we need to respect backwards compatibility anyways and still check rayStartParams.

[kevin85421]

Would it be so bad for Ray autoscaler to check labels in both places (API field and rayStartParams) for some period of time? I think even if we agree that Labels field is the ideal approach, we need to respect backwards compatibility anyways and still check rayStartParams.

No strong opinions if @rueian is willing to maintain the complexity in Ray Autoscaler. I propose removing it for now because I think most users don't use label-based scheduling, and even fewer users use label-based scheduling with the Autoscaler at this moment. It's a good timing to get rid of some potential tech debt, and we decide not to support rayStartParams["labels"] in KubeRay v1.5.0.

Have we announced anything about label-based scheduling publicly?

[rueian]

I think leaving the old parsing code untouched in the Ray autoscaler is okay, or just put a TODO for removing it someday :)

What we really need to do is reject any label set from other than direct value in the new top-level labels field. Setting labels via env variables, variable expansions, and rayStartParams["labels"] should all be rejected by KubeRay. This way, we can make sure the Ray autoscaler can get the labels directly from the new top-level labels field.

[ryanaoleary]

To summarize my points in today's community sync,

• Resources: It can improve UX by avoiding the need to specify something like resources: '"{\"Custom1\": 1, \"Custom2\": 5}"' in rayStartParams (example). Therefore, this is safe to add to CRD.

• Labels:

  • The primary goal of adding Labels to the CRD is to avoid parsing the string rayStartParams["labels"] into Ray labels in the Ray Autoscaler. The Ray Autoscaler already has logic to parse the string. Therefore, this benefit would not be realized if we decide not to remove the string parsing logic from the Ray Autoscaler.
  • The other benefit is to verify the label values at the OpenAPI level, but this is not significant enough to justify handling both rayStartParams["labels"] and the new field Labels at the same time.

My proposals for Labels:

• Proposal 1: Not adding Labels to the CRD and keeping the Ray Autoscaler parsing the string is fine with me, but Edward didn't like it.
• Proposal 2: Add Labels to the CRD and remove the string parsing logic in Ray Autoscaler so that Ray Autoscaler doesn't need to handle both rayStartParams["labels"] and Labels.

cc @rueian @Future-Outlier @ryanaoleary @andrewsykim

Proposal 2 sounds good to me too. This PR should contain all the required changes currently and is good to review, I'll update ray-project/ray#57260 to include TODO comments to remove the string parsing logic. I'll also update the label setting logic so that we don't consider environment variables.

[ryanaoleary]

if we want to go with proposal 2

Where are labels currently being set in env vars in KubeRay? We set some default labels in Ray core based on env vars (i.e. accelerator-type for TPU is set using an env var that's set automatically by GKE), but I can remove the ones that we no longer plan to set using KubeRay (i.e. from this closed PR: #3699). Is there anywhere else that env vars are being considered that we need to remove?