Fix/allowed token ids validation - #1455
Open
DivyaNarahari97 wants to merge 2 commits into
Open
Conversation
stop_sentences_to_token_ids() drops entries that encode to no tokens
(e.g. "", [], or any string the tokenizer maps to nothing), but
StopSequenceGroups.initialize() then indexed the *original*
stop_sequences list by the *filtered* list's index. Every entry after a
dropped one shifted by one position, so a group's sequence_str came from
the wrong stop entry.
Two user-visible failures:
stop=["", "END"] -> to_strings() == []
"END" silently loses string matching entirely.
stop=["unknown", "stop2"] -> to_strings() == ["unknown"]
generation stops on a string the user never requested, because
"unknown"'s string got attached to "stop2"'s token ids.
The second is the damaging one: these strings drive stop matching in
DecodeReq.stop_sequences_str_match() and trailing-stop trimming in the
OpenAI completion path, so a request can terminate early on unrelated
text.
Carry (token_ids, original_entry) pairs through the filter so the two can
never drift apart. stop_sentences_to_token_ids() keeps its original
signature as a thin wrapper over the new helper.
Also repairs the test module, which imported a DecodeNode class that no
longer exists in sampling_params and therefore failed at collection --
meaning none of these tests had been running. Replaced with an equivalent
NodeUUId round-trip test and added regression coverage for the alignment
bug above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…buffer
AllowedTokenIds.initialize() asserted over self.ids -- the ctypes array
being written into, which at that point is still zero-filled -- rather
than the incoming ids argument. Iterating a c_int array always yields
Python ints, so the assertion was vacuously true and never rejected
anything.
Non-int input therefore fell through to the slice assignment on the next
line and surfaced as a raw ctypes TypeError ("'str' object cannot be
interpreted as an integer") instead of the intended AssertionError with
its message. Matches the equivalent check in StopSequence.initialize,
which correctly validates its argument.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
fix(sampling): correct stop-sequence string pairing and allowed_token_ids validation
Summary
Two independent bugs in
lightllm/server/core/objs/sampling_params.py, plus a repair to the test module that was supposed to be covering them.1. Stop sequence token ids could be paired with the wrong string
stop_sentences_to_token_ids()drops entries that encode to no tokens ("",[], or any string the tokenizer maps to nothing).StopSequenceGroups.initialize()then indexed the originalstop_sequenceslist using the filtered list's index:Every entry after a dropped one shifts by a position, so a group's
sequence_strcomes from the wrong stop entry.Impact — two user-visible failures:
stop=to_strings()before["", "END"][]"END"silently loses string matching entirely["unknown", "stop2"]["unknown"]The second is the damaging one. These strings drive stop matching in
DecodeReq.stop_sequences_str_match()and trailing-stop trimming in the OpenAI completion path, so a request can terminate early on unrelated text.Fix — carry
(token_ids, original_entry)pairs through the filter so the two can't drift apart.stop_sentences_to_token_ids()keeps its original signature as a thin wrapper over the new helper, so no callers change.2.
AllowedTokenIds.initialize()validated the wrong variableself.idsis thec_intarray being written into, still zero-filled at that point. Iterating a ctypesc_intarray always yields Pythonints, so the assertion is vacuously true for every input and rejects nothing.Impact — low. Bad input was still rejected, just one line later and with a confusing error:
Fix — assert over
ids(the argument). This matchesStopSequence.initialize()a few classes up, which validates its argument correctly.3. The test module for all of the above never ran
unit_tests/server/core/objs/test_sampling_params.pyimportedDecodeNode, a class that no longer exists insampling_params. The module failed at collection, so none of its tests had been running. Replaced with an equivalentNodeUUIdround-trip test.Testing
test_stop_sequence_groups_keeps_ids_and_strings_aligned, covering the aligned case, both drop-then-shift cases, and pure-id entries.test_allowed_token_ids_rejects_non_int.