-
Notifications
You must be signed in to change notification settings - Fork 438
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
[kubectl-plugin] add --worker-gpu flag for cluster creation #2675
Conversation
Signed-off-by: Andrew Sy Kim <[email protected]>
@chiayi PTAL |
@@ -85,6 +89,7 @@ func TestConvertRayClusterApplyConfigToYaml(t *testing.T) { | |||
WorkerReplicas: 3, | |||
WorkerCPU: "2", | |||
WorkerMemory: "10Gi", | |||
WorkerGPU: "0", |
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.
Do you think it is worth it to check the yaml output when the GPU is non-zero? Or is it enough to see that the value is properly set in the applyconfig?
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, will add a test for when GPU is set to 1
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.
ah nvm, I already updated the test TestGenerateRayCluterApplyConfig
to set GPU to 1
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.
Oh oops my bad, missed that.
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!
@@ -98,6 +99,18 @@ func (rayClusterSpecObject *RayClusterSpecObject) generateRayClusterSpec() *rayv | |||
corev1.ResourceMemory: resource.MustParse(rayClusterSpecObject.WorkerMemory), | |||
})))))) | |||
|
|||
gpuResource := resource.MustParse(rayClusterSpecObject.WorkerGPU) | |||
if !gpuResource.IsZero() { |
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.
Since we are adding the GPU option, should we add the option to specify the gke-accelerator
?
I do think this could also be done on a later date though.
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.
We probably want to generalize it further and support an option to specify node selectors
Why are these changes needed?
Add the option to specify worker GPU count. For now we default to nvidia.com/gpu but we can support other GPU types in the future.
Related issue number
#2608
Checks