Skip to content

[RayCluster] add annotation to enable non-login bash #3630

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

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

Conversation

fscnick
Copy link
Contributor

@fscnick fscnick commented May 18, 2025

Why are these changes needed?

According to #3247, the -l overwrite the PATH resulting in uv not working properly. Introduce an annotation ray.io/non-login-bash-cmd to remove -l from the command.

Previouly, there is a PR #427 to add -l to be convenient for conda init or or set CLASSPATH. Which might have conflict with this current issue. Thus, add an annotation to let user choose which one is suitable for them.

Related issue number

Part of #3247

Checks

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

@fscnick fscnick force-pushed the add-annotation-non-login-bash branch from 44f3e65 to 26199c6 Compare May 18, 2025 15:18
@fscnick
Copy link
Contributor Author

fscnick commented May 18, 2025

screenshot of head-pod:
head-pod

screenshot of worker-pod:
worker-pod 01
worker-pod 02

screenshot of uv run :
uv-demo

@fscnick fscnick closed this May 18, 2025
@fscnick fscnick reopened this May 18, 2025
@fscnick fscnick marked this pull request as ready for review May 18, 2025 16:24
@fscnick
Copy link
Contributor Author

fscnick commented May 18, 2025

@kevin85421 PTAL

@kevin85421
Copy link
Member

The API is too specific; we should make it more general-purpose. We could start by introducing a KubeRay environment variable to define the default container Command for all Pods created by the KubeRay operator.

KUBERAY_DEFAULT_CONTAINER_COMMAND: If the environment variable is not set, the default container Command is []string{"/bin/bash", "-lc", "--"}. If it is set, containers such as the wait-for-gcs init container, the head Pod’s Ray container, the worker Pods’ Ray containers, and the RayJob submitter’s Command will use the value of the environment variable, split by commas ... etc

(Split by commas may not be a good idea. You should figure it out.)

@AvihaiSam
Copy link

The API is too specific; we should make it more general-purpose. We could start by introducing a KubeRay environment variable to define the default container Command for all Pods created by the KubeRay operator.

KUBERAY_DEFAULT_CONTAINER_COMMAND: If the environment variable is not set, the default container Command is []string{"/bin/bash", "-lc", "--"}. If it is set, containers such as the wait-for-gcs init container, the head Pod’s Ray container, the worker Pods’ Ray containers, and the RayJob submitter’s Command will use the value of the environment variable, split by commas ... etc

(Split by commas may not be a good idea. You should figure it out.)

@kevin85421 one can use different jobs/clusters/services with different base image with the same KubeRay operator.
I think this should be supported either as the suggested annotation, or extend the CRDs to support it.
either way - it should allow the ability to define the exact container command.

@kevin85421
Copy link
Member

kevin85421 commented May 19, 2025

one can use different jobs/clusters/services with different base image with the same KubeRay operator.

I understand, but I don’t want to change the CRD at this moment unless we encounter enough actual issues. It’s hard to remove or change behavior once it’s been added to the CRD.

In most cases, the person who deploys KubeRay and the one who builds the Docker images are typically from the same team, while the person who writes the CR YAML may be from a different team.

I think this change should meet most users' needs. I will reconsider updating CRD if the assumptions are incorrect.

either way - it should allow the ability to define the exact container command.

Users can disable the injection of init container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants