Use `SIGINT` as the default signal in `queue kill` #8657

karajan1001 · 2022-12-05T03:41:17Z

fix: #8624

Add a new flag --force for queue kill
Make SIGINT to be the default option and SIGKILL to be with --force
Remove SIGINT blocking.
Add tests for queue kill

wait for treeverse/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. 🙏

dvc/stage/run.py

codecov · 2022-12-05T03:47:23Z

Codecov Report

Base: 93.50% // Head: 93.50% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (268360c) compared to base (5f0b1ed).
Patch coverage: 100.00% of modified lines in pull request are covered.

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

Impacted Files	Coverage Δ
dvc/commands/queue/kill.py	`100.00% <100.00%> (ø)`
dvc/repo/experiments/queue/celery.py	`85.49% <100.00%> (+0.11%)`	⬆️
dvc/stage/run.py	`94.04% <100.00%> (ø)`
tests/unit/command/test_queue.py	`100.00% <100.00%> (ø)`
tests/unit/repo/experiments/queue/test_celery.py	`96.42% <100.00%> (+0.03%)`	⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

pmrowla · 2022-12-09T04:38:08Z

@karajan1001 there's merge conflicts in this PR, CI can't run until it's fixed

pmrowla

This LGTM, pending @dberenbaum review of the help/usage messages

dvc/commands/queue/kill.py

dberenbaum · 2022-12-15T15:33:17Z

When I run dvc queue kill using https://github.com/iterative/vscode-dvc-demo, it hangs and prints an error in the terminal like:

$ Process _DaemonProcess-1:
Traceback (most recent call last):
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/multiprocessing/process.py", line 314, in _bootstrap
    self.run()
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 232, in run
    super().run()
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 224, in _spawn
    with cls(*args, **kwargs):
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 103, in __exit__
    self.wait()
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 196, in wait
    self._proc.wait(timeout=timeout)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/subprocess.py", line 1207, in wait
    return self._wait(timeout=timeout)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/subprocess.py", line 1941, in _wait
    (pid, sts) = self._try_wait(0)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/subprocess.py", line 1899, in _try_wait
    (pid, sts) = os.waitpid(self.pid, wait_flags)
KeyboardInterrupt

karajan1001 · 2022-12-16T02:51:19Z

When I run dvc queue kill using https://github.com/iterative/vscode-dvc-demo, it hangs and prints an error in the terminal like:

$ Process _DaemonProcess-1:
Traceback (most recent call last):
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/multiprocessing/process.py", line 314, in _bootstrap
    self.run()
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 232, in run
    super().run()
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 224, in _spawn
    with cls(*args, **kwargs):
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 103, in __exit__
    self.wait()
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/site-packages/dvc_task/proc/process.py", line 196, in wait
    self._proc.wait(timeout=timeout)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/subprocess.py", line 1207, in wait
    return self._wait(timeout=timeout)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/subprocess.py", line 1941, in _wait
    (pid, sts) = self._try_wait(0)
  File "/Users/dave/miniforge3/envs/torch/lib/python3.10/subprocess.py", line 1899, in _try_wait
    (pid, sts) = os.waitpid(self.pid, wait_flags)
KeyboardInterrupt

Need to suppress this error message in dvc-task, treeverse/dvc-task#103

dberenbaum · 2022-12-16T15:43:31Z

dvc/stage/run.py

+    old_handler = None

    exec_cmd = _make_cmd(executable, cmd)
-    old_handler = None


@karajan1001 What is this change about?

@karajan1001 If you can explain this change, I'm happy to approve. Thanks!

Actually not anything affected by this. I will undo this.

dberenbaum · 2022-12-16T15:44:12Z

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

karajan1001 · 2022-12-20T03:24:15Z

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

dberenbaum · 2022-12-20T18:39:04Z

Thanks @karajan1001! The help messages look good. See the one question I had in this PR.

pmrowla · 2022-12-27T05:46:33Z

@karajan1001 this has conflicts w/the queue stop --kill PR that was merged

fix: treeverse#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

karajan1001 added enhancement Enhances DVC A: experiments Related to dvc exp A: task-queue labels Dec 5, 2022

karajan1001 requested a review from pmrowla December 5, 2022 03:41

karajan1001 self-assigned this Dec 5, 2022

karajan1001 commented Dec 5, 2022

View reviewed changes

dvc/stage/run.py Show resolved Hide resolved

karajan1001 force-pushed the fix8624 branch 2 times, most recently from bc0ea4e to 19f98f5 Compare December 8, 2022 13:33

karajan1001 force-pushed the fix8624 branch from 19f98f5 to ac6c99d Compare December 10, 2022 03:16

karajan1001 changed the title ~~[WIP] Use SIGINT as the default signal in queue kill~~ Use SIGINT as the default signal in queue kill Dec 10, 2022

karajan1001 force-pushed the fix8624 branch from ac6c99d to 7008331 Compare December 10, 2022 10:33

pmrowla approved these changes Dec 11, 2022

View reviewed changes

dvc/commands/queue/kill.py Outdated Show resolved Hide resolved

dvc/commands/queue/kill.py Outdated Show resolved Hide resolved

karajan1001 force-pushed the fix8624 branch from 7008331 to c087836 Compare December 15, 2022 10:54

karajan1001 requested a review from dberenbaum December 15, 2022 11:09

karajan1001 mentioned this pull request Dec 15, 2022

Add --force and change the default doc to dvc queue kill treeverse/dvc.org#4183

Merged

karajan1001 mentioned this pull request Dec 16, 2022

Suppress the error message when we kill the process. treeverse/dvc-task#103

Merged

dberenbaum reviewed Dec 16, 2022

View reviewed changes

dberenbaum added p2-medium Medium priority, should be done, but less important and removed p2-medium Medium priority, should be done, but less important labels Dec 20, 2022

dberenbaum approved these changes Dec 21, 2022

View reviewed changes

Use SIGINT as the default signal in queue kill

268360c

fix: treeverse#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

karajan1001 force-pushed the fix8624 branch from c087836 to 268360c Compare December 30, 2022 08:01

karajan1001 merged commit 06673dd into treeverse:main Dec 30, 2022

karajan1001 deleted the fix8624 branch December 30, 2022 08:19

Use SIGINT as the default signal in queue kill #8657

Use SIGINT as the default signal in queue kill #8657

Uh oh!

Conversation

karajan1001 commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pmrowla commented Dec 9, 2022

Uh oh!

pmrowla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dberenbaum commented Dec 15, 2022

Uh oh!

karajan1001 commented Dec 16, 2022

Uh oh!

dberenbaum Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

dberenbaum Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

karajan1001 Dec 21, 2022

Choose a reason for hiding this comment

Uh oh!

dberenbaum commented Dec 16, 2022

Uh oh!

karajan1001 commented Dec 20, 2022

Uh oh!

dberenbaum commented Dec 20, 2022

Uh oh!

pmrowla commented Dec 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use `SIGINT` as the default signal in `queue kill` #8657

Use `SIGINT` as the default signal in `queue kill` #8657

karajan1001 commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading