chore: better quantization calibration loop for modelopt - #2806
Closed
michaelfeil wants to merge 2 commits into
Closed
chore: better quantization calibration loop for modelopt #2806michaelfeil wants to merge 2 commits into
michaelfeil wants to merge 2 commits into
Conversation
michaelfeil
changed the title
Collaborator
|
@michaelfeil @Tracin pls help review this MR. Thanks |
juney-nvidia
changed the title
Collaborator
|
Closing since no response after #2806 (comment). Feel free to rebase and reopen if the PR is still relevant! |
yuxianq
added a commit
to yuxianq/TensorRT-LLM
that referenced
this pull request
May 22, 2026
…and header Build #39475 failed Build-x86_64 NVIDIA#2806 and Build-SBSA NVIDIA#2786 with a linker error: undefined references to ``trtllmGenContextPreprocess``, ``trtllmGenContextPostprocess``, and ``trtllmGenGenerationPreprocess`` — all three with one extra trailing ``long`` parameter compared to the defined symbols in ``libth_common.so``. Root cause: ``trtllmGenQKVProcessOp.cpp`` (the definitions) had ``sink_token_length`` removed in a prior commit, but two surfaces still declared / passed it: 1. ``cpp/tensorrt_llm/thop/trtllmGenFusedOps.h`` (declarations consumed by ``bindings.cpp``) — still listed ``int64_t sink_token_length`` in all three signatures, so the compiler generated calls with the wrong arity that the linker could not resolve. 2. ``cpp/tensorrt_llm/nanobind/thop/bindings.cpp`` — the two lambda wrappers (``trtllmGenContextPreprocessBinding`` and ``trtllmGenGenerationPreprocessBinding``) still took ``sink_token_length`` as a parameter and forwarded it to the underlying call. The three ``m.def(...)`` registrations also still listed ``nb::arg("sink_token_length")``. Fix: drop ``sink_token_length`` from all six remaining call-sites — three in the header and three in the nanobind bindings. The Python callers in ``trtllm_gen.py`` already don't pass it. Verified: pre-commit (clang-format / ruff / etc.) all pass; no residual ``sink_token_length`` reference in the trtllm-gen surface. Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.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
3 participants
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.
Contribution by Baseten.co - ( engine builder team: https://docs.baseten.co/performance/engine-builder-overview )
Trt-LLM has too many parameters to set. batch_size for quantization is needn't to be one of them, and can be automatically infered with trial and error.
As follow-up, it would be nice to have these changes integrated in modelopt, and used in modelopt. Additionally, modelopt uses a get_max_batch_size(model, max_sample_length) to determine a suitable max batch size. None of the options will make this
We at Baseten want to expose e.g. batch_size=64 for quantization and run the same workflow for any model - lowering the batch size by factors of two as needed.