[RayJob] Rewrite RayJob envtest #1916
Conversation
| }) | ||
|
|
||
| It("test cluster selector", func() { |
There was a problem hiding this comment.
This test totally doesn't make sense. ClusterSelector must be set before the RayJob's creation. If users set the ClusterSelector after DashboardURL is set, the RayJob may stuck there. Maybe we should consider whether deprecate the ClusterSelector feature or not. Remove "test cluster selector".
There was a problem hiding this comment.
Maybe we should consider whether deprecate the ClusterSelector feature or not.
I never really thought the selector made sense because multiple Ray clusterse can share the same label. Instead this field should just be an object reference or just a name field.:
clusterRef:
name: my-raycluster
OR
clusterName: my-raycluster
There was a problem hiding this comment.
It makes sense to me, but I prefer to deemphasize this feature by not mentioning it in the documentation and avoiding updates. To submit Ray jobs to an existing RayCluster, users should use ray job submit instead of ClusterSelector. We may consider deprecating it or decide to revisit it when we receive sufficient feedback on its use cases from users.
|
cc @andrewsykim |
Why are these changes needed?
The envtest is very messy:
RayJobspec is very different from the real RayJob YAML file.Ittests are irrelevant, yet we include all of them in a single suite with unnecessary dependencies.ClusterSelectortest has already been covered by e2e tests.This PR's tests focus on the most common code path of RayJob, i.e., New -> Initializing -> Running -> Complete.
Related issue number
Checks