Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mickvangelderen
Copy link

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 the command when unspecified.

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

// 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 {
Copy link
Collaborator

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?

@mickvangelderen
Copy link
Author

@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.

@andrewsykim
Copy link
Collaborator

@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.

@andrewsykim
Copy link
Collaborator

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 the command when unspecified.

For your use-case, why not just override the ENTRYPOINT, to whatever you use in your container?

@mickvangelderen
Copy link
Author

mickvangelderen commented Jun 28, 2024

I'm not sure to be honest , still thinking about it.

Thank you for considering this idea.

For your use-case, why not just override the ENTRYPOINT, to whatever you use in your container?

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:

  • the head pod,
  • the worker pod, and the
  • the submitter pod

all use args instead of command so that if a Dockerfile provides ray thanks to some custom ENTRYPOINT, everything "just works".

Additionally, it would also be great if the entrypoint field in the RayJob spec was a string[]. If that is the case, ray can just append the entrypoint to the ray submitter command and use that as args. No one needs to spawn login shells and construct a correct shell string for '-c'.

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.

@kevin85421
Copy link
Member

I think this PR only breaks the case where only args in submitterPodTemplate are set. You are the first one I've heard mention this case. It is OK for me. Let me discuss this with the community next week. I will keep you updated.

@kevin85421 kevin85421 self-assigned this Jul 5, 2024
@mickvangelderen
Copy link
Author

I think this PR only breaks the case where only args in submitterPodTemplate are set. You are the first one I've heard mention this case. It is OK for me. Let me discuss this with the community next week. I will keep you updated.

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.

@kevin85421 kevin85421 added 1.3.0 and removed 1.2.0 labels Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants