Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Use dask cuda worker when GPU resource > 0 #339

Closed
wants to merge 2 commits into from
Closed

Use dask cuda worker when GPU resource > 0 #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76edcb4
add cuda support
ljstrnadiii Mar 28, 2023
6a82451
add simple test for workerspec arg + expected resources
ljstrnadiii Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions go/tasks/plugins/k8s/dask/dask.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ func createWorkerSpec(cluster plugins.DaskWorkerGroup, defaults defaults) (*dask
memory := limits.Memory().String()
workerArgs = append(workerArgs, "--memory-limit", memory)
}
// If limits includes gpu, assume dask cuda worker cli startup
// https://docs.rapids.ai/api/dask-cuda/nightly/quickstart.html#dask-cuda-worker
if !limits.Name(flytek8s.ResourceNvidiaGPU, "0").IsZero() {
workerArgs[0] = "dask-cuda-worker"
}
Comment on lines +190 to +194
Copy link
Member

Choose a reason for hiding this comment

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

After having taken a closer look, I am not so sure whether this is a good default behavior. A few questions I've got:

  • Is this the default behavior that every user wants? E.g. from the docs I saw that it states:

It only helps with deployment and management of Dask workers in multi-GPU systems.

and a lot of k8s deployments will have single GPU workers.

  • Should this command also be applied on GPU requests, not just limits? 🤔
  • Is this the desired behavior in case the default resources (e.g. the task resources) also have GPUs?

Copy link
Author

Choose a reason for hiding this comment

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

Good point! I'll make changes to instead add the option to specify a gpu worker command.

Copy link
Author

@ljstrnadiii ljstrnadiii May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK, after thinking about this for a little and getting somewhat more familiar with the code base(s), tell me if this makes sense:

  • go to flyteidl plugins dask.proto and add a field "command".
  • update flytekit plugins python code for models and task here and here to include the "command" option.
  • lastly, update dask.go to get default command if not specified.

question: This seems like three separate prs. Is this order correct? update proto, update models, update dask.go? Also, "command" could be "startup_command". I don't care, but any suggestions are welcome.

@bstadlbauer if this sounds good to you I will try to get this in over the next week.

}

wokerSpec := v1.PodSpec{
Expand Down
28 changes: 28 additions & 0 deletions go/tasks/plugins/k8s/dask/dask_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,34 @@ func TestBuildResourceDaskDefaultResoureRequirements(t *testing.T) {
assert.Contains(t, workerSpec.Containers[0].Args, "2G")
}

func TestBuildResourceGPUCudaWorkerArgs(t *testing.T) {
protobufResources := core.Resources{
Limits: []*core.Resources_ResourceEntry{
{
Name: core.Resources_GPU,
Value: "1",
},
},
}
expectedResources, _ := flytek8s.ToK8sResourceRequirements(&protobufResources)

flyteWorkflowResources := v1.ResourceRequirements{}

daskResourceHandler := daskResourceHandler{}
taskTemplate := dummyDaskTaskTemplate("", &protobufResources)
taskContext := dummyDaskTaskContext(taskTemplate, &flyteWorkflowResources, false)
resource, err := daskResourceHandler.BuildResource(context.TODO(), taskContext)
assert.Nil(t, err)
assert.NotNil(t, resource)
daskJob, ok := resource.(*daskAPI.DaskJob)
assert.True(t, ok)

// Default Workers
workerSpec := daskJob.Spec.Cluster.Spec.Worker.Spec
assert.Equal(t, *expectedResources, workerSpec.Containers[0].Resources)
assert.Contains(t, workerSpec.Containers[0].Args, "dask-cuda-worker")
}

func TestBuildResourcesDaskCustomResoureRequirements(t *testing.T) {
protobufResources := core.Resources{
Requests: []*core.Resources_ResourceEntry{
Expand Down