Skip to content

[batch/qob] Ctrl-c when interactively running a batch cancels the batch #12801

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

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

daniel-goldstein
Copy link
Contributor

CHANGELOG: Hitting CTRL-C while interactively using Batch or Query-on-Batch cancels the underlying batch.

@@ -794,7 +800,7 @@ async def compile_job(job):
if verbose:
print(f'Waiting for batch {batch_handle.id}...')
starting_job_id = min(j._client_job.job_id for j in unsubmitted_jobs)
status = batch_handle.wait(disable_progress_bar=disable_progress_bar, starting_job=starting_job_id)
status = await batch_handle._async_batch.wait(disable_progress_bar=disable_progress_bar, starting_job=starting_job_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was made because it is both silly to nest two run_until_complete and doing so also causes the KeyboardInterrupt stacktrace to be printed twice. This fixes so the stacktrace is only printed once and you don't get complaints about a task not handling the exception.

if batch._batch_handle is not None:
print("Received a keyboard interrupt, cancelling the batch...")
batch._batch_handle.cancel()
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to make this change a long time ago and got a bunch of push back.

cc: @konradjk ; If you ctrl-c a hailtop.batch script that is waiting on a batch (e.g. b.run(wait=True)), should the batch be cancelled?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, let's just make the QoB change and revisit this after some discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I remember the argument but feel like these two behaviors are consistent with each other. Happy to remove the batch behavior for now though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I completely agree with you but, as the thread suggests, there was divergent opinions. I think people think of Batch as a submission sort of thing. IMHO, if you're trying to submit you should wait=False, but 🤷 . Let's take the win and fight this battle separately.

@@ -443,8 +443,17 @@ async def _rpc(self,
raise reconstructed_error
raise reconstructed_error.maybe_user_error(ir)

def _cancel_on_ctrl_c(self, coro):
try:
async_to_blocking(coro)
Copy link
Contributor

Choose a reason for hiding this comment

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

return async...

Copy link
Contributor

Choose a reason for hiding this comment

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

also types!

@danking danking merged commit a9b70ae into hail-is:main Mar 22, 2023
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.

2 participants