Generate cells using Foyle AI #1356

jlewi · 2024-05-16T23:03:28Z

Create a command which sends an RPC to Foyle to generate cells
Resulting cells are inserted into the notebook
A configuration option lets user set the address of the Foyle server

…e completions. Right now this is just a stub which creates a pop up box.

…o update the version of the protos installed to import the latest versions that include the ai definition.

jlewi · 2024-05-16T23:05:45Z

src/extension/ai/converters.ts

+// cellToCellData converts a NotebookCell to a NotebookCellData.
+// NotebookCell is an interface used by the editor.
+// NotebookCellData is a concrete class.
+export function cellToCellData(cell: vscode.NotebookCell): vscode.NotebookCellData {


@sourishkrout Do I need these conversion routines to convert between the protos and vscode's native data structures? Are there existing routines I can reuse?

this static method should do the job: https://github.com/stateful/vscode-runme/blob/main/src/extension/serializer.ts#L302-L333

Thanks. I updated the code to reuse the existing routines.
We cell need the function cellToCellData though because that converts from the vscode API obtained from the editor into the API used by the serialization routines.

I also had to make one of the serializer methods public.

@jlewi is there maybe a file missing in the latest commits?

in src/extension/ai/generate.ts

@sourishkrout Can you help me figure out what's going on here and how to fix it. I think it should have been

import {Serializer} from '../../types'

I'm trying to import Notebook defined here.

I'm trying to instantiate an instance of Notebook to pass to Notebook.Revive here
https://github.com/stateful/vscode-runme/pull/1356/files#diff-7561d1e499859730c4937494e85d7dddfbe11332a496476afc5d082860df7335R81

What's the proper way

Instantiate a notebook

Populate the cells in the notebook with the proto

I'm a little confused by what's going on. I see the error in vscode but npm run bundle succeeds and I thought I had installed it successfully from source.

So I think I partially understand the error. Serializer.Notebook is a type not a class so I can't use new. I wrote some conversion code to convert the proto type to the Serializer.CellType

PTAL

Correct. The Serializer.Notebook is a type that in structural typing sense fits different Parsers. Runme has a legacy WASM-based parser from its prototype days. It made it possible to transparently switching between them.

Using your latest commits fixed the error for me. The additional conversion code is necessary to satisfy the structure required by the type. If we removed the legacy WASM-parser we could probably just absorb this code into the lower layer.

I have seen the Typescript LSP and/or the ESlint LSP getting "confused" which happens when files are being moved around right under it. The easy fix is to restart them using the command palette. In any case, it compiles cleanly now on all fronts webpack/tsc, and eslint & typescript LSPs. Likely a side-effects of linter, bundler/linker, etc not being integral to node.

jlewi · 2024-05-16T23:06:46Z

src/extension/ai/converters.ts

+    cellPb.kind = CellKind.MARKUP
+  }
+
+  // TODO(jeremy):  cell.metadata is type map[string]Any; cellPb.metadata is type map[string]string


@sourishkrout Is there existing conversion code I can use here? If not is there a way to do this other than just testing if the values are strings and if not ignoring them?

I believe this static method is what you're looking for:

https://github.com/stateful/vscode-runme/blob/main/src/extension/serializer.ts#L740-L757

Thanks.
Done.

tests/extension/ai/converters.test.ts

jlewi · 2024-05-16T23:12:11Z

@sourishkrout Would you PTAL? The code is a bit rough (lots of lint errors); however I had some questions about how various things should be done and I thought you might be able to provide some pointers?

FWIW the code does work; I was able to run it and get suggestions provided by Foyle

sourishkrout

Overall good stuff. Filled in some gaps for you. Hopefully the requested changes are reasonable to get this merged.

I think it's probably wise to forgo unit tests with mocking for the app/UI logic and e2e integration tests until the integration moves out of alpha.

sourishkrout · 2024-05-17T17:31:11Z

package.json

+        "command": "runme.aiGenerate",
+        "key": "win+;",
+        "mac": "cmd+;",
+        "when": "resourceExtname==.md || resourceExtname==.markdown"


notebookType == runme might be a better choice here instead of keying on file extension

sourishkrout · 2024-05-17T17:32:40Z

src/extension/ai/client.ts

+
+// TODO(jeremy): Should we take transport as an argument?
+export function initAIServiceClient(): AIServiceClient {
+  const config = vscode.workspace.getConfiguration(extName)


nit, in runme-land we have a few zod-schemata-based abstractions to handle config. not super important right now.

Can you point me at a code snippet for the right pattern?

it's all in src/utils/configuration.ts but we ok changing it post-merge

sourishkrout · 2024-05-17T17:36:30Z

src/extension/ai/converters.ts

+    cellPb.kind = CellKind.MARKUP
+  }
+
+  // TODO(jeremy):  cell.metadata is type map[string]Any; cellPb.metadata is type map[string]string


