Skip to content

Commit

Permalink
Add logging to confirm that stdout isn't empty when triggering on cel…
Browse files Browse the repository at this point in the history
…l execution (#333)

* The purpose of this PR is to add logging to confirm that #309 is
fixed; i.e. that when triggering on cell execution for interactive (and
non interactive cells) the output is included in the request

* This PR adds logging that checks if the selected cell has non-empty
stdout if its empty we log a message. The presence of these log messages
thus indicates cell execution triggered completion that included no
stdout.

* This will include false positives because in certain cases the command
may not produce any output.

* With this logging I was unable to reproduce any instances where
completion was triggered by cell execution but stdout wasn't included.

* This PR fixes a bug in the mimetype for stdout; there was an extra
space. I don't think that actually impacted processing because that
constant was only used in unittests before this PR.

* fix #309
  • Loading branch information
jlewi authored Nov 5, 2024
1 parent 6b887ee commit 7515a60
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 1 deletion.
20 changes: 20 additions & 0 deletions app/pkg/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,26 @@ func (a *Agent) StreamGenerate(ctx context.Context, stream *connect.BidiStream[v
doc.Blocks[selectedCell] = block
}

if req.Trigger == v1alpha1.StreamGenerateRequest_CELL_EXECUTE {
stdOutLength := 0
for _, o := range doc.Blocks[selectedCell].Outputs {
for _, item := range o.Items {
if item.Mime == docs.VSCodeNotebookStdOutMimeType {
if item.GetTextData() != "" {
stdOutLength = len(item.GetTextData())
break
}
}
}
}

if stdOutLength == 0 {
// N.B.This is to help debug https://github.com/jlewi/foyle/issues/309
// This will create false positives in the event the command produces no output
log.Info("Empty stdout", logs.ZapProto("request", req), "doc", doc, "selectedCell", selectedCell)
}
}

if req.GetContextId() != state.getContextID() {
return status.Errorf(codes.InvalidArgument, "ContextId doesn't match current value; expected %s; got %s", state.getContextID(), req.GetContextId())
}
Expand Down
2 changes: 1 addition & 1 deletion app/pkg/docs/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ const (
// https://github.com/jlewi/foyle/issues/286
StatefulRunmeOutputItemsMimeType = "stateful.runme/output-items"
StatefulRunmeTerminalMimeType = "stateful.runme/terminal"
VSCodeNotebookStdOutMimeType = "application/vnd.code.notebook.stdout "
VSCodeNotebookStdOutMimeType = "application/vnd.code.notebook.stdout"
)
6 changes: 6 additions & 0 deletions developer_guides/runme.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ Now we can install the extension using the vscode binary
/Applications/Visual\ Studio\ Code.app/Contents/Resources/app/bin/code --force --install-extension ~/git_vscode-runme/runme-extension.vsix
```

```bash {"id":"01JBQ3ZJSS2FXAH4E0CK6SH841"}
cd ~/git_vscode-runme
npm run build
npm run bundle
```

```bash {"id":"01JAH5BVWFNHDGGECF7PRE2EE9","interactive":"false"}
# To check the installed version of the RunMe extension in Visual Studio Code after installation, use the following command:
BINARY="/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code"
Expand Down

0 comments on commit 7515a60

Please sign in to comment.