-
Notifications
You must be signed in to change notification settings - Fork 751
feat(amazonq): refactor: implement payload size management for chat requests #6820
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
Conversation
|
| getLogger().debug(`current request total prompts size: ${totalPromptSize}`) | ||
|
|
||
| // Type C context: Preserving current file context as much as possible | ||
| // truncate the text to keep texts in the middle instead of blindly truncating the tail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity did this show better improvements on the backend or anything? Or was it just an idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously we cap userInput at 4k which is very hardcoded, for now it can allow up to 100k when there's no other context which can be flexible, backend always allow up to 100k for now so we fully utilize this. This was aligned within the product and eng.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I think you are referring to the specific "truncating to preserve the middle" logic. If this is the case, I do this because when we include current file context, we expand from the current highlighted code abvoe and below in parallel, so considering the "middle context" which is the highlighted code or current cursor to be the most relevant, we truncate head and tail instead of truncating tail, which may truncate out the middle context first.
By doing the above, we always truncate the middle context at last.
|
|
||
| // Type B2(explicit @files) context: Preserving files as much as possible | ||
| if (triggerPayload.additionalContents !== undefined) { | ||
| for (const additionalContent of triggerPayload.additionalContents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not inline with 97 just so you could have seperatation for the different type of processing you have to do? I think theres a lot that relies on additionalContents being defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand the question, this logic is very similar to line 97 logic, only difference being, line 97 is for @ prompt and line 114 is for @ files. Since we aligned that @ prompt has a higher priority than @ files, we preserve @ prompt first and then @ files comes in if there's still available space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, i'll wait until someone from codewhisperer reviews it though. The lint is failing with a minor thing btw
| "description": "User clicked on add region command" | ||
| }, | ||
| { | ||
| "name": "amazonq_addMessage", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this here instead of using the ones from commons ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that just got merged, I will quickly replace.
- Add size limits for different context types (user input, prompts, rules, files, workspace) - Implement truncation logic to preserve content around the middle of file text - Replace static contextMaxLength with workspaceChunkMaxSize for workspace content - Add debug logging for tracking payload sizes of different components - Filter out empty contexts from additionalContents and relevantTextDocuments - Update type definitions to include content type in AdditionalContentEntryAddition
…ed many undefined checks becaus we are treating them as 0 in the end anyway
| } | ||
| } | ||
|
|
||
| function preserveContexts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this function? Needs a docstring, with an input => output example.
| return truncationInfo | ||
| } | ||
|
|
||
| function truncateUserSpecificContexts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't the service do this, why does the client need to do it?
|
Surely this has a customer-facing impact that should be called out in the changelog? |
debug log:

telemetry:
Unit tests and telemetry pending update.
Problem
Solution
feature/xbranches will not be squash-merged at release time.