Add RuntimeCodeExecutionMode to `runtime.executeCode()` in the API #5450

dhruvisompura · 2024-11-22T00:29:03Z

Description

This partially addresses the ask from #4856.

The ask is to make the positron API executeCode call respect the RuntimeCodeExecutionMode and RuntimeErrorBehavior values being passes in. This PR only handles RuntimeCodeExecutionMode. The work for RuntimeErrorBehavior will be done in a separate PR.

The runtime session execute is already setup to work with RuntimeCodeExecutionMode, see workbench.action.executeCode.silently (#2684) for an example of silent code execution via the session directly.

For code execution to work as expected, some changes were made in the PositronConsoleService to prevent data from being rendered in the console or added to history based off the RuntimeCodeExecutionMode.

QA Notes

We will want to verify that the three different values for RuntimeCodeExecutionMode are respected when positron.runtime.executeCode() is called and verify there aren't any regressions:

RuntimeCodeExecutionMode.Interactive means the code was entered interactively, and should be executed and stored in the runtime's history
- We should verify that executing code via the console as a user would runs the code interactively.
- We should verify that positron.runtime.executeCode() without a RuntimeCodeExecutionMode defaults to RuntimeCodeExecutionMode.Interactive
- Verify the command workbench.action.executeCode.console runs the code interactively.
RuntimeCodeExecutionMode.Transient means the code should be executed (shown in console) but not stored in history
RuntimeCodeExecutionMode.Silent means the code should neither displayed to the user nor stored in history
- Verify the command workbench.action.executeCode.silently runs the code interactively.

The PositronZedLanguageRuntime has a new command exec silent that can execute a code snippet in the language silently. The exec command now explicitly passes a RuntimeCodeExecutionMode of Interactive to make it clear what is happening.

Screenshot

Silent Code Execution - Print

Screen.Recording.2024-11-21.at.4.55.57.PM.mov

Silent Code Execution - Variable Assignment

Screen.Recording.2024-11-21.at.4.55.24.PM.mov

Interactive Code Execution - Print

Screen.Recording.2024-11-21.at.4.54.40.PM.mov

Interactive Code Execution - Variable Assignment

Screen.Recording.2024-11-21.at.4.53.15.PM.mov

dhruvisompura · 2024-11-22T01:06:19Z

extensions/positron-zed/src/positronZedLanguageRuntime.ts

 		} else if (match = code.match(/^exec ([a-zA-Z]+) (.+)/)) {
 			// Execute code in another language.
 			const languageId = match[1];
 			const codeToExecute = match[2];
-			this.simulateCodeExecution(id, code, languageId, codeToExecute);
+			this.simulateCodeExecution(id, code, languageId, codeToExecute, positron.RuntimeCodeExecutionMode.Interactive);


The default value that code is executed as is RuntimeCodeExecutionMode.Interactive if a RuntimeCodeExecutionMode isn't provided so this is change is a 50/50 on if we want to explicitly provide the value here or not.

dhruvisompura · 2024-11-22T01:09:07Z

extensions/positron-zed/src/positronZedLanguageRuntime.ts

 		// Enter busy state and output the code.
 		this.simulateBusyState(parentId);
 		this.simulateInputMessage(parentId, code);

 		// Let the user know what we're about to do
 		this.simulateOutputMessage(parentId, `Executing ${languageId} snippet: ${codeToExecute}`);

+		// Don't focus the console if code should being executed silently
+		const focus = mode !== positron.RuntimeCodeExecutionMode.Silent;


I figured it makes sense to update the focus value based off the RuntimeCodeExecutionMode. Since this is all used for dev testing it doesn't really matter but this felt more sane than always having focus set to true.

src/vs/workbench/contrib/positronConsole/browser/positronConsoleActions.ts

dhruvisompura · 2024-11-22T01:44:19Z

src/vs/workbench/api/common/positron/extHostLanguageRuntime.ts

-	public executeCode(languageId: string, code: string, focus: boolean, allowIncomplete?: boolean): Promise<boolean> {
-		return this._proxy.$executeCode(languageId, code, focus, allowIncomplete);
+	public executeCode(languageId: string, code: string, focus: boolean, allowIncomplete?: boolean, runtimeCodeExecutionMode?: RuntimeCodeExecutionMode): Promise<boolean> {
+		return this._proxy.$executeCode(languageId, code, focus, allowIncomplete, runtimeCodeExecutionMode);


This call is going out to the mainThreadLanguageRuntime which doesn't support RuntimeCodeExecutionMode and has been added, the meat of the changes are in the positronConsoleService

dhruvisompura · 2024-11-22T02:16:12Z

src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts

 	 */
-	private doExecuteCode(code: string) {
+	private doExecuteCode(code: string, runtimeCodeExecutionMode?: RuntimeCodeExecutionMode) {


This is where the meat of the changes are that need eyes.

It seems like the mainThreadLanguageRuntime is allowed to call the session directly but extensions aren't and have to go through the mainThreadLanguageRuntime which ends up going through this positronConsoleService which is why we need to handle the different RuntimeCodeExecutionMode consequences here.

My understanding is that the _runtimeItemActivities list contains the information rendered by the console UI, aka the code that will be executed and its output. The list is modified in the service here and we need to conditionally decide when to add things to the list based off the RuntimeCodeExecutionMode value.

The console history list is created on the component side and relies on the executionHistoryService to create the history initially and the onDidExecuteCode event to update the list. The only listeners I found for this event was in the console so it should be fine if we don't fire it when we don't want to add input to the history.

I haven't fully dug into how the executionHistoryService creates the initial history list now that I am reflecting back on these changes so I don't know if I've handled all the scenarios for the console input history case. My testing didn't reveal anything but I'll take another look into this to make sure I didn't miss anything

lionel-

Looks good!

src/positron-dts/positron.d.ts

src/vs/workbench/contrib/positronConsole/browser/positronConsoleActions.ts

lionel- · 2024-11-22T09:16:03Z

src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts

+		// Code that is not executed interactively should not show up in the console history
+		if (codeExecutionMode === RuntimeCodeExecutionMode.Interactive) {
+			// Fire the onDidExecuteCode event.
+			this._onDidExecuteCodeEmitter.fire(code);
+		}


I wonder if we should instead keep firing the event but add mode to the emitted data so that handlers can decide to decline handling (e.g. decline adding to the console history).

I worry that we're creating a footgun for future handlers of this event as it might not be clear it will only be fired for interactive executions.

I think this is worth at least taking a quick look at, to see if it is doable.

I had a similar thought and was wondering if that is the route to go but didn't initially add it because it seems like this service is mainly used to drive information the user sees (I could totally be wrong about this though 😬 ). I agree with this feeling like a footgun so I'll update this so it includes the mode!

lionel- · 2024-11-22T09:17:50Z

src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts

+			// Add the provisional ActivityItemInput. This provisional ActivityItemInput will be
+			// replaced with the real ActivityItemInput when the runtime sends it (which can take a
+			// moment or two to happen).
+			this.addOrUpdateUpdateRuntimeItemActivity(id, activityItemInput);


IIUC, if we don't add this provisional item here, it won't be added back in when the runtime responds? Or does the runtime not send one because execution is silent?

why is this updatingupdating twice 😆

oh I think I see what's going on: the Jupyter protocol advises kernels to rebroadcast an execution input: https://jupyter-client.readthedocs.io/en/stable/messaging.html#code-inputs. If the execute_request is silent, we don't rebroadcast (see https://github.com/posit-dev/ark/blob/cab11b9d0af80ae27d589985ab69bde679d06d5f/crates/ark/src/interface.rs#L610-L614 in Ark). So the input item will not be added back in.

I think it might be worth adding notes about these behaviour dependencies here?

I can add a note about this! One thing that I don't understand is the code path for what happens to the rebroadcast execution input. It sounds like what you are saying is that the frontend is provided the execution input again from the kernel and somewhere in the code the runtimeItems list is updated to have that information if it doesn't?

Here is my understanding of things:

We rebroadcast the input from the client in Ark here: https://github.com/posit-dev/ark/blob/586df407ef05dd99e83770825f282fe4b47ecbab/crates/ark/src/interface.rs#L608-L621

The kernel supervisor forwards that message as positron.LanguageRuntimeInput here:

positron/extensions/positron-supervisor/src/RuntimeMessageEmitter.ts

Line 136 in 9084df0

onExecuteInput(message: JupyterMessage, data: JupyterExecuteInput) {

On the main process this type is known as ILanguageRuntimeMessageInput:

positron/src/vs/workbench/services/languageRuntime/common/languageRuntimeService.ts

Line 195 in 9084df0

export interface ILanguageRuntimeMessageInput extends ILanguageRuntimeMessage {

The console service handles this message to add a runtime activity item here:

positron/src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts

Lines 1436 to 1459 in 9084df0

this._runtimeDisposableStore.add(this._session.onDidReceiveRuntimeMessageInput(languageRuntimeMessageInput => {

// If trace is enabled, add a trace runtime item.

if (this._trace) {

this.addRuntimeItemTrace(

formatCallbackTrace('onDidReceiveRuntimeMessageInput', languageRuntimeMessageInput) +

'\nCode:\n' +

languageRuntimeMessageInput.code

);

}

// Add or update the runtime item activity.

this.addOrUpdateUpdateRuntimeItemActivity(

languageRuntimeMessageInput.parent_id,

new ActivityItemInput(

ActivityItemInputState.Executing,

languageRuntimeMessageInput.id,

languageRuntimeMessageInput.parent_id,

new Date(languageRuntimeMessageInput.when),

this._session.dynState.inputPrompt,

this._session.dynState.continuationPrompt,

languageRuntimeMessageInput.code

)

);

}));

extensions/positron-zed/src/positronZedLanguageRuntime.ts

juliasilge

This is looking great, pending the couple of minor changes!

I want to highlight for QA verification that we really can only verify this in a dev build currently, via these two dev-build-only methods:

The Zed extension exec and exec silent
The "JavaScript: Connect Extension Host Runtime" command that lets you run Positron API commands

As a reminder (since I myself had forgotten), for that JavaScript console, you would do something like:

var p = acquirePositronApi();
p.runtime.executeCode("r", "1 + 1", p.RuntimeCodeExecutionMode.Silent)

You'll see some unrelated errors if you do this, related to how this fake-ish runtime is not hooked up to the Variables pane and similar.

If we wanted to write automated tests for the Positron API, that would be something fairly new. The closest we have right now is here:
https://github.com/posit-dev/positron/tree/main/extensions/vscode-api-tests/src/singlefolder-tests/positron

juliasilge · 2024-11-22T19:49:52Z

src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts

+		// Code that is not executed interactively should not show up in the console history
+		if (codeExecutionMode === RuntimeCodeExecutionMode.Interactive) {
+			// Fire the onDidExecuteCode event.
+			this._onDidExecuteCodeEmitter.fire(code);
+		}


I think this is worth at least taking a quick look at, to see if it is doable.

src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts

dhruvisompura · 2024-11-23T01:21:14Z

FYI, looks like there's a gap in this solution when it comes to handling execution of pending code. The execution explicitly uses Interactive as a mode which is no longer correct.

I'm currently working through the changes needed to handle that scenario but I may just do the work in a follow up PR if that's alright with folks. I can hold off on merging this PR until the pending code execution scenarios are handled so we don't introduce changes that aren't fully functional.

dhruvisompura added 4 commits November 19, 2024 14:25

Add RuntimeCodeExecutionMode to runtime.executeCode() in the API

6473c64

Do not render code that should be silently executed in console

30b91b6

Add code ran interactively to console history

61ec05e

Add support for silent code execution in test extension

51078e6

dhruvisompura commented Nov 22, 2024

View reviewed changes

src/vs/workbench/contrib/positronConsole/browser/positronConsoleActions.ts Outdated Show resolved Hide resolved

dhruvisompura commented Nov 22, 2024

View reviewed changes

dhruvisompura marked this pull request as ready for review November 22, 2024 02:16

dhruvisompura requested review from softwarenerd, lionel- and juliasilge November 22, 2024 02:19

lionel- mentioned this pull request Nov 22, 2024

Should execution mode be handled by the main thread rather than runtimes? #5452

Open

lionel- previously approved these changes Nov 22, 2024

View reviewed changes

juliasilge previously approved these changes Nov 22, 2024

View reviewed changes

dhruvisompura added 2 commits November 22, 2024 15:10

Rename variable to be less verbose

9aff7aa

Pass code execution mode to event handlers

b2e9c72

dhruvisompura dismissed stale reviews from juliasilge and lionel- via b2e9c72 November 23, 2024 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RuntimeCodeExecutionMode to `runtime.executeCode()` in the API #5450

Add RuntimeCodeExecutionMode to `runtime.executeCode()` in the API #5450

dhruvisompura commented Nov 22, 2024 •

edited

Loading

dhruvisompura Nov 22, 2024

dhruvisompura Nov 22, 2024

dhruvisompura Nov 22, 2024

dhruvisompura Nov 22, 2024

lionel- left a comment

lionel- Nov 22, 2024

juliasilge Nov 22, 2024

dhruvisompura Nov 22, 2024 •

edited

Loading

lionel- Nov 22, 2024

lionel- Nov 22, 2024

lionel- Nov 22, 2024

dhruvisompura Nov 23, 2024

lionel- Nov 25, 2024

juliasilge left a comment

juliasilge Nov 22, 2024

dhruvisompura commented Nov 23, 2024

	this._runtimeDisposableStore.add(this._session.onDidReceiveRuntimeMessageInput(languageRuntimeMessageInput => {
	// If trace is enabled, add a trace runtime item.
	if (this._trace) {
	this.addRuntimeItemTrace(
	formatCallbackTrace('onDidReceiveRuntimeMessageInput', languageRuntimeMessageInput) +
	'\nCode:\n' +
	languageRuntimeMessageInput.code
	);
	}

	// Add or update the runtime item activity.
	this.addOrUpdateUpdateRuntimeItemActivity(
	languageRuntimeMessageInput.parent_id,
	new ActivityItemInput(
	ActivityItemInputState.Executing,
	languageRuntimeMessageInput.id,
	languageRuntimeMessageInput.parent_id,
	new Date(languageRuntimeMessageInput.when),
	this._session.dynState.inputPrompt,
	this._session.dynState.continuationPrompt,
	languageRuntimeMessageInput.code
	)
	);
	}));

Add RuntimeCodeExecutionMode to runtime.executeCode() in the API #5450

Are you sure you want to change the base?

Add RuntimeCodeExecutionMode to runtime.executeCode() in the API #5450

Conversation

dhruvisompura commented Nov 22, 2024 • edited Loading

Description

QA Notes

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionel- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvisompura Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasilge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvisompura commented Nov 23, 2024

Add RuntimeCodeExecutionMode to `runtime.executeCode()` in the API #5450

Add RuntimeCodeExecutionMode to `runtime.executeCode()` in the API #5450

dhruvisompura commented Nov 22, 2024 •

edited

Loading

dhruvisompura Nov 22, 2024 •

edited

Loading