Skip to content

fix: keep text when truncating zero characters in context shift - #679

Open
OsamaAnsar wants to merge 1 commit into
withcatai:masterfrom
OsamaAnsar:fix/truncate-text-zero-size
Open

OsamaAnsar wants to merge 1 commit into
withcatai:masterfrom
OsamaAnsar:fix/truncate-text-zero-size

Conversation

@OsamaAnsar

Copy link
Copy Markdown

Description of change

truncateTextAndRoundToWords truncated from the end with text.slice(0, -truncateSize). When truncateSize is 0 this is text.slice(0, -0), i.e. an empty string, so truncating nothing from the end deleted the whole text (the later truncateSize === 0 guard is never reached because of the early res.length === 0 return). The truncateStart = true branch already returned the text unchanged for size 0.

This is reachable from eraseFirstResponseAndKeepFirstSystemChatContextShiftStrategy: in compressHistoryThatLedToModelResponseAtIndex, removeChars is clamped to 0 to mean "keep this user message", but the resulting empty string makes the code treat it as fully removed and drop the history item. In truncateLlamaTextAndRoundToWords, once the requested size was consumed exactly at an item boundary, the next item was truncated by 0 (so an earlier string became "") and a following SpecialTokensText or SpecialToken was dropped as well.

Changes:

  • return the text as is when truncateSize <= 0 (this also covers negative sizes, where slice(-n) / slice(0, n) previously produced unrelated results)
  • in truncateLlamaTextAndRoundToWords, once the size has been fully consumed, keep the remaining items whole

Before / after (real output from running the functions, npx tsx):

                                      before             after
tr("hello world", 0, _, false)        ""                 "hello world"
tr("hello world", 0, _, true)         "hello world"      "hello world"
tr("hello world", -3, _, false)       ""                 "hello world"
LlamaText ["abc", <x>, "def"], drop 3 from end    ["abc"]        ["abc","<x>"]
LlamaText ["abc", <x>, "def"], drop 3 from start  ["def"]        ["<x>","def"]

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.ts
  • test/standalone/utils/eraseFirstResponseAndKeepFirstSystemChatContextShiftStrategy.test.ts

With only the source fix reverted, 7 of the 11 new tests fail; with the fix all 11 pass. tsc --noEmit and 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

  • Code is up-to-date with the master branch
  • npm run format to apply eslint formatting
  • npm run test passes with this change - the new tests, tsc --noEmit and eslint pass; tests that need the native addon / models were not run
  • This pull request links relevant issues as Fixes #0000 - N/A, no existing issue found
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change - N/A, no API or usage change
  • The new commits and pull request title follow conventions explained in pull request guidelines (PRs that do not follow this convention will not be merged)

`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

No deployments
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.

1 participant