Cody completion: Mega bug bash thread #52365

philipp-spiess · 2023-05-24T13:26:21Z

This PR includes a bunch of fixes regarding #51344

I've not done a good job at splitting this up into commits and I will also need to add some tests for this (I have some ideas for how to make an effective test setup for this already) but for now, I did some extensive explanation of the changes and why they are added inline 🙂

Test plan

See test plan added to the inline discussions.

philipp-spiess · 2023-05-24T13:34:24Z

client/cody/src/completions/index.ts

            if (error.message === 'aborted') {
                return []
            }
-
+            console.error(error)
+            debug('CodyCompletionProvider:inline:error', `${error}\n${error.stack()}`)


Bugfix: Makes sure that inline completion errors are added to the new debug log. This is going to be helpful if we have to debug customer issues.

philipp-spiess · 2023-05-24T13:36:19Z

client/cody/src/completions/index.ts

@@ -28,6 +28,7 @@ export class CodyCompletionItemProvider implements vscode.InlineCompletionItemPr
    private maxSuffixTokens: number
    private abortOpenInlineCompletions: () => void = () => {}
    private abortOpenMultilineCompletion: () => void = () => {}
+    private lastContentChanges: Map<string, 'add' | 'del'> = new Map()


Question: This map can grow with the number of files changes in one session. Do you think we should rather use a LRU cache here instead right away or is it fine if we get back to that later?

since I barely ever restart vscode, it could be that this grows unboundedly 🤔 Maybe if it's not a ton of effort rather do it now vs bad surprises later?

philipp-spiess · 2023-05-24T13:36:36Z

client/cody/src/completions/index.ts

+        // Avoid showing completions when we're deleting code (Cody can only insert code at the
+        // moment)
+        const lastChange = this.lastContentChanges.get(document.fileName) ?? 'add'
+        if (lastChange === 'del') {
+            // When a line was deleted, only look up cached items and only include them if the
+            // untruncated prefix matches. This fixes some weird issues where the completion would
+            // render if you insert whitespace but not on the original place when you delete it
+            // again
+            const cachedCompletions = inlineCompletionsCache.get(prefix, false)
+            if (cachedCompletions) {
+                return cachedCompletions.map(toInlineCompletionItem)
+            }
+            return []
+        }


New heuristics: Avoid showing completions when we're deleting code

There is only one exception and that is when we have a cached completion at exactly the same prompt. This necessary since you could be inserting spaces and then pressing backspace to get back to were you were before (exactly when you triggered the completion) and we currently show the same completion while you insert more backspace but then would hide it when you go back.

Here's a video of the new behavior:

Screen.Recording.2023-05-24.at.14.12.59.mov

Notice there is a bug right now because we add a cache entry for the completion with a trimmed prefix and that is now also matching in this logic. You can see this at the end of the video where removing all indentation from the line reveals the suggestion again. We need some extra bookkeeping to fix this so I decided it's not worth doing it just yet. WDYT?

Todo for myself: Add a todo about that in code

philipp-spiess · 2023-05-24T13:41:46Z

client/cody/src/completions/index.ts

-        await new Promise<void>(resolve =>
-            setTimeout(() => resolve(), Math.max(0, waitMs - estimatedLLMResponseLatencyMS))
-        )


Bug fix: The previous heuristics would use the waitMs and subtract the estimated LLM response. I looked at prod data for dotcom and the estimate was overshooting it but also I noticed that the waitMs was set to 500 before and the latency to 700 so we would not debounce in some situations 😬

I've tweaked the timeouts again. It will unfortunlaty be a bit slower but in most cases it's not noticeable. Start of the line completions are now properly debounced by 200ms though!

philipp-spiess · 2023-05-24T13:42:35Z

client/cody/src/completions/index.ts

+        if (hasVisibleCompletions(results)) {
+            const params = { ...LOG_INLINE, latency: Date.now() - start, timeout }
+            logEvent('CodyVSCodeExtension:completion:suggested', params, params)
+            inlineCompletionsCache.add(results)
+            return results.map(toInlineCompletionItem)


Bug fix: We were recording completions as being suggested even though they might all be empty.

I've also added some logging to get a better understanding of latency and timeouts used.

philipp-spiess · 2023-05-24T13:44:01Z

client/cody/src/completions/provider.ts

@@ -213,7 +213,7 @@ export class EndOfLineCompletionProvider extends CompletionProvider {
        suffix: string,
        injectPrefix: string,
        defaultN: number = 1,
-        protected multiline: boolean = false
+        protected multilineMode: null | 'block' | 'statement' = null


Change: While not needed yet, this adds support for a statement delimited multiline request as well. This will use different conditions to truncate the response and will be necessary to allow completions in these scenarios:

const result =

philipp-spiess · 2023-05-24T13:45:17Z

client/cody/src/completions/provider.ts

+            // If odd indentation is detected (i.e Claude adds a space to every line),
+            // we fix it for the whole multiline block first.
+            //
+            // We can skip the first line as it was already corrected above
+            if (hasOddIndentation) {
+                for (let i = 1; i < lines.length; i++) {
+                    if (indentation(lines[i]) >= startIndent) {
+                        lines[i] = lines[i].replace(/^\ /, '')
+                    }
+                }
+            }


New heuristics: We already identified "odd" space insertions before. After more trial and error, I noticed that when this case happens (where Claude inserts a space before the response even though the indentation would match), it will actually do so for every line that is part of this indentation in a multiline request.

This tries to fix it. 🤞

philipp-spiess · 2023-05-24T13:45:52Z

client/cody/src/completions/provider.ts

@@ -302,7 +323,7 @@ export class EndOfLineCompletionProvider extends CompletionProvider {
                if (indentation(line) < startIndent) {
                    // When we find the first block below the start indentation, only include it if
                    // it is an end block
-                    if (line.trim() === '}') {
+                    if (line.trim().startsWith('}')) {


Change: This makes it so that if something closes in e.g. }), it would also be emitted.

philipp-spiess · 2023-05-24T13:48:28Z

client/cody/src/completions/provider.ts

+        // If a completed line matches the next line of the suffix 1:1, we remove
+        const lines = completion.split('\n')
+        const matchedLineIndex = lines.findIndex((line, index) => {
+            if (index === 0) {
+                line = currentLinePrefix + line
+            }
+            if (line.trim() !== '' && nextLine.trim() !== '') {
+                // We need a trimRight here because the machine likes to add trailing whitespace.
+                //
+                // TODO: Fix this earlier in the post process run but this needs to be careful not
+                // to alter the meaning
+                return line.trimRight() === nextLine
+            }
+            return false
+        })
+        if (matchedLineIndex !== -1) {
+            completion = lines.slice(0, matchedLineIndex).join('\n')


New heuristics: Single and multi-line completions are now being checked to exactly match the next line. In case of a single line completion, they will be removed. In case of multi-line, we truncate the response. See this comparison:

Before:

After:

(added some logging to the second screenshot to show you this actually worked)

philipp-spiess · 2023-05-24T13:49:09Z

client/cody/src/completions/provider.ts

+        // Ignore completions that start with a whitespace. These are handled oddly in VS Code
+        // since you can't accept them via tab.
+        //
+        // TODO: Should we trim the response instead?
+        if (completion.trimStart().length !== completion.length) {
+            return null
+        }


New heuristics: We have gotten complaints the the suggestions would start with whitespace which will make it impossible to accept them via tab (since that would instead add a tab character if that is the case).

So for now, filter them out.

sourcegraph-bot · 2023-05-24T14:02:07Z

📖 Storybook live preview

eseliger · 2023-05-24T14:23:14Z

client/cody/src/completions/index.ts

@@ -28,6 +28,7 @@ export class CodyCompletionItemProvider implements vscode.InlineCompletionItemPr
    private maxSuffixTokens: number
    private abortOpenInlineCompletions: () => void = () => {}
    private abortOpenMultilineCompletion: () => void = () => {}
+    private lastContentChanges: Map<string, 'add' | 'del'> = new Map()


since I barely ever restart vscode, it could be that this grows unboundedly 🤔 Maybe if it's not a ton of effort rather do it now vs bad surprises later?

philipp-spiess · 2023-05-24T17:00:47Z

Shipping behind the existing feature flag.

This PR includes a bunch of fixes regarding #51344 I've not done a good job at splitting this up into commits and I will also need to add some tests for this (I have some ideas for how to make an effective test setup for this already) but for now, I did some extensive explanation of the changes and why they are added inline 🙂 ## Test plan See test plan added to the inline discussions.

philipp-spiess added 4 commits May 24, 2023 15:18

Cody completion: Mega bug bash thread

ca6c40b

Log latency and timeouts used

8427863

Cleanup

d247bb6

Improve variable names

bcc2727

cla-bot bot added the cla-signed label May 24, 2023

github-actions bot added the team/code-exploration Issues owned by the Code Exploration team label May 24, 2023

Bugfix

b75a8d0

philipp-spiess self-assigned this May 24, 2023

philipp-spiess requested review from beyang, valerybugakov and a team May 24, 2023 13:52

philipp-spiess commented May 24, 2023

View reviewed changes

philipp-spiess marked this pull request as ready for review May 24, 2023 13:52

eseliger approved these changes May 24, 2023

View reviewed changes

philipp-spiess added 3 commits May 24, 2023 16:43

Fix linter

42ef05a

Use LRU cache for insertn mode detection

ec6c6b5

Add REAMDE

454201c

philipp-spiess merged commit cedfa50 into main May 24, 2023
22 of 23 checks passed

philipp-spiess deleted the ps/completion-fixes branch May 24, 2023 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cody completion: Mega bug bash thread #52365

Cody completion: Mega bug bash thread #52365

philipp-spiess commented May 24, 2023 •

edited

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023

eseliger May 24, 2023

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023 •

edited

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023

philipp-spiess May 24, 2023

sourcegraph-bot commented May 24, 2023 •

edited

eseliger May 24, 2023

philipp-spiess commented May 24, 2023

Cody completion: Mega bug bash thread #52365

Cody completion: Mega bug bash thread #52365

Conversation

philipp-spiess commented May 24, 2023 • edited

Test plan

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

philipp-spiess May 24, 2023 • edited

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

sourcegraph-bot commented May 24, 2023 • edited

Choose a reason for hiding this comment

philipp-spiess commented May 24, 2023

philipp-spiess commented May 24, 2023 •

edited

philipp-spiess May 24, 2023 •

edited

sourcegraph-bot commented May 24, 2023 •

edited