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

[Bug] HeadGroupSpec.Replicas is unused #612

Closed
2 tasks done
kevin85421 opened this issue Oct 4, 2022 · 7 comments · Fixed by #1589
Closed
2 tasks done

[Bug] HeadGroupSpec.Replicas is unused #612

kevin85421 opened this issue Oct 4, 2022 · 7 comments · Fixed by #1589
Assignees
Labels
bug Something isn't working P2 Important issue, but not time critical

Comments

@kevin85421
Copy link
Member

Search before asking

  • I searched the issues and found no similar issues.

KubeRay Component

ray-operator

What happened + What you expected to happen

This bug is found by #605. When I updated the value of headGroupSpec.replicas from 1 to 2. It breaks this rule. The number of workers is determined by worker.Replicas (code), but HeadGroupSpec.Replicas is unused in KubeRay.

Possible solutions:
(1) Remove the HeadGroupSpec.Replicas field.
(2) Create head pods based on HeadGroupSpec.Replicas.

Reproduction script

  • Updated the value of headGroupSpec.replicas in your YAML file from 1 to 2.

Anything else

No response

Are you willing to submit a PR?

  • Yes I am willing to submit a PR!
@kevin85421 kevin85421 added the bug Something isn't working label Oct 4, 2022
@kevin85421 kevin85421 self-assigned this Oct 4, 2022
@kevin85421
Copy link
Member Author

cc @DmitriGekhtman

kevin85421 added a commit to kevin85421/kuberay that referenced this issue Oct 4, 2022
@DmitriGekhtman
Copy link
Collaborator

DmitriGekhtman commented Oct 4, 2022

Thank you for tracking this!
See also #362, which made the field optional.

I think we should remove the field in the next RayCluster API version.

@DmitriGekhtman
Copy link
Collaborator

So the solution is bullet (2) in your proposal.
I believe I've already removed the field in all example docs and documented the field as deprecated in the raycluster_types.go.

@DmitriGekhtman
Copy link
Collaborator

Oh, actually I think I left it in the examples for compatibility with KubeRay 0.2.0.
But now that we've released KubeRay 0.3.0, we can remove the field from all examples!
@kevin85421 would you mind doing the honors of hunting down and deleting all instances of HeadGroupSpec.replicas in example yamls?

@kevin85421
Copy link
Member Author

Thank @DmitriGekhtman for the reply! Why do not we remove HeadGroupSpec.Replicas from raycluster_types.go? I tried to remove the field from raycluster_types.go, and it can still keep backward compatibility.

# Step 1: Remove  `HeadGroupSpec.Replicas` from `raycluster_types.go`
# Step 2: Update CRD / generated API
make sync

# Step 3: zz_generated_deepcopy.go
make generate 

# Step4: Build docker image
make docker-image

# Step 5: Install new KubeRay operator (controller:latest) and new CRD
# Step 6: Install RayCluster with `HeadGroupSpec.Replicas`

# Work well.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Oct 4, 2022

We have an issue to track it and we need to do some clean up on the api.

@kevin85421
Copy link
Member Author

Related issue: #368

kevin85421 added a commit to kevin85421/kuberay that referenced this issue Oct 21, 2022
@kevin85421 kevin85421 added the P2 Important issue, but not time critical label Oct 21, 2022
kevin85421 added a commit to kevin85421/kuberay that referenced this issue Oct 21, 2022
davidxia added a commit to davidxia/kuberay that referenced this issue Oct 31, 2023
This field has been deprecated since 0.3.0.
[Related comment](ray-project#612 (comment))
davidxia added a commit to davidxia/kuberay that referenced this issue Oct 31, 2023
in example YAMLs

This field has been deprecated since 0.3.0.
[Related comment](ray-project#612 (comment))
davidxia added a commit to davidxia/kuberay that referenced this issue Oct 31, 2023
It has been deprecated since 0.3.0.

Changes to generated files made by running `cd ray-operator && make sync
generate manifests`.

closes ray-project#612
davidxia added a commit to davidxia/kuberay that referenced this issue Oct 31, 2023
It has been deprecated since 0.3.0.

Changes to generated files made by running `cd ray-operator && make sync
generate manifests`.

closes ray-project#612
davidxia added a commit to davidxia/kuberay that referenced this issue Oct 31, 2023
It has been deprecated since 0.3.0.

Changes to generated files made by running `cd ray-operator && make sync
generate manifests`.

closes ray-project#612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P2 Important issue, but not time critical
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants