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.

Sign up for GitHub

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

Jump to bottom

Adding direct KubeRay compatibility to the SDK #358

Merged
merged 10 commits into from
Oct 23, 2023
Merged

Adding direct KubeRay compatibility to the SDK #358

merged 10 commits into from
Oct 23, 2023

Conversation

Maxusmusti
Copy link
Collaborator

@Maxusmusti Maxusmusti commented Sep 25, 2023 •
edited
Loading

Issue link

#359

What changes have been made

Working on adding direct KubeRay support (generate and manage RayClusters)

Verification steps

Manual test of regular flow, as well as new mcad=False in ClusterConfiguration flow

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2023
@Maxusmusti Maxusmusti marked this pull request as ready for review October 3, 2023 18:07
@openshift-ci openshift-ci bot requested a review from anishasthana October 3, 2023 18:08
@Maxusmusti Maxusmusti changed the title WIP: Adding direct KubeRay compatibility to the SDK Adding direct KubeRay compatibility to the SDK Oct 3, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2023
KPostOffice
KPostOffice reviewed Oct 3, 2023
View reviewed changes
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

A few comments/questions

src/codeflare_sdk/utils/generate_yaml.py Outdated Show resolved Hide resolved
src/codeflare_sdk/cluster/cluster.py Outdated Show resolved Hide resolved
src/codeflare_sdk/cluster/cluster.py Outdated Show resolved Hide resolved
src/codeflare_sdk/cluster/cluster.py Show resolved Hide resolved
src/codeflare_sdk/cluster/cluster.py Show resolved Hide resolved
src/codeflare_sdk/cluster/cluster.py Outdated Show resolved Hide resolved
@Maxusmusti Maxusmusti requested a review from KPostOffice October 9, 2023 16:38
@MichaelClifford
Copy link
Collaborator

Tested! works as expected. Thanks @Maxusmusti

MichaelClifford
MichaelClifford previously requested changes Oct 9, 2023
View reviewed changes
src/codeflare_sdk/cluster/cluster.py Outdated Show resolved Hide resolved
src/codeflare_sdk/cluster/cluster.py Outdated Show resolved Hide resolved
@Maxusmusti Maxusmusti self-assigned this Oct 13, 2023
@anishasthana anishasthana requested review from Bobbins228 and dimakis and removed request for MichaelClifford October 18, 2023 17:06
@anishasthana anishasthana dismissed MichaelClifford’s stale review October 19, 2023 19:16

Michael is out right now.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2023
Bobbins228
Bobbins228 requested changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

Really cool stuff I was able to successfully create ray clusters with and without mcad!
Just needs a rebase now and I can approve 👍

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2023
@Maxusmusti Maxusmusti requested a review from Bobbins228 October 23, 2023 14:55
Bobbins228
Bobbins228 approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

Ran through after rebase all is as it should nice work /approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023
@Maxusmusti
Copy link
Collaborator Author

Thanks @Bobbins228, just rebased on Kevin's dashboard oauth changes, if everything still works as expected then it should be good to go!

@Bobbins228 Bobbins228 merged commit 830cbce into project-codeflare:main Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelClifford MichaelClifford MichaelClifford left review comments

@Bobbins228 Bobbins228 Bobbins228 approved these changes

@anishasthana anishasthana Awaiting requested review from anishasthana

@KPostOffice KPostOffice Awaiting requested review from KPostOffice

@dimakis dimakis Awaiting requested review from dimakis

Assignees

@MichaelClifford MichaelClifford

@Maxusmusti Maxusmusti

@Bobbins228 Bobbins228

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Maxusmusti @MichaelClifford @KPostOffice @Bobbins228 @openshift-merge-robot