Skip to content

chore: remove unnecessary empty rayStartParams #3586

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.

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

Merged
merged 2 commits into from
Jun 24, 2025

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented May 13, 2025

in example YAMLs and tests. rayStartParams is optional after #3202
released in 1.4.0. We remove them in YAMLs for testing but keep in user-facing examples for backwards compatibility with older CRD versions.

contributes to #3580

Checks

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

@davidxia
Copy link
Contributor Author

depends on #3547

@davidxia davidxia force-pushed the patch29 branch 2 times, most recently from 33506db to 49f4179 Compare May 13, 2025 14:06
@davidxia davidxia force-pushed the patch29 branch 15 times, most recently from 6465363 to 56aa8b8 Compare June 3, 2025 15:00
@davidxia davidxia marked this pull request as ready for review June 3, 2025 15:51
@davidxia
Copy link
Contributor Author

davidxia commented Jun 3, 2025

there are a few other places with rayStartParams: {}, but the associated tests timed out. I wonder if #3724 is needed

@davidxia davidxia changed the title chore: remove unnecessary empty rayStartParams chore: comment out unnecessary empty rayStartParams Jun 20, 2025
@davidxia davidxia mentioned this pull request Jun 20, 2025
2 tasks
@kevin85421
Copy link
Member

would you mind rebasing with the master branch?

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.

  • How about removing the comment "rayStartParams is optional with RayCluster CRD from KubeRay 1.4.0 or later but required in earlier versions"? If we add comments to all YAML files, it will be harder to maintain—we'd need to update dozens of YAML files whenever the comment changes.
  • We can only add the comments to https://github.com/ray-project/kuberay/blob/master/ray-operator/config/samples/ray-cluster.sample.yaml
  • We can remove rayStartParams: {} from YAMLs for testing. For example, YAMLs under apiserver/test/cluster, memory_benchmark, perf-tests, ... etc.
  • However, I would suggest to keep rayStartParams: {} for user-facing examples. I guess some users may follow the Ray website and use their existing KubeRay operator which is older than v1.4.0.

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 <[email protected]>
@davidxia
Copy link
Contributor Author

@kevin85421 sgtm, done!

@kevin85421 kevin85421 merged commit 682c46b into ray-project:master Jun 24, 2025
25 checks passed
@davidxia davidxia changed the title chore: comment out unnecessary empty rayStartParams chore: remove unnecessary empty rayStartParams Jun 24, 2025
@davidxia davidxia deleted the patch29 branch June 24, 2025 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants