fix(trtllm): forward routing.priority to the engine, mapped into range - #30
fix(trtllm): forward routing.priority to the engine, mapped into range#30sopwg612 wants to merge 2 commits into
Conversation
The trtllm handler reads priority off the top level of the request. That key is the canary health-check pin from ai-dynamo#8488 and nothing else sets it -- the Rust frontend puts per-request priority at routing.priority, which ai-dynamo#7492 wired for vllm and sglang but never for trtllm. So every real request fell through to DEFAULT_REQUEST_PRIORITY and per-request priority never reached the engine, which is the behaviour the docs record as "not currently exposed through Dynamo" for TensorRT-LLM. Keep the top-level read first so the canary pin still wins, and fall back to routing.priority beneath it. routing is already in scope from the dp_rank lookup twelve lines up. The value is mapped rather than passed through: routing.priority is dynamo's unbounded higher-is-urgent scale, while GenerationRequest validates a float in [0.0, 1.0] (tensorrt_llm/executor/request.py). clamp(0.5 + 0.1 * p) puts 1 at 0.6 and saturates at 5, matching the map deepapi already uses for its aggregated pytrtllm path. An earlier attempt passed the raw value and collided with that validator. vllm and sglang need nothing -- both already read routing.priority, and sglang normalizes in its own adapter the same way. NOT RUN: tensorrt_llm is not importable on the host these were written on, and the test module skips without CUDA. The new tests need a GPU build container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqzQrZR2qrvxM6DsxqGciu
|
CI note — the three red checks are pre-existing fork infrastructure/lint noise, not this change:
Running pre-commit locally against only this PR's two files passes every formatting hook (isort, black, flake8, ruff, codespell, whitespace/EOL) and produces zero marker violations for The already-merged #29 shows the same three failures on the same base branch, which is the confirmation that this is the branch's steady state rather than a regression here. |
|
Follow-up with the decisive test, since "same failures as #29" was only circumstantial.
modifying 15 files —
So this PR cannot turn those green by changing anything in this PR — it needs a separate branch-wide formatting cleanup. Flagging rather than folding a 15-file reformat of vendored TRT-LLM patch files into a 2-file behavior change. |
|
Fixed, in #31 rather than here. The red checks were the branch's, not this PR's — #31's CI: I also verified this PR specifically: cherry-picking Merge order: #31 first, then rebase this. |
pre-commit/action runs --all-files, so every PR against this branch inherits its lint state -- isort, black, ruff and the pytest-marker report have all been red on the branch tip itself, which buries any real finding a feature PR might introduce. - isort/black over 8 planner modules plus trtllm/publisher.py. - Exclude container/deps/**/patches/ . Those are verbatim snapshots of upstream engine source that a Dockerfile COPYs over the installed package; they are maintained by diffing against upstream, so reformatting them destroys the diff. All four ruff findings were in that tree and are upstream's code, not ours. - Add the standard planner pytestmark block to the four unit test files that lacked it, clearing all 65 missing marker sets. Formatting only: the AST of every touched file is unchanged, except rust_adapter.py where isort reorders two stdlib imports (same import set, same non-import AST, no executable code interleaved) and the four test files which gain exactly the pytestmark node. Signed-off-by: Sihan Wang <sihan@deepinfra.com>
GitHub Actions
Failure
1 participant
The gap
Two upstream changes never met:
nvext.agent_hints.priority→routing.priority, and wired vllm and sglang to read it.So trtllm reads:
while every other adapter reads
routing:handlers.pyrouting.get("priority", 0)prefill_handler.pyrouting.get("priority")decode_handler.py(request.get("routing") or {}).get("priority")request.get("priority", ...)← top levelPreprocessedRequesthas no top-levelpriorityfield, so real traffic always falls through toDEFAULT_REQUEST_PRIORITYand per-request priority never reaches the engine. That's the behaviour the docs record as "TensorRT-LLM: per-request engine scheduling priority is not currently exposed through Dynamo."The fix
Top-level read stays first, so the canary pin from ai-dynamo#8488 still wins.
routing.prioritybecomes the fallback beneath it.routingis already in scope — bound twelve lines up fordp_rank.Why mapped, not passed through
routing.priorityis dynamo's unbounded, higher-is-urgent scale.GenerationRequest.__init__validates a float in[0.0, 1.0](tensorrt_llm/executor/request.py). Passing the raw value collides with that validator.clamp(0.5 + 0.1 * p)is the map deepapi already uses for its aggregated pytrtllm path, sign pre-flipped:routing.priorityPer-engine conversion in the adapter is the established pattern here — vllm negates for its lower-is-urgent convention, sglang does
int()plus a guard on the inverting flag.Requires
scheduler_config.waiting_queue_policy=priorityon the worker. Under the default FCFS waiting queue TRT-LLM logs "the priority value will be ignored" and discards it.Testing
All 49 tests in
test_trtllm_handler_base.pypass on GPU (L40S), run insidethe image the prod workers actually use
(
dynamo-trtllm-runtime:07-09-62fb5b6f-...) with this branch'scomponents/src/dynamo/trtllmmounted over the installed package. The nineTestHealthCheckPrioritycases are confirmed individually as PASSED, notskipped -- the class is async, so it needs
pytest-asyncioplus the repo'sasyncio_mode=auto, without which pytest silently skips the whole class.Added: parametrized mapping (1→0.6, 2→0.7, 5→1.0, 50→1.0, 0→0.5), and that the health-check top-level pin beats a conflicting
routing.priority.Second commit: greening the branch CI
pre-commit/actionruns--all-files, so every PR againstfeat-deepinfra-runtime-07-09inherited the branch's own lint debt: checking outthe branch tip with zero PR changes applied fails isort, black, ruff and the
pytest-marker report, and
pre-merge-status-checkthen goes red by aggregation.That buries any real finding a feature PR might introduce, so the fix rides
along here (originally split out as #31, now folded in at your request).
plannermodules andtrtllm/publisher.py.container/deps/trtllm/patches/v1.3.8/py_executor.py(F821 Undefined name 'ray',E712 == False). That is a verbatim snapshot of upstream TRT-LLMsource that a Dockerfile COPYs over the installed package, maintained by
diffing against upstream — reformatting it destroys the diff, and its findings
are upstream's code. So
container/deps/.*/patches/.*joins the existingtop-level
exclude, which already covers*.patch; these are the same thingkept as
.py.pytestmark = [gpu_0, pre_merge, unit, planner]added to the four unit testfiles lacking it, matching ~10 siblings that already carry it. Clears all 65
missing marker sets.
test_num_req_gate.pyalso neededimport pytest,placed above its deliberate
prometheus_namescompatibility shim so the shimand its trailing late import stay intact.
Formatting only — verified, not asserted. Every touched file's AST was
dumped before and after and compared: 11 of 12 identical;
rust_adapter.pydiffers only because isort reorders two stdlib imports (identical import set,
identical non-import AST, zero executable statements interleaved among them);
the four test files differ by exactly the added
pytestmarknode.pre-commit run --all-filesnow passes clean on this branch, and the sameconfig verified green in CI on #31 (
pre-commitpass,pre-merge-status-checkpass) before being folded in here.
Still red, not fixable by code
docker pull ghcr.io/deepinfra/dynamo/helm-tester:0.1.1→manifest unknownon this fork,so the header script never runs. Fails on every deepinfra/dynamo PR regardless
of content.
Both were documented the same way in #26.
Related
Supersedes #28 (reverted by #29) — same read, but with the range mapping that #28 was missing. Absorbs #31 (branch CI cleanup), which is closed in favour of the second commit here. Companion deepinfra/backend PR ai-dynamo#3982 emits the header.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QqzQrZR2qrvxM6DsxqGciu