Skip to content

Add Job Cancelling and More Unit Tests #79

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
Merged
merged 3 commits into from
Mar 31, 2023
Merged

Add Job Cancelling and More Unit Tests #79

merged 3 commits into from
Mar 31, 2023

Conversation

Maxusmusti
Copy link
Collaborator

@Maxusmusti Maxusmusti commented Mar 30, 2023 •
edited
Loading

Added:

  • Job cancelling + unit test
  • More unit tests for torchx-mcad jobs

@Maxusmusti Maxusmusti marked this pull request as ready for review March 31, 2023 15:47
KPostOffice
KPostOffice reviewed Mar 31, 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.

Everything else looks good at first glance. Just had a question.

@KPostOffice
Copy link
Collaborator

Can you also make cluster default to None so the user isn't required to pass any args?

codeflare-sdk/src/codeflare_sdk/job/jobs.py

Line 156 in 8266307

def __init__(self, job_definition: "DDPJobDefinition", cluster: "Cluster"):

MichaelClifford
MichaelClifford requested changes Mar 31, 2023
View reviewed changes
@MichaelClifford
Copy link
Collaborator

/LGTM

@Maxusmusti Maxusmusti merged commit 35b1abd into main Mar 31, 2023
@Maxusmusti Maxusmusti deleted the jobcancel branch April 4, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelClifford MichaelClifford MichaelClifford approved these changes

@KPostOffice KPostOffice Awaiting requested review from KPostOffice

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Maxusmusti @KPostOffice @MichaelClifford