[refactor][operator]: make RayStartParams optional
#3202
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a8efa58 to
a1e53d9
Compare
omitempty on RayStartParams
c9781e1 to
80eb350
Compare
andrewsykim
reviewed
Mar 17, 2025
davidxia
commented
Mar 17, 2025
There was a problem hiding this comment.
the important changes are in this file
Comment on lines
89
to
149
| // TODO: Look for better workaround/fixes for RayStartParams. Currently using `WithRayStartParams()` requires | ||
| // a non-empty map with valid key value pairs and will not populate the field with empty/nil values. This | ||
| // isn't ideal as it forces the generated RayCluster yamls to use those parameters. | ||
| headRayStartParams := map[string]string{ | ||
| "dashboard-host": "0.0.0.0", | ||
| } | ||
| workerRayStartParams := map[string]string{ | ||
| "metrics-export-port": "8080", | ||
| } |
There was a problem hiding this comment.
can remove this now since the map can be empty
98625b2 to
973af72
Compare
davidxia
commented
Mar 21, 2025
aa3243a to
8b0650b
Compare
omitempty on RayStartParamsRayStartParams optional
8b0650b to
6359479
Compare
davidxia
commented
Mar 23, 2025
Comment on lines
57
to
60
| expectedWorkerRayStartParams := map[string]string{ | ||
| "metrics-export-port": "8080", | ||
| } | ||
| maps.Copy(expectedWorkerRayStartParams, testRayClusterYamlObject.WorkerRayStartParams) |
There was a problem hiding this comment.
We can remove the hard-coded metrics-export-port now.
Comment on lines
-184
to
-346
| rayStartParams: | ||
| metrics-export-port: "8080" |
There was a problem hiding this comment.
This shows that when rayStartParams aren't set in the RayClusterApplyConfiguration, the field is absent from the serialized YAML.
05f6a13 to
557b90a
Compare
davidxia
commented
Mar 24, 2025
* `RayClusters` with `rayStartParams: {}` will continue to be serialized with
the empty map.
* `RayClusters` without `rayStartParams` will be serialized without the field.
This prevents a backwards-incompatible change with RayClusters that use the
autoscaler with Ray versions earlier than 2.45.0 which lack this change
ray-project/ray#51954.
Signed-off-by: David Xia <david@davidxia.com>
e940dbe to
b1c0d28
Compare
|
@kevin85421 I think b1c0d28 makes backwards compatible. |
kevin85421
approved these changes
May 11, 2025
2 tasks
|
Open a follow up issue: #3580 |
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
May 13, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
4 tasks
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
May 13, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
May 13, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
pawelpaszki
pushed a commit
to opendatahub-io/kuberay
that referenced
this pull request
May 26, 2025
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 1, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 1, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 1, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 1, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 2, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 3, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 3, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 20, 2025
in example YAMLs and tests. `rayStartParams` is optional after ray-project#3202 released in 1.4.0. We comment out with an explanation instead of removing entirely so users with older RayCluster CRD versions have a hint about the incompatibility. Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
Jun 23, 2025
in example YAMLs and tests. `rayStartParams` is optional after ray-project#3202 released in 1.4.0. We comment out with an explanation instead of removing entirely so users with older RayCluster CRD versions have a hint about the incompatibility. Signed-off-by: David Xia <david@davidxia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
in
RayClusterCRD by marking it with// +optional.Make it not serialized when empty with
omitempty.This change is backwards compatible on K8s clusters with existing
RayCluster CRD and RayCluster CRs.
before this change.
before this change: as an empty map,
{}in YAML.kubectl create|applyRayClusters that lackRayStartParams in both head and worker groups. Previously the K8s API
would return validation errors requiring this field to be present.
When we get these RayClusters back with
kubectl get raycluster -o yaml, there's norayStartParamsfield present.See Optional vs. Required in K8s API Conventions doc.
The required changes to the RayCluster CRD YAML can be achieved with only
omitempty, but we also add// +optionalas recommended by the doc above.Signed-off-by: David Xia david@davidxia.com