chore: add instructions to modify SLA to profile_sla doc; update component name #2167
Conversation
WalkthroughThe profiling job's Kubernetes YAML was updated to include new command-line arguments for GPU constraints and timing parameters. Corresponding documentation now explicitly instructs users to configure these SLA parameters before running the profiling job, adding a dedicated step and clarifying the workflow sequence. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
benchmarks/profiler/deploy/profile_sla_job.yaml (1)
40-51: Quote SLA numeric arguments for consistency and to avoid YAML → string coercion surprises
argsis a[]stringin the Pod spec. Supplying un-quoted scalars (3000,150,200,20) relies on the YAML → JSON → Go unmarshalling stack to coerce integers to strings. It works today but is brittle and inconsistent with the quoted GPU values above.- - --isl - - 3000 + - --isl + - "3000" - - --osl - - 150 + - --osl + - "150" - - --ttft - - 200 + - --ttft + - "200" - - --itl - - 20 + - --itl + - "20"Keeps the style uniform and removes any implicit type conversion.
docs/architecture/pre_deployment_profiling.md (1)
92-113: Replace bold text with proper headings to satisfy markdown-lint (MD036)The static analysis hints flag the use of emphasis instead of headings (
**Step 2: …**,**Step 3: …**, etc.). Converting them to level-3 headings keeps the visual hierarchy and silences the linter.-**Step 2: Set SLA target** +### Step 2: Set SLA target ... -**Step 3: Run profiling (required)** +### Step 3: Run profiling (required)Do the same for the subsequent Step 4 block.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
benchmarks/profiler/deploy/profile_sla_job.yaml(1 hunks)docs/architecture/pre_deployment_profiling.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/architecture/pre_deployment_profiling.md
92-92: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
124-124: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
docs/architecture/pre_deployment_profiling.md (1)
85-88: LGTM – early export ofDOCKER_IMAGE/DGD_CONFIG_FILEimproves the flowPlacing the exports right after the build step avoids users forgetting to set them later. No issues spotted here.
tedzhouhk
requested review from
a team,
GuanLuo,
alec-flowers,
biswapanda,
grahamking,
ishandhanani,
jthomson04,
kkranen,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25 and
tmonty12
as code owners
tedzhouhk
changed the title
Summary by CodeRabbit
Documentation
Chores