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: Do not trigger a completion if a single-line completion was accepted #2446

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Dec 20, 2023

This is a small optimization that should cut down a bit on unnecessary LLM request:

In VS Code, if you accept a completion, this changes the document and thus issue a new completion request. Makes sense! The thing is: we always complete full lines so the chance that you do want a follow-up completion is very slim. In some cases the ending ) or ; helps to guard against this but not in all cases.

This PR adds a special logic now to detect this case and not issue a completion (so accepting a completion via tab won't issue a new request but any further keystroke (like enter) will).

Test plan

  • Added tests
Screen.Recording.2023-12-20.at.15.33.07.mov

@philipp-spiess philipp-spiess requested a review from a team December 20, 2023 14:37
@philipp-spiess philipp-spiess self-assigned this Dec 20, 2023
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

Looks great. Some feedback inline about readable tests.

}
)
)
).not.toEqual<V>(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

// Thus, this results in a request that almost always has zero results but still incurs network
// and inference costs.
it('should not make a request after accepting a completion', async () => {
const initialRequestParams = params(
Copy link
Contributor

Choose a reason for hiding this comment

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

This test and the next one would be more readable if they used a helper function that's parameterized on the trigger kind to show that, exact same situation with implicit trigger doesn't cause a follow on completion attempt, but manual trigger does.

The difference in trigger kind is only one line, it is hard to spot it in the midst of the rest of the test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

muuuch better 🙇

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Nice!

@philipp-spiess philipp-spiess merged commit 5cda5cf into main Dec 21, 2023
13 of 14 checks passed
@philipp-spiess philipp-spiess deleted the ps/ac-no-request-after-accepting branch December 21, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants