-
Notifications
You must be signed in to change notification settings - Fork 412
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
Allow overriding just the args in submitter pod #2208
base: master
Are you sure you want to change the base?
Allow overriding just the args in submitter pod #2208
Conversation
// If the command in the submitter pod template isn't set, use the default command. | ||
if len(submitterTemplate.Spec.Containers[utils.RayContainerIndex].Command) == 0 { | ||
// If neither the command nor the args are set in the submitter pod template, use the default command. | ||
if len(submitterTemplate.Spec.Containers[utils.RayContainerIndex].Command) == 0 && len(submitterTemplate.Spec.Containers[utils.RayContainerIndex].Args) == 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.
Can you add a unit test for this case?
@andrewsykim I can invest some time to learn go and how to add tests, but first: are you okay with this breaking change? Anyone who currently uses ray and only specifies the args gets the command overridden. After this gets merged, that will no longer be the case. I am not sure what your stability promises are exactly. |
I'm not sure to be honest , still thinking about it. In general we try our best not to break compatibility per our compatiblitiy promise: https://docs.ray.io/en/latest/cluster/kubernetes/references.html#kuberay-api-compatibility-and-guarantees However, I haven't personally seen a lot of cases of overriding the submitter pod template and not sure how many users this will break. |
For your use-case, why not just override the ENTRYPOINT, to whatever you use in your container? |
Thank you for considering this idea.
My use-case is generic over docker images. I can not assume the ENTRYPOINT is anything in particular. In general, it would help me a bunch if:
all use Additionally, it would also be great if the I opened an issue to discuss this #2209, although that issue is currently geared towards making the submitter pod consistent with the head and worker. |
I think this PR only breaks the case where only args in |
This issue documents a different solution for my use-case (issuing ray submit through a login shell as well, just like what is done for the head and workers). I think it would be good to pick that up too for consistency. Although it raises similar questions about backwards compatibility. |
Why are these changes needed?
My submitter Pod's docker image has configured an ENTRYPOINT which I do not wish to override. I just want to set the
args
but kuberay will override thecommand
when unspecified.Related issue number
Checks