Repository navigation
fix: keep text when truncating zero characters in context shift - #679
Open
OsamaAnsar wants to merge 1 commit into
Open
OsamaAnsar wants to merge 1 commit into
OsamaAnsar wants to merge 1 commit into
Conversation
`truncateTextAndRoundToWords` used `text.slice(0, -truncateSize)`, which is an empty string when `truncateSize` is 0, so truncating nothing from the end deleted the whole text (the `truncateStart` branch already returned the text as is). A context shift that clamped the number of characters to remove to 0 therefore dropped chat history items it meant to keep. Return the text as is when there is nothing to truncate (including negative sizes), and keep the remaining items of a `LlamaText` once the requested size has been fully consumed.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
truncateTextAndRoundToWordstruncated from the end withtext.slice(0, -truncateSize). WhentruncateSizeis0this istext.slice(0, -0), i.e. an empty string, so truncating nothing from the end deleted the whole text (the latertruncateSize === 0guard is never reached because of the earlyres.length === 0return). ThetruncateStart = truebranch already returned the text unchanged for size0.This is reachable from
eraseFirstResponseAndKeepFirstSystemChatContextShiftStrategy: incompressHistoryThatLedToModelResponseAtIndex,removeCharsis clamped to0to mean "keep this user message", but the resulting empty string makes the code treat it as fully removed and drop the history item. IntruncateLlamaTextAndRoundToWords, once the requested size was consumed exactly at an item boundary, the next item was truncated by0(so an earlier string became"") and a followingSpecialTokensTextorSpecialTokenwas dropped as well.Changes:
truncateSize <= 0(this also covers negative sizes, whereslice(-n)/slice(0, n)previously produced unrelated results)truncateLlamaTextAndRoundToWords, once the size has been fully consumed, keep the remaining items wholeBefore / after (real output from running the functions,
npx tsx):With the real context shift strategy, a
GeneralChatWrapper, a stub word-count tokenizer (no model) and a history of system + 3 user messages (60 words each) + one 300 word model response,maxTokensCount: 400: before the fix the first two user messages are removed entirely (the remaining history is system,uc...(60 words), model); after the fix the first user message is kept whole and the second is shortened (system,ua...(60 words),ub...(38 words), model).Tests (new, standalone, no model needed):
test/standalone/utils/truncateTextAndRoundToWords.test.tstest/standalone/utils/eraseFirstResponseAndKeepFirstSystemChatContextShiftStrategy.test.tsWith only the source fix reverted, 7 of the 11 new tests fail; with the fix all 11 pass.
tsc --noEmitand eslint on the changed files pass.Not tested: the native addon could not be built in my environment, so the existing suites that need it (
LlamaGrammar,AddonJinjaRenderer,resolveChatWrapper, ...) and the model-dependent tests were not run; no model inference was performed.Pull-Request Checklist
masterbranchnpm run formatto apply eslint formattingnpm run testpasses with this change - the new tests,tsc --noEmitand eslint pass; tests that need the native addon / models were not runFixes #0000- N/A, no existing issue found