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

[Feature][RFC] RayCluster CRD: interface cleanup #368

Closed
1 of 2 tasks
DmitriGekhtman opened this issue Jul 10, 2022 · 5 comments
Closed
1 of 2 tasks

[Feature][RFC] RayCluster CRD: interface cleanup #368

DmitriGekhtman opened this issue Jul 10, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request P1 Issue that should be fixed within a few weeks

Comments

@DmitriGekhtman
Copy link
Collaborator

DmitriGekhtman commented Jul 10, 2022

Search before asking

  • I had searched in the issues and found no similar feature requirement.

Description

Minimal configuration for RayCluster CRs should be as simple as possible. Some suggestions:

  • Make rayStartParams optional. If specifying {} is valid, not specifying the field at all should also be valid.
    • Most users shouldn't have to specify any rayStartParams at all.
    • Consider always adding --block. The current default behavior of adding a "sleep infinity" after ray start is unintuitive and probably incorrect.
  • Make serviceType optional. Default to ClusterIP -- this is K8s's default and what most users would expect.

Use case

Make default configuration as simple and minimal as possible for standard use-cases.

Related issues

No response

Are you willing to submit a PR?

  • Yes I am willing to submit a PR!
@DmitriGekhtman DmitriGekhtman added the enhancement New feature or request label Jul 10, 2022
@DmitriGekhtman DmitriGekhtman changed the title [Feature] RayCluster CRD: interface nits. [Feature][RFC] RayCluster CRD: interface nits. Jul 10, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 12, 2022

Most users shouldn't have to specify any rayStartParams at all.

If that's optional, are there minimum configs to make Ray start up work? If so, we can append args on behalf of users.

Consider always adding --block

Agree. Bytedance enable it by default in downstream version.

Make serviceType optional. Default to ClusterIP

Agree. Additionally, we added these fields in CRD in the past. We are considering to move these fields to annotation instead.

@DmitriGekhtman
Copy link
Collaborator Author

If that's optional, are there minimum configs to make Ray start up work? If so, we can append args on behalf of users?

Besides what we already do, it may just be --block. We should double-check.

@Jeffwan Jeffwan added this to the v0.4.0 release milestone Jul 27, 2022
@DmitriGekhtman DmitriGekhtman changed the title [Feature][RFC] RayCluster CRD: interface nits. [Feature][RFC] RayCluster CRD: interface cleanup Aug 15, 2022
@DmitriGekhtman DmitriGekhtman added the P1 Issue that should be fixed within a few weeks label Nov 4, 2022
@DmitriGekhtman
Copy link
Collaborator Author

cc @kevin85421 @sihanwang41 on the entry-point issues
Link to the discussion: https://ray-distributed.slack.com/archives/C02GFQ82JPM/p1669647595429959

@Jeffwan
Copy link
Collaborator

Jeffwan commented Mar 15, 2023

#861 If we plan to make any API changes (remove any unhelpful fields), clean up is a breaking change and let's support multi version in beta version.

@kevin85421
Copy link
Member

The last TODO is #976. Close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 Issue that should be fixed within a few weeks
Projects
None yet
Development

No branches or pull requests

3 participants