This repository was archived by the owner on Apr 20, 2026. It is now read-only.
Add gpt-oss-120b FP4 disaggregated benchmark recipes for GB200#189
Draft
nlevin-ui wants to merge 1 commit into
Draft
Add gpt-oss-120b FP4 disaggregated benchmark recipes for GB200#189nlevin-ui wants to merge 1 commit into
nlevin-ui wants to merge 1 commit into
Conversation
Add 11 srt-slurm recipes for gpt-oss-120b FP4 on GB200 NVL72 (ptyche), translated from SemiAnalysis InferenceX configurations. Includes both TP and DEP (disaggregated expert parallel) configs across 1K/1K and 8K/1K input/output sequence lengths. Recipes: - 1k1k: 6 configs (2 TP, 4 DEP) covering 3-13 GPU topologies - 8k1k: 5 configs (4 TP, 1 DEP) covering 4-9 GPU topologies DEP recipes include MNNVL-optimized MoE all-to-all env vars (TRTLLM_MOE_ALLTOALL_BACKEND, TRTLLM_FORCE_ALLTOALL_METHOD, TRTLLM_MOE_A2A_WORKSPACE_MB) on decode workers, matching the original SA benchmark scripts. Also includes backwards-compatible topology changes: - Support dedicated_node allocation via prefill_nodes/decode_nodes kwargs (default 0 = existing packing behavior) to prevent ZMQ port collisions when partial-GPU workers land on the same node - Pass --max-num-tokens/--max-batch-size/--max-seq-len as CLI args to trtllm worker (YAML-only may not be respected per upstream) Made-with: Cursor
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/srtctl/core/topology.py (1)
252-289:⚠️ Potential issue | 🟡 MinorFail fast with explicit node-capacity checks before indexing
available_nodes.In dedicated mode, undersized node allocations currently surface as raw
IndexErrorfrom list indexing. Please raise a contextualValueErrorso misconfigurations are diagnosable.Suggested fix
def allocate_workers_simple( mode: WorkerMode, count: int, gpus_per_worker: int, dedicated_node: bool = False, ) -> list[Endpoint]: nonlocal node_idx, gpu_offset - result = [] + result: list[Endpoint] = [] nodes_per_worker = (gpus_per_worker + gpus_per_node - 1) // gpus_per_node for i in range(count): + if gpus_per_worker >= gpus_per_node and (node_idx + nodes_per_worker) > len(available_nodes): + raise ValueError( + f"Insufficient nodes for {mode}[{i}]: need {nodes_per_worker}, " + f"available {len(available_nodes) - node_idx}" + ) if nodes_per_worker >= 1 and gpus_per_worker >= gpus_per_node: # Multi-node or full-node worker worker_nodes = tuple(available_nodes[node_idx + j] for j in range(nodes_per_worker)) node_idx += nodes_per_worker @@ else: # Partial node worker if dedicated_node: # Force each worker onto a fresh node if gpu_offset > 0: node_idx += 1 gpu_offset = 0 @@ - worker_node = available_nodes[node_idx] + if node_idx >= len(available_nodes): + raise ValueError( + f"Insufficient nodes for {mode}[{i}]: no node available at index {node_idx}" + ) + worker_node = available_nodes[node_idx] gpu_indices = frozenset(range(gpu_offset, gpu_offset + gpus_per_worker))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/core/topology.py` around lines 252 - 289, In allocate_workers_simple, indexing available_nodes can raise IndexError when node_idx or node_idx + nodes_per_worker exceeds available_nodes length; add explicit capacity checks before any access (both in the multi-node branch where you construct worker_nodes = tuple(available_nodes[node_idx + j] ...) and in the partial-node/dedicated_node path before reading available_nodes[node_idx]) and raise a clear ValueError with context (include mode, count, node_idx, required nodes, and len(available_nodes)) to fail fast on misconfiguration; also check before advancing node_idx (e.g., when incrementing for dedicated_node or when gpu_offset + gpus_per_worker exceeds gpus_per_node) that there remain nodes available and raise the same contextual ValueError if not.
🧹 Nitpick comments (3)
recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen1_dep4_batch512_eplb0_mtp0.yaml (1)
22-90: Consider reducing config drift with shared recipe primitives.The prefill/decode env and TRTLLM blocks are heavily duplicated across the new recipes. A shared base (or YAML anchors) would make future tuning safer and cheaper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen1_dep4_batch512_eplb0_mtp0.yaml` around lines 22 - 90, The prefill_environment/decode_environment and duplicated trtllm_config blocks (prefill/decode) are repeated; refactor by extracting shared definitions (e.g., a top-level common_environment and common_trtllm_config) or use YAML anchors/aliases to centralize keys like TRTLLM_SERVER_DISABLE_GC, TRTLLM_WORKER_DISABLE_GC, OVERRIDE_QUANT_ALGO, and the trtllm_config.prefill/decode common fields (cuda_graph_config, kv_cache_config, cache_transceiver_config, print_iter_log, num_postprocess_workers) and then reference them from the specific prefill_environment, decode_environment, trtllm_config.prefill and trtllm_config.decode sections to remove duplication and make future tuning safer.recipes/trtllm/gptoss-fp4/8k1k/stp/ctx1_gen1_tp4_batch128_eplb0_mtp0.yaml (1)
14-17: Pingpus_per_decodeexplicitly for TP4 decode.This recipe encodes TP4 in decode config, so it’s safer to make decode GPU allocation explicit in
resourcesrather than relying on implicit defaults.Proposed patch
resources: gpu_type: "gb200" prefill_nodes: 1 prefill_workers: 1 gpus_per_prefill: 1 decode_workers: 1 decode_nodes: 1 + gpus_per_decode: 4 gpus_per_node: 4Also applies to: 64-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@recipes/trtllm/gptoss-fp4/8k1k/stp/ctx1_gen1_tp4_batch128_eplb0_mtp0.yaml` around lines 14 - 17, The decode config currently encodes TP4 via gpus_per_node: 4 but doesn't explicitly set gpus_per_decode; add a gpus_per_decode: 4 entry under the resources/decode allocation to explicitly pin 4 GPUs for TP4 decoding (and mirror the same change for the other decode block referenced around lines 64-65), so the decode allocation no longer relies on implicit defaults.recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen3_dep4_batch1024_eplb0_mtp0.yaml (1)
14-17: Consider addinggpus_per_decode: 4for clarity.The implicit calculation
(decode_nodes * gpus_per_node) // decode_workers = (3 * 4) // 3 = 4is correct and aligns with the decode TP/EP=4 config. However, making it explicit improves readability and prevents accidental misconfiguration ifdecode_nodesordecode_workersare later adjusted.Suggested improvement
resources: gpu_type: "gb200" prefill_nodes: 1 prefill_workers: 1 gpus_per_prefill: 1 decode_workers: 3 decode_nodes: 3 + gpus_per_decode: 4 gpus_per_node: 4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen3_dep4_batch1024_eplb0_mtp0.yaml` around lines 14 - 17, Add an explicit gpus_per_decode key to the YAML to document the decode parallelism instead of relying on the implicit computation; update the recipe near the existing decode_workers, decode_nodes, and gpus_per_node entries (where decode_workers: 3, decode_nodes: 3, gpus_per_node: 4 are defined) to include gpus_per_decode: 4 so the decode TP/EP configuration is clear and resilient to future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/srtctl/core/topology.py`:
- Around line 277-281: Collapse the nested conditional by combining checks for
dedicated_node and gpu_offset > 0 into a single if statement so the block
becomes a single conditional that increments node_idx and resets gpu_offset;
locate the code in topology.py where dedicated_node, gpu_offset and node_idx are
manipulated (the block containing "if dedicated_node:") and replace the inner
nested if with a single combined condition (if dedicated_node and gpu_offset >
0) performing node_idx += 1 and gpu_offset = 0.
---
Outside diff comments:
In `@src/srtctl/core/topology.py`:
- Around line 252-289: In allocate_workers_simple, indexing available_nodes can
raise IndexError when node_idx or node_idx + nodes_per_worker exceeds
available_nodes length; add explicit capacity checks before any access (both in
the multi-node branch where you construct worker_nodes =
tuple(available_nodes[node_idx + j] ...) and in the partial-node/dedicated_node
path before reading available_nodes[node_idx]) and raise a clear ValueError with
context (include mode, count, node_idx, required nodes, and
len(available_nodes)) to fail fast on misconfiguration; also check before
advancing node_idx (e.g., when incrementing for dedicated_node or when
gpu_offset + gpus_per_worker exceeds gpus_per_node) that there remain nodes
available and raise the same contextual ValueError if not.
---
Nitpick comments:
In `@recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen1_dep4_batch512_eplb0_mtp0.yaml`:
- Around line 22-90: The prefill_environment/decode_environment and duplicated
trtllm_config blocks (prefill/decode) are repeated; refactor by extracting
shared definitions (e.g., a top-level common_environment and
common_trtllm_config) or use YAML anchors/aliases to centralize keys like
TRTLLM_SERVER_DISABLE_GC, TRTLLM_WORKER_DISABLE_GC, OVERRIDE_QUANT_ALGO, and the
trtllm_config.prefill/decode common fields (cuda_graph_config, kv_cache_config,
cache_transceiver_config, print_iter_log, num_postprocess_workers) and then
reference them from the specific prefill_environment, decode_environment,
trtllm_config.prefill and trtllm_config.decode sections to remove duplication
and make future tuning safer.
In `@recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen3_dep4_batch1024_eplb0_mtp0.yaml`:
- Around line 14-17: Add an explicit gpus_per_decode key to the YAML to document
the decode parallelism instead of relying on the implicit computation; update
the recipe near the existing decode_workers, decode_nodes, and gpus_per_node
entries (where decode_workers: 3, decode_nodes: 3, gpus_per_node: 4 are defined)
to include gpus_per_decode: 4 so the decode TP/EP configuration is clear and
resilient to future changes.
In `@recipes/trtllm/gptoss-fp4/8k1k/stp/ctx1_gen1_tp4_batch128_eplb0_mtp0.yaml`:
- Around line 14-17: The decode config currently encodes TP4 via gpus_per_node:
4 but doesn't explicitly set gpus_per_decode; add a gpus_per_decode: 4 entry
under the resources/decode allocation to explicitly pin 4 GPUs for TP4 decoding
(and mirror the same change for the other decode block referenced around lines
64-65), so the decode allocation no longer relies on implicit defaults.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
recipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen1_dep2_batch1536_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen1_dep4_batch512_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen1_tp4_batch256_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen2_dep2_batch1536_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen3_dep4_batch1024_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/1k1k/stp/ctx1_gen4_tp2_batch32_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/8k1k/stp/ctx1_gen1_tp4_batch128_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/8k1k/stp/ctx1_gen1_tp8_batch4_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/8k1k/stp/ctx2_gen1_dep2_batch512_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/8k1k/stp/ctx2_gen1_tp2_batch512_eplb0_mtp0.yamlrecipes/trtllm/gptoss-fp4/8k1k/stp/ctx2_gen1_tp4_batch1024_eplb0_mtp0.yamlsrc/srtctl/backends/base.pysrc/srtctl/backends/sglang.pysrc/srtctl/backends/trtllm.pysrc/srtctl/backends/vllm.pysrc/srtctl/cli/do_sweep.pysrc/srtctl/core/topology.py
Comment on lines
+277
to
+281
| if dedicated_node: | ||
| # Force each worker onto a fresh node | ||
| if gpu_offset > 0: | ||
| node_idx += 1 | ||
| gpu_offset = 0 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -300 src/srtctl/core/topology.py | tail -30Repository: ishandhanani/srt-slurm
Length of output: 1168
🏁 Script executed:
rg -nUP 'if dedicated_node:' src/srtctl/core/topology.py -A 5Repository: ishandhanani/srt-slurm
Length of output: 577
🏁 Script executed:
cat pyproject.toml | grep -A 20 '\[tool.ruff'Repository: ishandhanani/srt-slurm
Length of output: 1320
🏁 Script executed:
cd src/srtctl/core && rg -n 'if dedicated_node:' topology.py -A 2 | head -20Repository: ishandhanani/srt-slurm
Length of output: 356
🏁 Script executed:
# Run ruff check to see lint violations in the file
rg 'name = "ruff"' . --type tomlRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Try running ruff lint on topology.py to check for SIM102 violations
python -m pip install ruff -q 2>/dev/null && python -m ruff check src/srtctl/core/topology.py --select SIM 2>/dev/null || echo "Checking with available ruff"Repository: ishandhanani/srt-slurm
Length of output: 880
Collapse nested if at line 277 to resolve Ruff SIM102 violation.
The SIM102 lint failure is confirmed. Combine the conditions using and as suggested below:
Suggested fix
- if dedicated_node:
- # Force each worker onto a fresh node
- if gpu_offset > 0:
- node_idx += 1
- gpu_offset = 0
+ if dedicated_node and gpu_offset > 0:
+ # Force each worker onto a fresh node
+ node_idx += 1
+ gpu_offset = 0🧰 Tools
🪛 GitHub Actions: CI
[error] 277-279: SIM102 Use a single if statement instead of nested if statements. Combine if statements using and.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/srtctl/core/topology.py` around lines 277 - 281, Collapse the nested
conditional by combining checks for dedicated_node and gpu_offset > 0 into a
single if statement so the block becomes a single conditional that increments
node_idx and resets gpu_offset; locate the code in topology.py where
dedicated_node, gpu_offset and node_idx are manipulated (the block containing
"if dedicated_node:") and replace the inner nested if with a single combined
condition (if dedicated_node and gpu_offset > 0) performing node_idx += 1 and
gpu_offset = 0.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Add 11 srt-slurm recipes for gpt-oss-120b FP4 on GB200 NVL72 (ptyche), translated from SemiAnalysis InferenceX configurations. Includes both TP and DEP (disaggregated expert parallel) configs across 1K/1K and 8K/1K input/output sequence lengths.
Recipes:
DEP recipes include MNNVL-optimized MoE all-to-all env vars (TRTLLM_MOE_ALLTOALL_BACKEND, TRTLLM_FORCE_ALLTOALL_METHOD, TRTLLM_MOE_A2A_WORKSPACE_MB) on decode workers, matching the original SA benchmark scripts.
Also includes backwards-compatible topology changes:
Made-with: Cursor
Summary by CodeRabbit
Release Notes