[Flyte-7005] Supporting Ray autoscalerOptions#7111
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7111 +/- ##
=======================================
Coverage 56.95% 56.96%
=======================================
Files 931 931
Lines 58250 58273 +23
=======================================
+ Hits 33179 33197 +18
- Misses 22017 22022 +5
Partials 3054 3054
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
951fbeb to
8a99445
Compare
|
Hi @0yukali0, |
|
Hi @machichima , i updated the setup steps and screenshot in the PR descriptions. |
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
| resourceRequirements := v1.ResourceRequirements{} | ||
| if requests := res.GetRequests(); len(requests) > 0 { | ||
| resourceRequirements.Requests = convertResourceEntriesToResourceList(requests) | ||
| } | ||
| if limits := res.GetLimits(); len(limits) > 0 { | ||
| resourceRequirements.Limits = convertResourceEntriesToResourceList(limits) | ||
| } |
There was a problem hiding this comment.
Could we use flytek8s.ToK8sResourceRequirements here instead?
flyte/flyteplugins/go/tasks/pluginmachinery/flytek8s/utils.go
Lines 52 to 68 in 6e84ac2
| autoScalarOptions.Env = []v1.EnvVar{} | ||
| for _, env := range envs { | ||
| name := env.GetKey() | ||
| if val := env.GetValue(); val != "" { | ||
| autoScalarOptions.Env = append(autoScalarOptions.Env, v1.EnvVar{ | ||
| Name: name, | ||
| Value: val, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we use flytek8s.ToK8sEnvVar instead?
| idleTimeoutTime := options.GetIdleTimeoutSeconds() | ||
| autoScalarOptions.IdleTimeoutSeconds = &idleTimeoutTime |
There was a problem hiding this comment.
If the idleTimeoutTime is not set (= 0), this part will overwrite the default value (60 sec) set in kuberay. we should only set autoScalarOptions.IdleTimeoutSeconds if it's not 0 (not empty)
There was a problem hiding this comment.
Hi @machichima
I will update it and the following test case to check the setting.
| } | ||
|
|
||
| func buildAutoscalerOptions(options *plugins.AutoscalerOptions) *rayv1.AutoscalerOptions { | ||
| var autoScalarOptions *rayv1.AutoscalerOptions |
There was a problem hiding this comment.
| var autoScalarOptions *rayv1.AutoscalerOptions | |
| var autoScalerOptions *rayv1.AutoscalerOptions |
nit
| } | ||
| } | ||
|
|
||
| func TestNewAutoscalerOptions(t *testing.T) { |
There was a problem hiding this comment.
| func TestNewAutoscalerOptions(t *testing.T) { | |
| func TestBuildAutoscalerOptions(t *testing.T) { |
nit
| assert.Equal(t, resource.MustParse("1Gi"), result.Resources.Limits[corev1.ResourceMemory]) | ||
| }) | ||
|
|
||
| t.Run("env literal value", func(t *testing.T) { |
There was a problem hiding this comment.
Could we add a test for env with empty value (should be skipped)?
There was a problem hiding this comment.
flytek8s.ToK8sEnvVar allows empty value, so i will skip the test with empty value.
| assert.Nil(t, buildAutoscalerOptions(nil)) | ||
| }) | ||
|
|
||
| t.Run("idle timeout propagated", func(t *testing.T) { |
There was a problem hiding this comment.
Could we also add a test like this:
t.Run("idle timeout zero should not be set", func(t *testing.T) {
result := buildAutoscalerOptions(&plugins.AutoscalerOptions{})
require.NotNil(t, result)
assert.Nil(t, result.IdleTimeoutSeconds)
})Which is related to my comment above: https://github.com/flyteorg/flyte/pull/7111/changes#r3114611720
If the IdleTimeoutSeconds is not set, it should remain empty so that Ray can apply its default value
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
Signed-off-by: Yuteng Chen <a08h0283@gmail.com>
| func convertResourceEntriesToResourceList(entries []*core.Resources_ResourceEntry) v1.ResourceList { | ||
| resourceList := v1.ResourceList{} | ||
| for _, entry := range entries { | ||
| var name v1.ResourceName | ||
| switch entry.GetName() { | ||
| case core.Resources_CPU: | ||
| name = v1.ResourceCPU |
There was a problem hiding this comment.
Do we use it anywhere? if not, can we remove it
| // custom autoscaler image | ||
| string image = 4; | ||
| // autoscaler container resources | ||
| core.Resources resources = 5; |
There was a problem hiding this comment.
Should we use core.K8sPod k8s_pod instead of core.Resources? SDk can use this podSpec to override to resource, label or annotation. same as HeadGroupSpec and WorkerGroupSpec
There was a problem hiding this comment.
No, AutoscalerOptions can't follow same solution as HeadGroupSpec and WorkerGroupSpec, the reason is following.
The api doc describes that AutoscalerOptions currently only support corev1.ResourceRequirements when HeadGroupSpec and WorkerGroupSpec support corev1.PodTemplateSpec.
There was a problem hiding this comment.
Same situation also exists in v1 version
Signed-off-by: yuteng chen <a08h0283@gmail.com>


Tracking issue
Related #7005
Why are the changes needed?
Ray autoScalerOptions is not supportted in flyte v1
What changes were proposed in this pull request?
How was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
2.1 flytectl demo start --dev
2.2 make compile && POD_NAMESPACE=flyte ./flyte start --config flyte-single-binary-local.yaml
3.1 uv venv -p 3.11 && source .venv/bin/activate && uv pip install -e ~/flytekit ~/flytekit/plugins/flytekit-ray && uv pip install -e ~/flyte/flyteidl
3.2 "pyflyte run --remote ray_example.py ray_workflow --n 10"
Screenshots
Related PRs
Docs link