fix https://github.com/NVIDIA/TensorRT-Edge-LLM/issues/211: advance the Philox offset per sampled token - #212
Open
filipemartinsubrobotics wants to merge 1 commit into
Conversation
Both LLM sampler call sites called topKtopPSamplingFromLogits without a philoxOffset, so it took its default of 0 at every decode step. curand_init is keyed on that offset, so one uniform was drawn for the whole sequence and sampling collapsed onto the argmax: temperature and top_p changed which deterministic path was taken but never produced a sample. Advance a per-token offset at both sites, reset per sequence through the existing resetForNewSequences hook. Greedy decoding is untouched; sampling now differs from greedy and a given input still reproduces exactly. The TTS talker path already varies this offset deliberately, which is what identifies the omission as a bug rather than a choice. Adds a test pinning the kernel's offset contract. It passes on the unpatched tree by design -- the kernel was never at fault -- so it documents the invariant a caller must uphold rather than guarding the regression.
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.
What does this PR do?
Type of change: Bug fix
Overview: Both LLM sampler call sites called
topKtopPSamplingFromLogitswithout aphiloxOffset, so it took its declared default of0at every decode step.curand_init(seed, batchIdx, offset)is keyed on that offset, so a single uniform was drawn for an entire sequence and sampling collapsed onto the argmax — temperature andtop_pselected which deterministic path was taken but never produced a sample. This advances a per-token offset at both sites, reset per sequence through the existingresetForNewSequenceshook.Usage
No API change. Sampling parameters in the input JSON now take effect:
# {"batch_size": 1, "temperature": 0.3, "top_p": 0.8, "max_generate_length": 120, "requests": [...]} llm_inference --engineDir engines/llm --checkpointDir onnx/llm \ --inputFile req.json --outputFile out.jsonBefore: output byte-identical to
temperature: 0.0. After: differs from greedy, and still reproducible for a given input.Measured on an INT4-AWQ 2.44 B Cosmos reasoner, 120 generated tokens, three runs each:
Before the fix, four configurations x three runs plus the greedy baseline shared one sha256 over 679 characters.
Pull Request Checklist
Pre-commit Checks
pre-commitby runningpip install pre-commit.pre-commit install.pre-commit run --all-filesand fixed any reported issues.(Run scoped to the five changed files: clang-format, codespell, CRLF and license hooks all pass.
git-clang-formatcorrected a lambda brace — the repo setsBeforeLambdaBody: false, which my first draft got wrong.)Tests
SamplingTest.AdvancingPhiloxOffsetChangesSampledTokeninunittests/cpp/sampler/samplingTests.cpppins the offset contract: distinct offsets must be able to select distinct tokens, and a repeated offset must reproduce.One caveat, stated rather than buried: this test passes on the unpatched tree. The kernel was never at fault — the bug was two callers — so it documents the invariant a caller must uphold rather than guarding the regression. Catching a non-advancing caller would need a decoder-level test against a real runtime, which I have not written. If you would like that, say so and I will attempt it.
Documentation
--helptext already said these parameters belong in the input JSON, which is now true in effect as well as in intent.Compatibility
Additional Information
Fixes #211. My first diagnosis in that issue was wrong and is corrected in a comment there: the parse was never missing.
What identifies this as an omission rather than a design choice is that the TTS talker path already varies the offset deliberately —
qwen3OmniTTSRuntime.cpp:4085passes/*philoxOffset=*/reprefillOffsetunder a comment about varying it per call.Out of scope on purpose: six TTS call sites carry the same omission (
qwen3OmniTTSRuntime.cpp2186, 2446, 2558, 2660, 2760, 4326). I have left them alone to keep this to one concern, and cannot test the TTS path. Happy to open a follow-up.A fixed seed (
42) is retained so a given input reproduces; the offset supplies the per-step variation. If you would prefer the seed to be user-settable, that is a separate feature and I have not added it.Verified on DGX B300 (SM103), CUDA 13.0, TensorRT 10.13.2, Edge-LLM v0.10.1 (
e8b2952).