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

Autocomplete: dynamically switch to multliline completions #1894

Merged
merged 12 commits into from
Nov 29, 2023

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented Nov 27, 2023

Context

  • TLDR: we now use the completion's first line to determine if it starts the new multiline syntax node. If it does, we continue streaming until the completion is truncated or we reach the token sample limit.
  • Closes Tree-sitter: dynamically switch singleline completions to multiline if the first line indicates the block start #1619
  • Adds a feature flag cody-autocomplete-dynamic-multline-completions to run an experiment on the updated logic.
  • Adds an experimental setting, cody.autocomplete.experimental.dynamicMultilineCompletions to test the feature locally.
  • Adds completionPostProcessLogger used for local debugging. It's disabled by default, and I will replace it with OpenTelemetry spans in a follow-up PR. I put a little effort into improving this logger because it's a throw-away code, but I need it here because debugging streaming-truncation bugs is much faster with it.
  • Revamps the multiline truncation logic:
    • We no longer look for the next new sibling after the completion. This approach is unreliable because the parse tree often breaks because of incomplete code in the completion insert text.
    • Instead, we take the descendant at the current cursor position (or multiline trigger position), look for the farthest ancestor that starts at the same line, and then wait for a moment when completion generates more line than this syntax node has. This means we do not care about broken parse-tree after the node under the cursor:
// Start completion
function test() {
  
}

// ---------------------------

// Continue streaming
function test() {
  █if (true) {
    console.log('hello world'
}

// ---------------------------

// Stop, because characters were added outside of `function test` syntax node boundaries
function test() {
  █if (true) {
    console.log('hello world'
}

function 

Screenshots

Group 2
Group 3
Group 4
Group 1

Follow-ups

Merged preparation PRs

Test plan

  • Tested manually:
    • Ensure that code completions are generated without changes on this branch.
    • Ensure that code completions are dynamically switched to multiline based on the first line when cody.autocomplete.experimental.dynamicMultilineCompletions is set to true.
  • Existing tests pass.
  • The functionality is disabled by default. I'm going to add more unit tests for the new logic in a separate PR (see planned follow-ups section).

@valerybugakov
Copy link
Member Author

Hey @philipp-spiess, could you take an early look at this? I still have outstanding TODOs, but the core implementation is finished.

@valerybugakov valerybugakov changed the base branch from main to vb/fix-parse-completion-positions November 27, 2023 10:56
@valerybugakov valerybugakov changed the base branch from vb/fix-parse-completion-positions to main November 27, 2023 10:57
Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

I’m SO excited for this. Pre-emptive approval but will stamp once it's ready for review :)

@umpox
Copy link
Contributor

umpox commented Nov 28, 2023

@valerybugakov Should this trigger a multiline completion:

image

I ran it on the HumanEval multiline dataset we have - it's just Python but should show a significant difference here, but it didn't seem to be generating multiline completions for these cases.

I have cody.autocomplete.experimental.dynamicMultilineCompletions set to true. Do I need to do anything else?

@philipp-spiess
Copy link
Contributor

@umpox I think this example won't trigger a multi-line completion since it already has content that has a deeper indentation.

Maybe that's a check we can bypass when manually triggering completions though? Can you try to comment this out: https://sourcegraph.com/github.com/sourcegraph/cody/-/blob/vscode/src/completions/detect-multiline.ts?L52%3A21-54%3A22

@umpox
Copy link
Contributor

umpox commented Nov 28, 2023

@philipp-spiess Yep that fixes it:

image

There does seem to be something that seems off though, surely this should still generate a multi line completion?:

image

🤔

@valerybugakov
Copy link
Member Author

@umpox, thanks for sharing the screenshots. The issue should be fixed here: ed20acb

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Let's ship it!

@valerybugakov valerybugakov merged commit a62d3d4 into main Nov 29, 2023
14 checks passed
@valerybugakov valerybugakov deleted the vb/dynamic-multiline-completions branch November 29, 2023 11:13
@olafurpg
Copy link
Member

This is huge! Looking forward to trying this out 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants