-
Notifications
You must be signed in to change notification settings - Fork 683
[Config] Standardize quoted resource values in sample manifests. #4339
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
Conversation
|
@CheyuWu Would love a review. |
Future-Outlier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, plz update these files too, thank you.
also cc @cheyu @justinyeh1995 @400Ping to take a look.
Missing Files in PR
1. Volcano Queue spec (capability field)
| File | Lines | Current | Should be |
|---|---|---|---|
ray-cluster.volcano-scheduler-queue.yaml |
8-9 | cpu: 4, memory: 6Gi |
cpu: "4", memory: "6Gi" |
ray-job.volcano-scheduler-queue.yaml |
8-9 | cpu: 4, memory: 6Gi |
cpu: "4", memory: "6Gi" |
2. Head Group resources - memory not quoted
| File | Lines | Current | Should be |
|---|---|---|---|
ray-cluster.tpu-v6e-singlehost.yaml |
18, 21 | memory: 40G |
memory: "40G" |
ray-cluster.tpu-v6e-16-multihost.yaml |
18, 21 | memory: 40G |
memory: "40G" |
ray-cluster.tpu-v4-multihost.yaml |
21, 22, 25, 26 | ephemeral-storage: 20Gi, memory: 40G, ephemeral-storage: 10Gi, memory: 40G |
add quotes |
ray-cluster.tpu-v4-singlehost.yaml |
21 | ephemeral-storage: 20Gi |
ephemeral-storage: "20Gi" |
ray-cluster.external-redis.yaml |
35, 38, 85, 88 | memory: 5Gi, memory: 2Gi, memory: 1Gi, memory: 1Gi |
add quotes |
ray-cluster.external-redis-uri.yaml |
35, 38, 85, 88 | same as above | add quotes |
3. RayJob head group
| File | Lines | Current | Should be |
|---|---|---|---|
ray-job.tpu-v6e-256-multihost.yaml |
33 | memory: 40G |
memory: "40G" |
|
Have updated the requested files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| rayClusterSpec: | ||
| headGroupSpec: | ||
| rayStartParams: | ||
| disable-usage-stats: 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small follow-up I noticed: besides cpu and memory, there are also some non-resource values still using single quotes (').
Since this PR aims to standardize quoting style, would it make sense to also convert those to double quotes (") for consistency across all sample manifests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just about params. I noticed the same thing with annotations as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, Do you think it makes sense to include those changes here as well, or would you prefer handling them in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be ideal if we move that to a separate PR since we will changing the production files.
@justinyeh1995 As asked in the issue proposal, only the changes needs to be made in the sample files to avoid behavioural change in production. So the changes have been made to files present in |
Hi @justinyeh1995 I think we can open a new issue to address the problem |
Ahh, I see. This makes sense. Let me check again. |
Sounds good to me! |
|
@justinyeh1995 Any more changes required which lie within the scope of this issue? |
| custom-label: custom-ray-head-service-label | ||
| annotations: | ||
| custom-annotation: custom-ray-head-service-annotation | ||
| custom-annotation: "custom-ray-head-service-annotation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: revert this back to unquoted as the string is unambiguous and it is a more common pattern.
justinyeh1995
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future-Outlier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursor review
|
cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
Future-Outlier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
|
cc @rueian to merge |
CheyuWu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM




Why are these changes needed?
Currently, resource values (e.g. cpu, memory) in sample Kubernetes manifests are inconsistent: some are quoted as strings, while others are not.
This PR standardizes all sample manifest files to use
double-quoted ("")resource values.Related issue number
Closes #4335
Checks