Skip to content

Conversation

@Bobbins228
Copy link
Contributor

@Bobbins228 Bobbins228 commented Nov 29, 2023

Issue link

Closes: #410

What changes have been made

Added the RayJobClient object for use of RayJobSubmission related commands with the SDK.
This allows us to leverage the SDK to submit jobs to Ray Clusters that were either not created by the SDK or are in a non Kubernetes environment.

Verification steps

  1. Create a Ray Cluster - (I recommend the mnist example RC)
  2. Run this example job submission
from codeflare_sdk.job.ray_jobs import RayJobClient

client = RayJobClient(
    address="<RAY_DASHBOARD_ADDRESS>"
)

job_id = client.submit_job(
    entrypoint="python mnist.py",
    runtime_env={"working_dir": "./","pip": "requirements.txt"},
)

client.get_job_status(job_id)
  1. Check the dashboard for the new job which should be running.
  2. Try out the other commands listed below
client.get_job_info(job_id)
client.get_job_status(job_id)
client.stop_job(job_id)
client.delete_job(job_id)
client.address()

# tail_job_logs returns an Iterator so we must iterate through it and print each line
async for lines in client.tail_job_logs(job_id):
    print(lines, end="") 

Checks

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

Copy link
Contributor

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

Great job Mark! I left a few requested changes below. Apart from those, I noticed there are some functions that are not unit tested, is there a reason for that? Just thinking about code coverage.

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 30, 2023
Copy link
Contributor

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

/lgtm thanks!

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

@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.

Thanks for the changes. Just some styling and docs nitpicks

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
@Bobbins228
Copy link
Contributor Author

@KPostOffice Added in the param descriptions from the ray docs and Changed the list_jobs return type to List[JobDetails]

Copy link
Contributor

@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.

Thanks for addressing the comments

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

openshift-ci bot commented Dec 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChristianZaccaria, KPostOffice

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 Dec 6, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 073b171 into project-codeflare:main Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

[Feature] Implement Ray Job Submission for Ray Clusters not in a Kubernetes environment

3 participants