Use the Foyle API to report Log Events #1589

jlewi · 2024-08-28T00:35:46Z

For motivation and description see: Move AI Logging Of Execution Into VSCode Frontend and out of RunMe GRPC Server jlewi/foyle#211
This PR allows us to log all executions along with some context for the execution. This will allow us to improve learning. Right now we don't learn unless a user accepts a suggestion and edits it. This will allow us to learn even if the user didn't use a suggested cell.
This PR introduces a SessionManager for the AI. A session is initiated each time the user switches focus to a different cell. The session corresponds to all the activity related to generating completions as the user edits that cell. A session is associated with a context (basically the notebook) as well as events (cell executions, suggestion acceptances). We log the start and end of the session.
Logging the start and end of the session should simplify log processing on the backend because now we know when a session is closed and there can be no more events associated with the session. Notably, we want to be able to determine which suggestions were rejected rather than accepted. Once a session is closed we know the suggestion was either accepted (there will be an accepted event) or the suggestion was not accepted.

* We weren't actually reading the baseURL from the config because the key should be "aiBaseURL" not "runme.aiBaseURL" * I tried to add logging to ensure we know what the actual value of the endpoint is but that seemed to prevent the extension from loading.

… to requests.

jlewi · 2024-08-30T15:01:51Z

@sourishkrout PTAL when you have a second.

J

sourishkrout

✅ LGTM. The only nit I have is perhaps adding a test to guard against accidental removal/change of the event reporter.

tests/extension/kernel.test.ts should have sufficiently mocked examples to test for it. I'm happy to push a test on top if you want me to write one. Lemme know.

sourishkrout · 2024-08-30T16:25:41Z

@jlewi, will this make the server side AI user logs entirely obsolete? I'm asking because I have a todo to port them from v1 to the v2 runner. Will that be moot now?

sourishkrout

Actually I did find some issues with async/await.

sourishkrout · 2024-08-30T16:46:35Z

src/extension/kernel.ts

@@ -691,6 +692,7 @@ export class Kernel implements Disposable {
    }

    TelemetryReporter.sendTelemetryEvent('cell.startExecute')
+    getEventReporter().reportExecution(cell)


the reporter call is async, we should probably await it here

Would that block execution below? My thinking was that we want LogEvent Reporting to be out of band and not block critical processing. If I was doing this in go I'd fire off a go routine. Is calling an async function and not awaiting it similar to doing go somefunc?

sourishkrout · 2024-08-30T16:47:27Z

src/extension/ai/events.ts

+    event.type = LogEventType.EXECUTE
+    event.cells = cells
+    event.contextId = SessionManager.getManager().getID()
+    this.reportEvents([event])


This should be returned to pass up the promise, expects Promise<void>.

an await should to it too

sourishkrout · 2024-08-30T16:49:04Z

src/extension/ai/events.ts

