Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Helm] Clean up RayCluster Helm chart ahead of KubeRay 0.4.0 release #751

Merged
merged 7 commits into from
Nov 22, 2022
Merged

[Helm] Clean up RayCluster Helm chart ahead of KubeRay 0.4.0 release #751

merged 7 commits into from
Nov 22, 2022

Conversation

DmitriGekhtman
Copy link
Collaborator

@DmitriGekhtman DmitriGekhtman commented Nov 21, 2022 •
edited
Loading

Why are these changes needed?

This PR cleans up the ray-cluster Helm chart.

  • Removes unnecessary and deprecated fields.
  • Adds some comments
  • Renames initArgs to rayStartParams in a backwards-compatible way.

Related issue number

Closes #739

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests: I deployed a Ray cluster using the default chart.
    • This PR is not tested :(

@DmitriGekhtman
simplify
e523930 
Signed-off-by: Dmitri Gekhtman <dmitri.m.gekhtman@gmail.com>
@DmitriGekhtman
Remove head replicas
4ee4bfb 
Signed-off-by: Dmitri Gekhtman <dmitri.m.gekhtman@gmail.com>
@DmitriGekhtman
rayStartParams
ac8aa9f 
Signed-off-by: Dmitri Gekhtman <dmitri.m.gekhtman@gmail.com>
@DmitriGekhtman
Fix ports
34c4e17 
Signed-off-by: Dmitri Gekhtman <dmitri.m.gekhtman@gmail.com>
@DmitriGekhtman
lint
b73c593 
Signed-off-by: Dmitri Gekhtman <dmitri.m.gekhtman@gmail.com>
kevin85421
kevin85421 reviewed Nov 22, 2022
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

This PR deletes a lot of redundant environment variables (I am not sure how to check whether they are redundant or not). An environment variable may be used by KubeRay or Ray. Is there any document about environment variables for Ray? Others look good to me.

helm-chart/ray-cluster/templates/raycluster-cluster.yaml Show resolved Hide resolved
helm-chart/ray-cluster/values.yaml Outdated Show resolved Hide resolved
@DmitriGekhtman @kevin85421
Update helm-chart/ray-cluster/values.yaml
0b309e1 
Co-authored-by: Kai-Hsun Chen <kaihsun@apache.org>
Signed-off-by: Dmitri Gekhtman <62982571+DmitriGekhtman@users.noreply.github.com>
@DmitriGekhtman
Copy link
Collaborator Author

This PR deletes a lot of redundant environment variables (I am not sure how to check whether they are redundant or not). An environment variable may be used by KubeRay or Ray. Is there any document about environment variables for Ray? Others look good to me.

I can certify that none of the environment variables are used by recent versions of Ray or KubeRay.
I did a similar cleanup of RayCluster CRs just before the 0.3.0 release.

@DmitriGekhtman
Update helm-chart/ray-cluster/values.yaml
80d545d 
Signed-off-by: Dmitri Gekhtman <62982571+DmitriGekhtman@users.noreply.github.com>
kevin85421
kevin85421 approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

LGTM

@DmitriGekhtman DmitriGekhtman merged commit 3a512da into ray-project:master Nov 22, 2022
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
@DmitriGekhtman
[Helm] Clean up RayCluster Helm chart ahead of KubeRay 0.4.0 release (r…
4db86ce 
…ay-project#751)

This PR cleans up the ray-cluster Helm chart.

Removes unnecessary and deprecated fields.
Adds some comments
Renames initArgs to rayStartParams in a backwards-compatible way.

Signed-off-by: Dmitri Gekhtman <dmitri.m.gekhtman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin85421 kevin85421 kevin85421 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] After KubeRay 0.4.0 release, emphasize Helm
2 participants
@DmitriGekhtman @kevin85421