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

Implement cancel job operation for CLI #695

Merged
merged 1 commit into from
Dec 15, 2024
Merged

Conversation

amritghimire
Copy link
Contributor

@amritghimire amritghimire commented Dec 12, 2024

This introduces a cancel job operation for CLI that can be used to
cancel a job running in studio.

Command:
datachain studio cancel <JOB_ID

This introduces a cancel job operation for CLI that can be used to
cancel a job running in studio.

Command:
`datachain studio cance <JOB_ID`
Copy link

cloudflare-workers-and-pages bot commented Dec 12, 2024

Deploying datachain-documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6fce628
Status: ✅  Deploy successful!
Preview URL: https://6b4cda96.datachain-documentation.pages.dev
Branch Preview URL: https://amrit-cancel-job.datachain-documentation.pages.dev

View logs

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.32%. Comparing base (b67d599) to head (6fce628).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/datachain/studio.py 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #695   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files         113      113           
  Lines       10798    10817   +19     
  Branches     1480     1483    +3     
=======================================
+ Hits         9429     9446   +17     
- Misses        991      992    +1     
- Partials      378      379    +1     
Flag Coverage Δ
datachain 87.26% <90.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amritghimire amritghimire requested a review from a team December 14, 2024 05:57
Copy link
Contributor

@dreadatour dreadatour left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I am not sure datachain studio cancel is a good command name, it is too general and not verbose enough IMO. But I have no better options.

@amritghimire amritghimire merged commit fc8c7c1 into main Dec 15, 2024
34 checks passed
@amritghimire amritghimire deleted the amrit/cancel-job branch December 15, 2024 07:29
@shcheklein
Copy link
Member

thanks @amritghimire !

datachain studio cancel <JOB_ID

let's rename it please though.

It should be:

datachain job cancel --studio (similar to ds)

same with run, same with API - it should be consistent + we should be using PUT, POST, DELETE, etc

@amritghimire
Copy link
Contributor Author

thanks @amritghimire !

datachain studio cancel <JOB_ID

let's rename it please though.

It should be:

datachain job cancel --studio (similar to ds)

I think that it may be confusing to users since we don't have non studio version of job in local. The reason I included the cancel inside studio is that the operation is a concept totally located in Studio.

@amritghimire
Copy link
Contributor Author

same with run, same with API - it should be consistent + we should be using PUT, POST, DELETE, etc

https://github.com/iterative/studio/pull/11077#discussion_r1885529578

@shcheklein
Copy link
Member

I think that it may be confusing to users since we don't have non studio version of job in local. The reason I included the cancel inside studio is that the operation is a concept totally located in Studio.

I see the point.

But, I don't think it justifies the inconsistency tbh (datasets, jobs, etc - all should be the same in API and in CLI to my mind). Also the name "datachain studio cancel" is strange a bit anymways. It should be then datachain studio job cancel

or let's find some other options please ...

@shcheklein
Copy link
Member

@amritghimire
Copy link
Contributor Author

I think that it may be confusing to users since we don't have non studio version of job in local. The reason I included the cancel inside studio is that the operation is a concept totally located in Studio.

I see the point.

But, I don't think it justifies the inconsistency tbh (datasets, jobs, etc - all should be the same in API and in CLI to my mind). Also the name "datachain studio cancel" is strange a bit anymways. It should be then datachain studio job cancel

or let's find some other options please ...

WDYT about datachain studio cancel-job?

is it PUT / POST though then?

It is POST currently. We already have POST request for update-job.

@shcheklein
Copy link
Member

WDYT about datachain studio cancel-job?

I think it is still not good, since it's not consistent with datasets. We should be using the same general schema for all types of entities. This is quite important I think.

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