fix: TiktokenCounter counts literal special-token strings as ordinary text - #12888
Shubhampm18 wants to merge 1 commit into
Conversation
… text count() encoded rendered messages/tools via Encoding.encode(), whose disallowed_special="all" default raises ValueError on text that merely contains a special-token marker such as the endoftext sentinel. Since allowed_special defaults to an empty set, encode() can never emit a special token here -- the strict check only turns ordinary source text into a crash. Switch to encode_ordinary(), mirroring the fix already landed for the token-based document splitters (deepset-ai#12853), and note the behavior in the constructor docstring. The fake encoder in the tests records calls to encode_ordinary and fails loudly if encode() is used again. Fixes deepset-ai#12869
|
@Shubhampm18 is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
Hi @Shubhampm18, thanks for your interest in contributing to Haystack! 🙏 This is an automated message to help us keep the review queue healthy. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add tool-schema regression coverage and document the behavior in the relevant documentation pages.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR fixes TiktokenCounter crashes on literal tiktoken special-token strings.
Changes:
- Uses
encode_ordinary()for rendered messages and tools. - Adds regression tests and documentation.
- Adds a release note.
| File | Description |
|---|---|
test/token_counters/test_tiktoken_counter.py |
Adds special-token regression coverage. |
releasenotes/notes/fix-tiktoken-counter-special-tokens-3a8f27d1e6b4c05a.yaml |
Documents the bug fix. |
haystack/token_counters/tiktoken_counter.py |
Uses ordinary encoding for rendered content. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def test_counts_literal_special_token_text_as_ordinary(self, fake_encoder): | ||
| # Regression for #12869: count() must route text through encode_ordinary so a literal | ||
| # special-token marker cannot raise the way encode()'s disallowed_special="all" default would. |
|
Thank you for your efforts! We're closing this PR because it duplicates #13009, which makes the same one-line fix to |
Related Issues
<|endoftext|>#12869Proposed Changes
TiktokenCounter.count()rendered messages/tools throughEncoding.encode(), whosedisallowed_special="all"default raisesValueErroron text that merely contains a special-token marker such as<|endoftext|>. Sinceallowed_specialdefaults to an empty set,encode()can never emit a special token here — the strict check only turns ordinary source text into a crash.Switches to
encode_ordinary(), mirroring the fix already merged for the token-based document splitters (#12853), and notes the behavior in the constructor docstring.How did you test it?
encode()raises;encode_ordinary()counts fine).test/token_counters/test_tiktoken_counter.py: 15 passed. The fake encoder now recordsencode_ordinaryand fails loudly ifencode()is used again; new unit + parameterized real-encoder integration tests cover the literal marker.ruff checkandruff format --checkpass on the changed files.Notes for the reviewer
The change is one call-site in
count(); everything else is tests/docs. This was the last remaining bare.encode()of user-supplied text inhaystack/after #12853, as noted in the issue.This PR was fully generated with an AI assistant. I have reviewed the changes and run the relevant tests.