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

Add interruptPrompt() method #803

Merged
merged 6 commits into from
Jun 30, 2023
Merged

Add interruptPrompt() method #803

merged 6 commits into from
Jun 30, 2023

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Jun 29, 2023

Related to #535.

Interrupting a prompt request from the backend requires updating some state in JupyterKernel. To support this, a new interruptPrompt() is added to the LanguageRuntime interface.

This new method is optional. If not implemented, interrupt() is called just as before.

@lionel- lionel- requested a review from jmcphers June 29, 2023 16:49
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

This LGTM, but could we make it simpler by just clearing inputRequests_ whenever an interrupt happens? (seems like no input request can/should survive an interrupt)

@@ -76,6 +76,14 @@ export class RRuntime implements positron.LanguageRuntime, vscode.Disposable {
this._kernel.replyToPrompt(id, reply);
}

async interruptPrompt(id: string): Promise<void> {
if (this._kernel.interruptPrompt) {
await this._kernel.interruptPrompt!(id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, here and elsewhere -- the ! assertion shouldn't be necessary inside an if block that tests to ensure the method exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I was expecting too but the LSP (or was it ESlint?) didn't seem to agree.

@@ -442,6 +442,11 @@ declare module 'positron' {
/** Reply to a prompt issued by the runtime */
replyToPrompt(id: string, reply: string): void;

/** Interrupt a prompt issued by the runtime. If not implemented,
* `interrupt()` is called instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: spacing, the * should be on the same line as the leading * on the previous line for the comment documentation parser to work correctly

@lionel-
Copy link
Contributor Author

lionel- commented Jun 30, 2023

This LGTM, but could we make it simpler by just clearing inputRequests_ whenever an interrupt happens? (seems like no input request can/should survive an interrupt)

hmm... I thought this layer was responsible for multiple kernels but looking more closely that's not the case. Since there can only be one active input request at a time IIUC, do we need that Map? I've removed all the new methods, simplified the Map away, and changed interrupt() to clear the active request. Can you take another look please?

@lionel- lionel- requested a review from jmcphers June 30, 2023 09:36
@lionel- lionel- force-pushed the bugfix/readline-interrupt branch from abf3d16 to aca1af1 Compare June 30, 2023 10:04
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

This feels right, and yes, I think since we have only one input request at a time there isn't any need for a map. Thanks!

@lionel- lionel- force-pushed the bugfix/readline-interrupt branch from aca1af1 to ee08ac0 Compare June 30, 2023 17:31
@lionel- lionel- merged commit 64082e8 into main Jun 30, 2023
@lionel- lionel- deleted the bugfix/readline-interrupt branch June 30, 2023 17:32
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