I believe this static method is what you're looking for:

https://github.com/stateful/vscode-runme/blob/main/src/extension/serializer.ts#L740-L757

sourishkrout · 2024-05-17T17:38:39Z

src/extension/ai/converters.ts

+// cellToCellData converts a NotebookCell to a NotebookCellData.
+// NotebookCell is an interface used by the editor.
+// NotebookCellData is a concrete class.
+export function cellToCellData(cell: vscode.NotebookCell): vscode.NotebookCellData {


this static method should do the job: https://github.com/stateful/vscode-runme/blob/main/src/extension/serializer.ts#L302-L333

src/extension/ai/generate.ts

tests/extension/ai/converters.test.ts

sourishkrout · 2024-05-17T17:45:48Z

src/extension/ai/client.ts

+export const extName = 'runme'
+
+// TODO(jeremy): Should we take transport as an argument?
+export function initAIServiceClient(): AIServiceClient {


we've been keeping the gRPC related abstractions under https://github.com/stateful/vscode-runme/tree/main/src/extension/grpc. Including re-exporting the generated protos, e.g. https://github.com/stateful/vscode-runme/blob/main/src/extension/grpc/projectTypes.ts.

This allows us to avoid spreading these long imports '@buf/stateful_runme.community_timostamm-protobuf-ts/runme/ai/v1alpha1/ai_pb.client' across the code base and helps with API version migration down the road.

Can always do this later.

package.json

sourishkrout · 2024-05-17T17:49:54Z

src/extension/kernel.ts

-      this.registerTerminalProfile(),
-    )
+
+    // DO NOT COMMIT this was a hack because I was getting crashes when trying to run the extension


let's figure out what's going on - sooner or later the lack of running these handlers will rear it's ugly head

Seems to be working for me now. I have no idea what changed. I uninstalled the extension and reinstalled it as part of trying to install a dev version of the extension in my vscode; so maybe it was that 🤷

…code NotebookDocument to proto.

…t of the code in converters.ts

jlewi

Thanks @sourishkrout . I believe I've addressed all your comments with the exception of the zota config issue and the experiments flag; see my replies to those comments.

package.json

jlewi · 2024-05-17T22:50:08Z

package.json

+        "command": "runme.aiGenerate",
+        "key": "win+;",
+        "mac": "cmd+;",
+        "when": "resourceExtname==.md || resourceExtname==.markdown"


jlewi · 2024-05-17T22:50:36Z

src/extension/ai/client.ts

+
+// TODO(jeremy): Should we take transport as an argument?
+export function initAIServiceClient(): AIServiceClient {
+  const config = vscode.workspace.getConfiguration(extName)


Can you point me at a code snippet for the right pattern?

jlewi · 2024-05-17T22:51:38Z

src/extension/ai/converters.ts

+// cellToCellData converts a NotebookCell to a NotebookCellData.
+// NotebookCell is an interface used by the editor.
+// NotebookCellData is a concrete class.
+export function cellToCellData(cell: vscode.NotebookCell): vscode.NotebookCellData {


Thanks. I updated the code to reuse the existing routines.
We cell need the function cellToCellData though because that converts from the vscode API obtained from the editor into the API used by the serialization routines.

I also had to make one of the serializer methods public.

jlewi · 2024-05-17T22:51:48Z

src/extension/ai/converters.ts

+    cellPb.kind = CellKind.MARKUP
+  }
+
+  // TODO(jeremy):  cell.metadata is type map[string]Any; cellPb.metadata is type map[string]string


Thanks.
Done.

src/extension/ai/generate.ts

package.json

jlewi · 2024-05-18T00:08:39Z

src/extension/kernel.ts

-      this.registerTerminalProfile(),
-    )
+
+    // DO NOT COMMIT this was a hack because I was getting crashes when trying to run the extension


Seems to be working for me now. I have no idea what changed. I uninstalled the extension and reinstalled it as part of trying to install a dev version of the extension in my vscode; so maybe it was that 🤷

jlewi · 2024-05-18T00:10:00Z

src/extension/ai/client.ts

+export const extName = 'runme'
+
+// TODO(jeremy): Should we take transport as an argument?
+export function initAIServiceClient(): AIServiceClient {


sourishkrout

Won't compile/build locally right now... likely due to a missing file? See comments.

Otherwise 👍 .

jlewi · 2024-05-22T23:06:25Z

@sourishkrout Sorry about that. I think its fixed.

sourishkrout · 2024-05-23T14:34:33Z

src/extension/ai/generate.ts

+      value: cell.value,
+      metadata: cell.metadata,
+      kind: kind,
+      // TODO(jeremy): Should we include outputs? The generate response should never contain outputs so we shouldn't


fyi, outputs are entirely optional in Runme. We only need them when recording of Session files is enabled which writes to a "carbon copy" of the original markdown notebook; never back to the original source file.

sourishkrout

👍 LGTM

sourishkrout · 2024-05-23T14:37:09Z

One of the unit tests failed which is an easy fix. registerCommand is called an additional time for the Foyle/AI cmd.

@jlewi the fix is to increment it to 40.

 FAIL  tests/extension/extension.test.ts > initializes all providers
AssertionError: expected "spy" to be called 39 times, but got 40 times
 ❯ tests/extension/extension.test.ts:102:36
    100|   expect(notebooks.registerNotebookCellStatusBarItemProvider).toBeCall…
    101|   expect(workspace.registerNotebookSerializer).toBeCalledTimes(1)
    102|   expect(commands.registerCommand).toBeCalledTimes(39)
       |                                    ^
    103|   expect(window.registerTreeDataProvider).toBeCalledTimes(1)
    104|   expect(window.registerUriHandler).toBeCalledTimes(1)

Error: AssertionError: expected "spy" to be called 39 times, but got 40 times
 ❯ tests/extension/extension.test.ts:102:36

jlewi · 2024-05-23T14:43:23Z

Thank you @sourishkrout ; I fixed the test.

sourishkrout · 2024-05-23T15:24:08Z

Merging. - The e2e integration test's failing because GitHub Actions are being finicky and have been under the weather with outages the last few days. It's not broken.

jlewi added 9 commits May 15, 2024 13:31

Add a note about how to start debugging it.

c7a0cdf

Try adding a command.

d742f17

Comment out kernel code.

372aa81

With kernel.ts partially commented out I can add a command to generat…

03538b7

…e completions. Right now this is just a stub which creates a pop up box.

run npm install @buf/stateful_runme.community_timostamm-protobuf-ts t…

bc6a243

…o update the version of the protos installed to import the latest versions that include the ai definition.

Create a manual test for running the gRPC client.

3d4a88c

Client can generate cells using the agent and insert them.

b3968b3

Add an option to set the foyle address.

dd44061

Delete jeremy.md

374a4af

jlewi commented May 16, 2024

View reviewed changes

jlewi added 2 commits May 16, 2024 16:07

Cleanup duplicate code.

1f4a033

Delete manual gRPC test.

f789f70

jlewi commented May 16, 2024

View reviewed changes

tests/extension/ai/converters.test.ts Outdated Show resolved Hide resolved

jlewi marked this pull request as ready for review May 16, 2024 23:10

sourishkrout requested changes May 17, 2024

View reviewed changes

jlewi added 6 commits May 17, 2024 13:51

Update the code to reuse the Serializer.marshalNotebook to go from vs…

41109c5

…code NotebookDocument to proto.

Now that we are reusing the serializer routines we can get rid of mos…

4b52c5d

…t of the code in converters.ts

Use a real logger.

969e838

Lot of cleanup.

d7d11ef

Cleanup; uncomment kernel.ts code.

ebc3096

Revert kernel.ts

7a01ace

jlewi commented May 18, 2024

View reviewed changes

sourishkrout self-requested a review May 21, 2024 21:41

sourishkrout requested changes May 21, 2024

View reviewed changes

Fixed the compile error.

55940e1

sourishkrout reviewed May 23, 2024

View reviewed changes

sourishkrout approved these changes May 23, 2024

View reviewed changes

Fix failing unittest by updating amount of time its called

b42a4d0

sourishkrout merged commit 066838d into stateful:main May 23, 2024
2 of 3 checks passed

jlewi mentioned this pull request May 24, 2024

Support using Foyle's AI capabilities as a plugin stateful/runme#574

Closed

3 tasks

Generate cells using Foyle AI #1356

Generate cells using Foyle AI #1356

Conversation

jlewi commented May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourishkrout May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourishkrout May 23, 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

jlewi commented May 16, 2024

sourishkrout left a 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

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

Choose a reason for hiding this comment

jlewi left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourishkrout left a comment

Choose a reason for hiding this comment

jlewi commented May 22, 2024

Choose a reason for hiding this comment

sourishkrout left a comment

Choose a reason for hiding this comment

sourishkrout commented May 23, 2024 • edited Loading

jlewi commented May 23, 2024

sourishkrout commented May 23, 2024

jlewi commented May 16, 2024 •

edited

Loading

sourishkrout May 21, 2024 •

edited

Loading

sourishkrout May 23, 2024 •

edited

Loading

sourishkrout commented May 23, 2024 •

edited

Loading