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

Trigger GhostCells On Cell Execution #1744

Merged
merged 15 commits into from
Oct 25, 2024
Merged

Trigger GhostCells On Cell Execution #1744

merged 15 commits into from
Oct 25, 2024

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Oct 17, 2024

Problem

Right now we trigger completion generation in response to onDidChangeTextDocument

vscode.workspace.onDidChangeTextDocument(eventGenerator.handleOnDidChangeNotebookCell),

That doesn't appear to trigger in the event a user executes a cell or in response to cell output change. I assume this is because the cell outputs aren't textDocuments.

Our handler is filtering by scheme vscode-notebook-cell here https://github.com/stateful/vscode-runme/blob/90fcc5b60322b4fe34be714ac120a5794004f837/src/extension/ai/ghost.ts#L308C40-L308C60
but I added some logging and it doesn't look like that is causing update events to output cells to be dropped.

Solution

We use vscode.workspace.onDidChangeNotebookDocument event. This appears to be invoked for all changes to the document. We can filter that event stream to look for changes to the output items.

One advantage of using this approach is that it looks to fire if the output is changing. For example if you have a long running command,

for i in {0..100}
do
    echo Loop Iter $i
    sleep 1
done

Then we want to retrigger completions when the output changes because have new information for the AI.

Record The Trigger

To enable better monitoring and troubleshooting, I've introduced an ENUM which records the event that triggers the completion. This gets attached to the request and gets logged by Foyle. This should be helpful for debugging why events were generated.

Interactive vs. Non-interactive cells

For non-interactive cells the output is included in the requests as expected. For interactive cells it is not. I suspect that is due to jlewi/foyle#286

Resolve Errors:

Occasionally I see errors like the following

2024-10-18 17:56:36.991 [error] Error: Cannot modify cell output after calling resolve
	at i.t (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:154:236381)
	at Object.clearOutput (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:154:237975)
	at replaceOutput (/Users/jlewi/git_vscode-runme/out/extension.js:1259570:10)
	at /Users/jlewi/git_vscode-runme/out/extension.js:1247816:49
	at async NotebookCellOutputManager.getExecutionUnsafe (/Users/jlewi/git_vscode-runme/out/extension.js:1247835:13)
	at async /Users/jlewi/git_vscode-runme/out/extension.js:1247803:13
	at async Mutex.withLock (/Users/jlewi/git_vscode-runme/out/extension.js:1260952:20)
	at async NotebookCellOutputManager.refreshOutputInternal (/Users/jlewi/git_vscode-runme/out/extension.js:1247802:9)
	at async NotebookCellOutputManager.showTerminal (/Users/jlewi/git_vscode-runme/out/extension.js:1247709:9)
	at async Object.complete (/Users/jlewi/git_vscode-runme/out/extension.js:1250285:45)

I don't know if that's related to this PR. I don't think my changes should be trying to modify output items. The stacktrace indicates its coming from other parts of Runme.

Alternative solutions

I initially tried trigger suggestions from Runme's _doExecute function. _doExecute is already using the AI eventReporter to report execution. So we can just modify EventReporter.reportExecution to trigger suggestion generation by sending a cell event to the streamCreator that manages the stream of suggestions.

The problem I found with that approach is that it seems like the requests to Foyle didn't always include the actual value of stdout. I suspect there might be a race condition. Furthermore, I don't believe that approach would periodically retrigger completions in the event of a long running command where the output is being updated.

Fix jlewi/foyle#309

@jlewi jlewi marked this pull request as ready for review October 18, 2024 01:19
@jlewi
Copy link
Contributor Author

jlewi commented Oct 18, 2024

@sourishkrout I'm noticing that in some cases the output doesn't get included in the Foyle request. Is this an async issue? By which I mean _doExecuteCell has been returned but the updates to the cellOutputItems haven't been applied by the time the AI fetches the document to send it.

The cell gets read here

let notebookData = new vscode.NotebookData(cellData)

If the cell updates are applied asynchronously then I think there could be a race condition.

@jlewi
Copy link
Contributor Author

jlewi commented Oct 24, 2024

@sourishkrout this is ready for a review.

jlewi added a commit to jlewi/vscode-runme that referenced this pull request Oct 24, 2024
branch: jlewi/ghost_markup
stateful#1744
Trigger GhostCells On Cell Execution
Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

✅ LGTM

@sourishkrout
Copy link
Member

sourishkrout commented Oct 25, 2024

@sourishkrout I'm noticing that in some cases the output doesn't get included in the Foyle request. Is this an async issue? By which I mean _doExecuteCell has been returned but the updates to the cellOutputItems haven't been applied by the time the AI fetches the document to send it.

The cell gets read here

let notebookData = new vscode.NotebookData(cellData)

If the cell updates are applied asynchronously then I think there could be a race condition.

Is this still an issue? There is likely a chance of a race. I can't tell exactly when and where from the top of my head, though.

@sourishkrout sourishkrout merged commit ce0e743 into main Oct 25, 2024
1 check passed
@sourishkrout sourishkrout deleted the jlewi/ghost_markup branch October 25, 2024 19:36
hotpocket pushed a commit to hotpocket/vscode-runme that referenced this pull request Nov 5, 2024
* Fix

* Cleanup.

* Update the comment.

* Try to use onDidChangeActiveTextEditors and onDidChangeVisibleTextEditors; that doesn't work. probably because the outputs aren't text editors.

* Start hacking on using the notebook change event.

* Success we appear to be successfully trigger when output updates and output appears to get sent at least if the cell is non-interactive.

* Add comment.

* Comment out changes to try to see why execution in non-interactive cells is no longer working.

* Revert "Comment out changes to try to see why execution in non-interactive cells is no longer working."

This reverts commit 2d650a6.

* Cleanup.

* Revert changes to trigger completions on doExecute. It seems redundant.

* Revert examples

* Revert changes to contributing.md

* Add lock file

---------

Co-authored-by: Sebastian Tiedtke <sebastiantiedtke@gmail.com>
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.

Trigger Foyle Ghost Cell Updates When Cell Output Changes / Cell Is Executed
2 participants