-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use SIGINT
as the default signal in queue kill
#8657
Conversation
Codecov ReportBase: 93.50% // Head: 93.50% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #8657 +/- ##
=======================================
Coverage 93.50% 93.50%
=======================================
Files 457 457
Lines 36203 36207 +4
Branches 5244 5246 +2
=======================================
+ Hits 33850 33854 +4
Misses 1845 1845
Partials 508 508
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
bc0ea4e
to
19f98f5
Compare
@karajan1001 there's merge conflicts in this PR, CI can't run until it's fixed |
SIGINT
as the default signal in queue kill
SIGINT
as the default signal in queue kill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, pending @dberenbaum review of the help/usage messages
When I run
|
Need to suppress this error message in |
old_handler = None | ||
|
||
exec_cmd = _make_cmd(executable, cmd) | ||
old_handler = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karajan1001 What is this change about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karajan1001 If you can explain this change, I'm happy to approve. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not anything affected by this. I will undo this.
@karajan1001 Since it's a small change and @pmrowla is away, I'm happy to review this one (or you could ask someone else on the team). LGTM except for one minor question. |
@pmrowla had already approved it, we can focus on documents reviewing. |
Thanks @karajan1001! The help messages look good. See the one question I had in this PR. |
@karajan1001 this has conflicts w/the |
fix: iterative#8624 1. Add a new flag `--force` for `queue kill` 2. Make `SIGINT` to be the default option and `SIGKILL` to be with `--force` 3. Add tests for `queue kill` 4. Bump dvc-task into 0.1.9
fix: #8624
--force
forqueue kill
SIGINT
to be the default option andSIGKILL
to be with--force
SIGINT
blocking.queue kill
wait for iterative/dvc-task#101
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