[None][feat] AutoDeploy: Fix hardcoded configs - #14943
Conversation
ADEngine's fake llm_args forced stream_interval=1 -> a response emitted every decode step -> per-token detok/harmony/HTTP at high concurrency. Read it from ad_config instead (PyExecutor gates emit on iter % stream_interval). gpt-oss-120b TP=2 conc=256 trtllm-serve: 17928 -> 23274 tok/s (+29.8%), 0.72x -> 0.93x PT. HTTP chunks/req 807->41.7 (= PT 46), ITL 12.1->7.5 ms, OSL mismatch 307->46. Signed-off-by: Taylor Yeonbok Lee <249374542+taylor-yb-lee@users.noreply.github.com>
…config ADEngine's fake llm_args hardcoded scheduling/batching stubs that silently dropped the user's yaml settings: - attention_dp_config = None -> attention-DP load balancing never engaged - batch_wait_timeout_ms = 0 - batch_wait_timeout_iters = 0 - batch_wait_max_tokens_ratio = 0.0 -> request-batching accumulation disabled Read them from ad_config (PyExecutor consumes all four, py_executor.py:366-374), falling back to the old stub defaults only when ad_config is absent. Pairs with the stream_interval fix already on this branch. Signed-off-by: Taylor Yeonbok Lee <249374542+taylor-yb-lee@users.noreply.github.com>
…rom ad_config Add regression tests in shim/test_engine.py (next to test_engine) covering the config-plumbing fix: - test_ad_engine_propagates_pyexecutor_scheduling_config: ad_config with stream_interval=20, attention_dp_config(enable_balance=...), batch_wait_* must surface on engine.llm_args (not the old hardcoded stubs). - test_ad_engine_scheduling_config_defaults_without_ad_config: ad_config=None falls back to stream_interval=1 / attention_dp_config=None / batch_wait_*=0. Skipped when CUDA is unavailable (ADEngine builds the model on CUDA). Signed-off-by: Taylor Yeonbok Lee <249374542+taylor-yb-lee@users.noreply.github.com>
Signed-off-by: Taylor Yeonbok Lee <249374542+taylor-yb-lee@users.noreply.github.com>
WalkthroughADEngine.init now conditionally initializes scheduling and batch-wait settings from ChangesADEngine scheduling configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/auto_deploy/singlegpu/shim/test_engine.py (2)
112-123: ⚡ Quick winAdd return type annotations to newly added helper/tests.
Please add explicit return types for
_make_cache_seq_interface,test_ad_engine_propagates_pyexecutor_scheduling_config, andtest_ad_engine_scheduling_config_defaults_without_ad_config.Suggested patch
-def _make_cache_seq_interface(device, max_seq_len=64, max_batch_size=8): +def _make_cache_seq_interface( + device: torch.device, max_seq_len: int = 64, max_batch_size: int = 8 +) -> CachedSequenceInterface: @@ -def test_ad_engine_propagates_pyexecutor_scheduling_config(): +def test_ad_engine_propagates_pyexecutor_scheduling_config() -> None: @@ -def test_ad_engine_scheduling_config_defaults_without_ad_config(): +def test_ad_engine_scheduling_config_defaults_without_ad_config() -> None:As per coding guidelines, "Static type checking with mypy is opt-in by submodule; always annotate functions with return types (use
Noneif function does not return)".Also applies to: 167-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/auto_deploy/singlegpu/shim/test_engine.py` around lines 112 - 123, Add explicit return type annotations to the newly added helper and test functions: annotate _make_cache_seq_interface to return CachedSequenceInterface, and annotate the tests test_ad_engine_propagates_pyexecutor_scheduling_config and test_ad_engine_scheduling_config_defaults_without_ad_config to return None; update any other nearby functions on lines referenced (e.g., the functions around 167-168) to include explicit return types as well so the module is fully annotated for mypy opt-in checks.
122-185: Test coverage for this regression scope is sufficient.
tests/unittest/auto_deploy/singlegpu/shim/test_engine.pynow covers both required paths: propagation fromad_configand fallback defaults whenad_configis omitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/auto_deploy/singlegpu/shim/test_engine.py` around lines 122 - 185, Tests added (test_ad_engine_propagates_pyexecutor_scheduling_config and test_ad_engine_scheduling_config_defaults_without_ad_config) verify ADEngine propagates scheduling/streaming knobs from an LlmArgs ad_config and falls back to previous stub defaults when omitted; if these tests fail, update ADEngine.__init__ (the ADEngine constructor) to copy stream_interval, attention_dp_config, batch_wait_timeout_ms, batch_wait_timeout_iters, and batch_wait_max_tokens_ratio from the provided ad_config LlmArgs into engine.llm_args (or preserve existing defaults when ad_config is None), ensuring AttentionDpConfig fields (enable_balance, batching_wait_iters, timeout_iters) are preserved; rerun the unit tests with get_inference_model and the cache_seq_interface fixtures to confirm passing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unittest/auto_deploy/singlegpu/shim/test_engine.py`:
- Around line 112-123: Add explicit return type annotations to the newly added
helper and test functions: annotate _make_cache_seq_interface to return
CachedSequenceInterface, and annotate the tests
test_ad_engine_propagates_pyexecutor_scheduling_config and
test_ad_engine_scheduling_config_defaults_without_ad_config to return None;
update any other nearby functions on lines referenced (e.g., the functions
around 167-168) to include explicit return types as well so the module is fully
annotated for mypy opt-in checks.
- Around line 122-185: Tests added
(test_ad_engine_propagates_pyexecutor_scheduling_config and
test_ad_engine_scheduling_config_defaults_without_ad_config) verify ADEngine
propagates scheduling/streaming knobs from an LlmArgs ad_config and falls back
to previous stub defaults when omitted; if these tests fail, update
ADEngine.__init__ (the ADEngine constructor) to copy stream_interval,
attention_dp_config, batch_wait_timeout_ms, batch_wait_timeout_iters, and
batch_wait_max_tokens_ratio from the provided ad_config LlmArgs into
engine.llm_args (or preserve existing defaults when ad_config is None), ensuring
AttentionDpConfig fields (enable_balance, batching_wait_iters, timeout_iters)
are preserved; rerun the unit tests with get_inference_model and the
cache_seq_interface fixtures to confirm passing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ea3647f-f265-4c83-bb5a-ea570f151fb4
📒 Files selected for processing (2)
tensorrt_llm/_torch/auto_deploy/shim/ad_executor.pytests/unittest/auto_deploy/singlegpu/shim/test_engine.py
galagam
left a comment
There was a problem hiding this comment.
LGTM. Thanks!
Approved in CodeRabbit Change Stack
|
/bot run |
|
PR_Github #52029 [ run ] triggered by Bot. Commit: |
|
PR_Github #52029 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52155 [ run ] triggered by Bot. Commit: |
|
PR_Github #52155 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52422 [ run ] triggered by Bot. Commit: |
|
PR_Github #52422 [ run ] completed with state
|
|
/bot run |
|
PR_Github #52526 [ run ] triggered by Bot. Commit: |
|
PR_Github #52526 [ run ] completed with state |
Signed-off-by: Taylor Yeonbok Lee <249374542+taylor-yb-lee@users.noreply.github.com>
Summary by CodeRabbit
Tests
Chores
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.