fix: [DYN-2883] Triton backend directory default#8697
Conversation
0de40c9 to
aaec7c9
Compare
|
/ok to test aaec7c9 |
WalkthroughIntroduces a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/backends/tritonserver/launch/identity.sh (1)
1-130: Consider adopting shared launch-script patterns for consistency.The script does not follow the launch-script conventions outlined in the coding guidelines. Consider refactoring to:
- Source
../../../common/launch_utils.shand../../../common/gpu_utils.shfor shared utilities- Background the worker process (lines 123-129) and use
wait_any_exitinstead of foreground execution- Replace the custom banner (lines 98-104) with
print_launch_banner- Replace sleep-based readiness (line 119) with a proper readiness check
These changes would align the script with the repository's standard launch patterns used by other backends (vLLM, TRT-LLM, SGLang).
As per coding guidelines: "For any script under examples/backends/*/launch (including identity.sh), ensure it follows the shared launch-script conventions: source ../../../common/gpu_utils.sh and ../../../common/launch_utils.sh instead of reimplementing process-management; background all launched processes and use wait_any_exit (not bare wait or foreground execution); emit a consistent startup banner via print_launch_banner; avoid relying on sleep-based readiness."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/backends/tritonserver/launch/identity.sh` around lines 1 - 130, Refactor the script to follow shared launch-script conventions by sourcing ../../../common/launch_utils.sh and ../../../common/gpu_utils.sh at the top, replace the custom banner block with a call to print_launch_banner, background the Triton worker (start tritonworker.py with & and capture its PID instead of running it in the foreground), use wait_any_exit to wait for any background process to exit (instead of relying on trap + foreground wait), and replace the fixed sleep readiness delay with the repository standard readiness check helper (from launch_utils.sh) to detect when the frontend is ready before starting the worker; update references to FRONTEND_PID/TRITON_PID accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/backends/tritonserver/launch/identity.sh`:
- Around line 1-130: Refactor the script to follow shared launch-script
conventions by sourcing ../../../common/launch_utils.sh and
../../../common/gpu_utils.sh at the top, replace the custom banner block with a
call to print_launch_banner, background the Triton worker (start tritonworker.py
with & and capture its PID instead of running it in the foreground), use
wait_any_exit to wait for any background process to exit (instead of relying on
trap + foreground wait), and replace the fixed sleep readiness delay with the
repository standard readiness check helper (from launch_utils.sh) to detect when
the frontend is ready before starting the worker; update references to
FRONTEND_PID/TRITON_PID accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 675b9bb8-57bb-49b9-9388-5b60ab1ec810
📒 Files selected for processing (3)
examples/backends/tritonserver/Dockerfileexamples/backends/tritonserver/README.mdexamples/backends/tritonserver/launch/identity.sh
Signed-off-by: Neal Vaidya <nealv@nvidia.com>
aaec7c9 to
3acea94
Compare
Summary
BACKEND_DIR=/opt/tritonserver/backendsin the Triton worker container image.launch/identity.shhonorBACKEND_DIRfrom the environment while preserving the local source-build fallback.Fixes DYN-2883
Root Cause
The container image copies prebuilt Triton backends into
/opt/tritonserver/backends, but the launch script always defaulted toexamples/backends/tritonserver/backends, which is only populated by the localmake allworkflow. The container quick-start therefore exited before the worker could start.Validation
bash -n examples/backends/tritonserver/launch/identity.shBACKEND_DIR=/opt/tritonserver/backends bash examples/backends/tritonserver/launch/identity.sh --helpBACKEND_DIR=/tmp/dynamo-missing-backends bash examples/backends/tritonserver/launch/identity.shconfirmed the override path is used for validationgit show --check --stat --oneline HEADDid not run the full Docker/GPU launch locally.
Summary by CodeRabbit
Documentation
BACKEND_DIRenvironment variable to specify Tritonserver backends directory location.Chores
BACKEND_DIRenvironment variable with default path/opt/tritonserver/backends.