Skip to content

fix(chat): Prune history if it's too large, ignore log window as active file #6989

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 10, 2025
Merged

fix(chat): Prune history if it's too large, ignore log window as active file #6989

merged 1 commit into from
Apr 10, 2025

Conversation

@jguoamz
Copy link
Contributor

@jguoamz jguoamz commented Apr 10, 2025 •
edited
Loading

Problem

An error occurred while processing your request.This error is reported to the team automatically. We will attempt to fix it as soon as possible.Details: Input is too long.
  • Handle Input is too long. by pruning history
  • A bug where status code is not being populated in error message
  • A bug where output log window can be considered as active file which can cause waste of tokens
  • A bug where toolUse JSON.parse is not being handled correctly

Solution

  • Prune history if it's too large
  • Fix status code bug in error message
  • Fix active file bug
  • Fix JSON.parse handling
  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Apr 10, 2025

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@jguoamz jguoamz force-pushed the historySize branch 5 times, most recently from c43d50f to fd340bf Compare April 10, 2025 03:05
@jguoamz jguoamz marked this pull request as ready for review April 10, 2025 03:06
@jguoamz jguoamz requested a review from a team as a code owner April 10, 2025 03:06
zixlin7
zixlin7 reviewed Apr 10, 2025
View reviewed changes
packages/core/src/codewhispererChat/storages/chatHistory.ts
} else {
this.logger.debug('No valid starting user message found in the history, clearing')
this.history = []
private calculateHistoryCharacterCount(): number {
Copy link
Contributor

@zixlin7 zixlin7 Apr 10, 2025

Choose a reason for hiding this comment

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

can we make use of the logic here? https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/codewhispererChat/controllers/chat/chatRequest/converter.ts#L34

Copy link
Contributor Author

@jguoamz jguoamz Apr 10, 2025

Choose a reason for hiding this comment

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

I think the usecase are too different and the history truncation/handling already exists in chatHistory.ts

zixlin7
zixlin7 reviewed Apr 10, 2025
View reviewed changes
packages/core/src/codewhispererChat/storages/chatHistory.ts
}
} while (
this.calculateHistoryCharacterCount() > MaxConversationHistoryCharacters &&
this.history.length > 2
Copy link
Contributor

@zixlin7 zixlin7 Apr 10, 2025

Choose a reason for hiding this comment

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

is it possible to exceed the chars count with history.length === 2?

Copy link
Contributor Author

@jguoamz jguoamz Apr 10, 2025

Choose a reason for hiding this comment

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

not possible

tsmithsz
tsmithsz reviewed Apr 10, 2025
View reviewed changes
packages/core/src/codewhispererChat/storages/chatHistory.ts
// Maximum number of messages to keep in history
const MaxConversationHistoryLength = 100
// Maximum number of characters to keep in history
const MaxConversationHistoryCharacters = 600_000
Copy link
Contributor

@tsmithsz tsmithsz Apr 10, 2025

Choose a reason for hiding this comment

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

Should we be more conservative with this number to leave room for the system prompt?

Copy link
Contributor Author

@jguoamz jguoamz Apr 10, 2025

Choose a reason for hiding this comment

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

this is already conservative given each token is roughly 3.5 chars

Copy link

@mr-lee mr-lee Apr 10, 2025

Choose a reason for hiding this comment

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

You also need to make sure that the validation logic inside Maestro is honored when pruning history

Copy link
Contributor Author

@jguoamz jguoamz Apr 10, 2025

Choose a reason for hiding this comment

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

yep, we handle that in ensureCurrentMessageIsValid

jguoamz
jguoamz commented Apr 10, 2025
View reviewed changes
packages/core/src/codewhispererChat/controllers/chat/controller.ts
customization: getSelectedCustomization(),
toolResults: toolResults,
origin: Origin.IDE,
context: session.context ?? [],
Copy link
Contributor Author

@jguoamz jguoamz Apr 10, 2025

Choose a reason for hiding this comment

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

setting falcon context here in the toolResults response will waste tokens, hence removing it

@jguoamz jguoamz force-pushed the historySize branch 3 times, most recently from 57ee56c to f3e9247 Compare April 10, 2025 15:32
@jguoamz jguoamz changed the title fix(chat): Prune history if it's too large fix(chat): Prune history if it's too large, populate statusCode in error message correctly, do not use log window as active document Apr 10, 2025
@jguoamz jguoamz changed the title fix(chat): Prune history if it's too large, populate statusCode in error message correctly, do not use log window as active document fix(chat): Prune history if it's too large, populate statusCode in error message, ignore log window as active file Apr 10, 2025
@jguoamz jguoamz closed this Apr 10, 2025
@jguoamz jguoamz reopened this Apr 10, 2025
@jguoamz jguoamz force-pushed the historySize branch 2 times, most recently from 253c90b to d398c74 Compare April 10, 2025 15:59
@jguoamz jguoamz changed the title fix(chat): Prune history if it's too large, populate statusCode in error message, ignore log window as active file fix(chat): Prune history if it's too large, ignore log window as active file Apr 10, 2025
@jguoamz jguoamz force-pushed the historySize branch 2 times, most recently from 2e58d0c to 0bbf73e Compare April 10, 2025 16:13
@zixlin7 zixlin7 merged commit 90dfc71 into aws:feature/agentic-chat Apr 10, 2025
19 of 22 checks passed
@jguoamz jguoamz deleted the historySize branch April 10, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@mr-lee mr-lee mr-lee left review comments

@zixlin7 zixlin7 zixlin7 approved these changes

@tsmithsz tsmithsz tsmithsz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jguoamz @mr-lee @zixlin7 @tsmithsz