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: Fix retrieval hints #2652

Merged
merged 13 commits into from
Jan 11, 2024
Merged

Autocomplete: Fix retrieval hints #2652

merged 13 commits into from
Jan 11, 2024

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Jan 10, 2024

I noticed that the context size hints returned values suffixed with chars but one of the values was not going through the same tokensToChars estimation.

This value was used by the context mixer and the retrieval strategies. Luckily, the Jaccard similarity strategy was not using that value. However, the context mixer was likely truncating results because of this, so I would suspect this to increase the average context length a bit (which could have a negative impact on performance). However there was another bug that counter-acted this change a bit. The previous totalFileContextChars was used to only count the additional code snippets (so excluding prefix and suffix) while the documentation said that this number is inclusive.

Also, the actual truncation happens at the prompt level and is unaffected by these hints. These only tell the context retriever how much context it should retrieve.

Some math shows us that the actual difference is not big:

Before:

maxRetrievedChars = maxContextTokens * 0.9

After:

maxRetrievedChars = 4 *  maxContextTokens * 0.9 - 4 * maxContextTokens * 0.6 - 4 * maxContextTokens * 0.1
maxRetrievedChars = 4 * maxContextTokens * ( 0.9 - 0.6 - 0.1 )
maxRetrievedChars = 4 * maxContextTokens * ( 0.2 )
maxRetrievedChars = maxContextTokens * 0.8

So yeah, almost the same 😅

However, with the new change, if fewer actual suffix or prefix characters are used, we will retrieve more context to fill up the window (which IMO is a good thing)

Test plan

  • Enable the feature flag
  • Make some autocomplete request
  • Observe that context snippers are still added.

@philipp-spiess philipp-spiess requested review from valerybugakov and a team January 10, 2024 10:23
@philipp-spiess philipp-spiess self-assigned this Jan 10, 2024
Comment on lines 49 to 51
totalFileContextChars: Math.floor(tokensToChars(0.9 * maxContextTokens)), // keep 10% margin for preamble, etc.
prefixChars: Math.floor(tokensToChars(0.6 * maxContextTokens)),
suffixChars: Math.floor(tokensToChars(0.1 * maxContextTokens)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow this doesn't add up to 100% haha, let me run some more tests to make sure this still works as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR descriptions with my findings. This was a fun one!

@philipp-spiess philipp-spiess marked this pull request as draft January 10, 2024 10:26
@philipp-spiess philipp-spiess marked this pull request as ready for review January 10, 2024 10:50
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 find!

@philipp-spiess philipp-spiess merged commit 97f9617 into main Jan 11, 2024
15 checks passed
@philipp-spiess philipp-spiess deleted the ps/fix-retrieval-hints branch January 11, 2024 15:34
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