Skip to content

Conversation

@nu1lspaxe
Copy link

  1. Remove validation logic in GetWorkerGroupDesiredReplicas (utils.go), and append to ValidateRayClusterSpec.
  2. Add other validation logic for worker group specs.
  3. Remove unnecessary test cases in GetWorkerGroupDesiredReplicas.

Why are these changes needed?

Move worker group replica validation from GetWorkerGroupDesiredReplicas to ValidateRayClusterSpec to centralize and improve spec validation, and remove redundant tests.

Related issue number

#4101 (Move Replica validation to validation.go)

Checks

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

@nu1lspaxe nu1lspaxe force-pushed the fix/validate-before-creation branch from d05d922 to 717af47 Compare October 8, 2025 01:33
Copy link
Collaborator

@win5923 win5923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s keep this PR focused on replicas validation and handle the CI migration in another one.
that would make it easier to review.

@nu1lspaxe nu1lspaxe force-pushed the fix/validate-before-creation branch from 3f37ada to 40b97cf Compare October 9, 2025 01:09
@win5923
Copy link
Collaborator

win5923 commented Oct 9, 2025

Hi @nu1lspaxe, Sorry we merged a Volcano PR which introduced some conflicts.
Would you mind helping to resolve them?

1. Remove validation logic in GetWorkerGroupDesiredReplicas (utils.go),
   and append to ValidateRayClusterSpec.
2. Add other validation logic for worker group specs.
3. Remove unnecessary test cases in GetWorkerGroupDesiredReplicas.

Fix: MinReplicas / MaxReplicas should verify if autoscaling is not
enabled
1. fix golangci-lint test (unused ctx)
@nu1lspaxe nu1lspaxe force-pushed the fix/validate-before-creation branch from 6452117 to 98ef48b Compare October 10, 2025 23:12
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