+        event.eventId = ulid()
+      }
+    }
+    await this.client.logEvents(req).catch((e) => {


return instead of await here otherwise the call stack won't account for asynchronicity

actually if the return value is irrelevant the await will do

I think if we use a return here it will change the return type to Promise
I think Promise is the better return type because I don't think the caller of the reporter should be trying to process any results of the reporter.

jlewi · 2024-08-30T17:03:00Z

@jlewi, will this make the server side AI user logs entirely obsolete?

Yes. I will send you a follow on PR to remove the EnableAILogs option in the frontend.

I was going to ask you if you wanted me to send you a PR to revert/cleanup much of stateful/runme#585

sourishkrout · 2024-08-30T17:11:41Z

Yes. I will send you a follow on PR to remove the EnableAILogs option in the frontend.

I was going to ask you if you wanted me to send you a PR to revert/cleanup much of stateful/runme#585

Yes, on the cleanup PR, please.

Btw, I just pushed #1602 to add testing, which I needed for the review anyway.

jlewi · 2024-08-30T17:15:04Z

Btw, I just pushed #1602 to add testing, which I needed for the review anyway

Thanks!

jlewi · 2024-08-30T17:58:21Z

stateful/runme#661 has the gRPC server changes

jlewi · 2024-08-30T23:20:16Z

src/extension/kernel.ts

@@ -691,6 +692,7 @@ export class Kernel implements Disposable {
    }

    TelemetryReporter.sendTelemetryEvent('cell.startExecute')
+    getEventReporter().reportExecution(cell)


@sourishkrout I'm replying to your comment
#1602 (review)
about adding an await here; on the original PR.

Yes. I incorrectly assumed the noop event reporter would be unless the "AI experiment" is turned on.

That was the intent. Is there a bug here that I need to fix? This is how I intended it to work.

The reporter should be the null op reporter by default
https://github.com/stateful/vscode-runme/pull/1589/files#diff-f0a5c022488c3922874e791475fce22933ea551ffbc4a37c414de2831f8af593R89

and then when the extension loads if the AutoCellExperiment is turned on it get changed to the actual event reproter.
https://github.com/stateful/vscode-runme/pull/1589/files#diff-d1df40b876292a3e89a6d7d6d6e7496db98fa7ad03028b3b0f78bc90487749bdR30

One potential issue I see is that if you disable AIAutoCell the reporter won't be switched back to the null op reporter until the extension is reloaded. I assume that can be triggered by doing a reload window?
That's not great but I didn't know how to subscribe to notifications when options changed and I figured its probably good enough for now.

I can rewrite this reasonably quickly at a later point. Wdyt @jlewi?
If your good with the current implementation then I'm happy.

For users trying out AI and Foyle I think this fixes a big UX issue. In particular, this will remove the need to explicitly add the location of the RunMe logs to the Foyle config. This also means we can turn on learning by default in Foyle (AiAutoCell would still be disabled by default in RunMe). So I see this is as a strict improvement.

The risk is that reporting cell executions causes a degraded experience. I've tried to mitigate that with the NullOp reporter. If users don't enable AI via the AutoCell option then no additional logic (modulo the actuall noop invocation) should be invoked.

Let's add a // todo(sebastian): rewrite to use non-blocking impl on top of the reportExecution call and merge for now. 👍

My bad. I turned off AI logs not aiAutoCell to check the default behavior. In any case, as long as it's not default, we're good to go.

Re, settings requiring a hard reload: that's consistent with all other settings and not something to worry about in this context. Ghost cell completing might be something we want to move into the notebook toolbar and have a setting for the default state—however, one thing at a time.

Here we go: bc2b0ac

@sourishkrout Thanks. I patched it. Your patch has an await in it. Did you mean to include the await?

* Cherry-pick Sebastian's change bc2b0ac

jlewi · 2024-09-02T20:45:27Z

src/extension/kernel.ts

@@ -691,6 +692,8 @@ export class Kernel implements Disposable {
    }

    TelemetryReporter.sendTelemetryEvent('cell.startExecute')
+    // todo(sebastian): rewrite to use non-blocking impl
+    await getEventReporter().reportExecution(cell)


@sourishkrout The await here came from your patch. Did you mean to include an await and make it blocking? If we call await here then I assume we block until the event is full reported? Whereas I thought if we don't use await here then reportExecution would perform the RPC asynchronously and not block actual execution?

it's intentional, @jlewi. the way the node eventloop works is that without await here the call isn't just asynchronous, it can be terminate/dropped prematurely. Since I/O is involved, that's highly likely. If the delivery succeeds it'll likely just be due to a "lucky race" and as a result unreliable. I'm happy to rewrite this but as long as it's behind a experiment/feature flag, it's safe to merge.

Thanks for the explanation!

@sourishkrout Is this good to merge?

Yep, approving the PR now.

* Add to tests * Add todo * Let vs var

sourishkrout

✅ LGTM. Will take care of a non-blocking event reporter at a later time.

sonarcloud · 2024-09-03T22:23:38Z

Quality Gate failed

Failed conditions
37.4% Coverage on New Code (required ≥ 45%)

See analysis details on SonarCloud

As described in jlewi/foyle#211 we will no longer rely on processing RunMe grpc logs to train the AI. This means we can simplify the Runme logging code and revert some of the changes in #585 * Related to stateful/vscode-runme#1589

jlewi added 14 commits August 27, 2024 11:44

Start writing the code to log events.

4d8d400

Update foyle buf package to pull in the events protos.

b70ac9f

Rough version of the code.

11b4bda

Update the connect package for Foyle.

528e083

Latest version; to try to show the problematic line

be7841d

Include context in execution events.

af7a3c7

Fix test.

306b002

Define a sessionmanager to manager the context ID so we can attach it…

10a946d

… to requests.

Include the selected index in the events.

fd2f569

Log accepted suggestions.

326411f

Log session starts and ends.

b04b1ea

Merge in BaseURLFix

5427337

Revert examples.

78376d0

jlewi requested a review from sourishkrout August 28, 2024 15:40

jlewi marked this pull request as ready for review August 28, 2024 15:41

sourishkrout mentioned this pull request Aug 28, 2024

Move AI Logging Of Execution Into VSCode Frontend and out of RunMe GRPC Server jlewi/foyle#211

Closed

jlewi added 4 commits August 28, 2024 13:45

Initialize the logger properly.

de0a3b5

Add ULID

0e6e2aa

Add a ULID to events

e76015b

Merge in upstream/main.

f487c58

sourishkrout approved these changes Aug 30, 2024

View reviewed changes

sourishkrout requested changes Aug 30, 2024

View reviewed changes

Add return

361dc8b

jlewi mentioned this pull request Aug 30, 2024

Remove changes to support logging for AI stateful/runme#661

Merged

jlewi commented Aug 30, 2024

View reviewed changes

Add todo

cf45b35

* Cherry-pick Sebastian's change bc2b0ac

jlewi commented Sep 2, 2024

View reviewed changes

sourishkrout self-requested a review September 3, 2024 22:10

Add tests to AI event logging (#1602)

d136c1b

* Add to tests * Add todo * Let vs var

sourishkrout approved these changes Sep 3, 2024

View reviewed changes

jlewi merged commit dda7d8a into main Sep 4, 2024
2 of 3 checks passed

jlewi deleted the jlewi/logevents branch September 4, 2024 04:54

jlewi mentioned this pull request Oct 11, 2024

Support for non-bash Runme notebooks cells jlewi/foyle#143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the Foyle API to report Log Events #1589

Use the Foyle API to report Log Events #1589

jlewi commented Aug 28, 2024 •

edited

Loading

jlewi commented Aug 30, 2024

sourishkrout left a comment •

edited

Loading

sourishkrout commented Aug 30, 2024

sourishkrout left a comment

sourishkrout Aug 30, 2024

jlewi Aug 30, 2024

sourishkrout Aug 30, 2024

sourishkrout Aug 30, 2024

jlewi Aug 30, 2024

sourishkrout Aug 30, 2024

sourishkrout Aug 30, 2024

jlewi Aug 30, 2024

jlewi commented Aug 30, 2024

sourishkrout commented Aug 30, 2024

jlewi commented Aug 30, 2024

jlewi commented Aug 30, 2024 •

edited

Loading

jlewi Aug 30, 2024

sourishkrout Aug 31, 2024 •

edited

Loading

sourishkrout Aug 31, 2024

jlewi Sep 2, 2024

jlewi Sep 2, 2024

sourishkrout Sep 3, 2024

jlewi Sep 3, 2024

jlewi Sep 3, 2024

sourishkrout Sep 3, 2024

sourishkrout left a comment

sonarcloud bot commented Sep 3, 2024

Use the Foyle API to report Log Events #1589

Use the Foyle API to report Log Events #1589

Conversation

jlewi commented Aug 28, 2024 • edited Loading

jlewi commented Aug 30, 2024

sourishkrout left a comment • edited Loading

Choose a reason for hiding this comment

sourishkrout commented Aug 30, 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

jlewi commented Aug 30, 2024

sourishkrout commented Aug 30, 2024

jlewi commented Aug 30, 2024

jlewi commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

sourishkrout Aug 31, 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

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

sonarcloud bot commented Sep 3, 2024

Quality Gate failed

jlewi commented Aug 28, 2024 •

edited

Loading

sourishkrout left a comment •

edited

Loading

jlewi commented Aug 30, 2024 •

edited

Loading

sourishkrout Aug 31, 2024 •

edited

Loading