Kimik3 madengine integration

[Cemberk]

built on top of existing kimi k3 pr for madengine enablement and testing

[Copilot]

Pull request overview

Adds Kimi-K3 MI300X support by integrating a new colocated multi-node vLLM launcher alongside updates to the existing disaggregated (prefill/decode) vLLM harness, plus reporting/benchmarking and documentation updates to make results consumable by madengine.

Changes:

• Introduces a 2-node colocated vLLM SLURM+container launcher (vllm_multinode) with shared-model recipe support and shutdown coordination.
• Extends the disaggregated launcher path to support TP_SIZE>1, multi-node pool host advertisement for MoRIIO, and NIAH benchmarking/reporting.
• Adds Kimi-K3 model recipes/cards, MI300X-specific docs/site-config templates, and a pinned Dockerfile for the K3 MI300X image.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
scripts/vllm_multinode/serve_colocated.sh	Per-node in-container entrypoint for colocated multi-node vLLM serving + benchmarking.
scripts/vllm_multinode/run_multinode.slurm	SLURM launcher that discovers nodes/IPs, runs per-node containers, and publishes perf CSV.
scripts/vllm_dissag/vllm_disagg.sh	Adds TP-aware topology math and peer-pool host export for KV connector.
scripts/vllm_dissag/run_xPyD_models.slurm	Adds Kimi-K3 support, TP_SIZE plumbing, NIAH option, safer RDMA mounts, perf CSV publish.
scripts/vllm_dissag/parse_to_csv.py	Adds narrow-schema perf CSV mode, NNODES-aware metadata, and NIAH parsing/output.
scripts/vllm_dissag/models.yaml	Adds Kimi-K3 MI300X serving recipe env + flags (MXFP4/int4 MoE path).
scripts/vllm_dissag/connectors/moriio.sh	Adds multi-node pod host list to KV config; TP-aware api-server-count and tp flag.
scripts/vllm_dissag/benchmark_niah.sh	Uses NIAH_MODEL when set; emits perf.csv for NIAH via parse_to_csv.py.
scripts/vllm_dissag/benchmark_niah.py	Raises default max_tokens, records finish_reason, and prints parseable result lines.
models.json	Adds Kimi-K3 MI300X disagg + colocated model-card entries with multiple_results CSV.
docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile	New pinned build for Kimi-K3 MI300X (MoRI/AITER/vLLM/router + graft step).
benchmark/kimi_k3/README.md	Notes gfx942 requires multi-node sharding and points to MI300X recipes.
benchmark/kimi_k3/mi300x/slurm-config.disagg.json	Site-config template for 4-node disaggregated MI300X runs.
benchmark/kimi_k3/mi300x/slurm-config.colocated.json	Site-config template for 2-node colocated MI300X runs.
benchmark/kimi_k3/mi300x/README.md	Full MI300X recipe documentation, tradeoffs, and madengine integration notes.
.gitignore	Ensures MI300X site-config templates are not ignored.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new run_multinode.slurm launcher has verified set -u-triggered failure paths (optional env expansion and hardcoded barrier port) that can prevent runs from starting or make barrier cleanup inconsistent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

scripts/vllm_multinode/run_multinode.slurm:158

• The barrier port is configurable via CONTAINER_BARRIER_PORT (and serve_colocated.sh honors it), but this fuser call always kills 2223. If the port is overridden, stale listeners on the configured port won’t be cleared and the barrier can hang/fail. Use the same ${CONTAINER_BARRIER_PORT:-2223} default here.

fuser -k 2223/tcp 2>/dev/null || true

• Files reviewed: 15/16 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are a few correctness/robustness blockers in the new launchers (per-pool host list logic, unsafe eval on args, and a hard-coded barrier port/kill behavior) that should be addressed before relying on these scripts in CI or shared clusters.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

scripts/vllm_dissag/vllm_disagg.sh:137

• PREFILL_POD_HOSTS / DECODE_POD_HOSTS are currently populated whenever either pool spans >1 node. This causes single-node pools (e.g., xP=2,yD=1 or xP=1,yD=2) to still export a non-empty peer list for the 1-node side, which defeats the stated goal of keeping the emitted KV config byte-identical for single-node pools (and can change connector behavior unexpectedly). Populate each host list only when that specific pool spans >1 node.
  scripts/vllm_multinode/run_multinode.slurm:79
• run_multinode.slurm supports BENCHMARK_SCRIPT=long_context and BENCHMARK_SCRIPT=niah, but the early REQUIRED_FILES check only validates benchmark_xPyD.sh. This means a missing long-context or NIAH script will fail later (inside the container) with a less actionable error. Validate all referenced benchmark scripts up front.
  scripts/vllm_multinode/run_multinode.slurm:247
• If CONTAINER_BARRIER_PORT is set in the launcher (e.g., to avoid collisions), it is not currently passed into the container environment, so serve_colocated.sh will still default to 2223 inside the container. Pass CONTAINER_BARRIER_PORT via docker run -e so the host-side choice is honored in-container.

scripts/vllm_multinode/serve_colocated.sh:113

• COLOCATED_EXTRA_ARGS is expanded with eval, which allows arbitrary command execution if the environment value is ever influenced by untrusted input (e.g., CI, wrapper scripts, or user-provided model cards). This can be avoided by using a non-eval parser (e.g., Python shlex.split) to safely produce argv tokens.

if [[ -n "${COLOCATED_EXTRA_ARGS:-}" ]]; then
    eval "extra_args=(${COLOCATED_EXTRA_ARGS})"
    serve_args+=("${extra_args[@]}")
fi

scripts/vllm_multinode/run_multinode.slurm:160

• The container barrier port is hard-coded to 2223 and the script unconditionally runs fuser -k 2223/tcp. This can kill unrelated processes on non-exclusive nodes and also makes concurrent runs on the same nodes fragile due to port collisions. Consider deriving a job-unique barrier port from SLURM_JOB_ID and using that for both the fuser cleanup and the in-container barrier.

srun --nodelist="$SELECTED_NODELIST" bash -c '
echo "Rank $SLURM_PROCID on $(hostname)"
docker rm -f $DOCKER_CONT_NAME 2>/dev/null || true
fuser -k 2223/tcp 2>/dev/null || true
sleep 2
docker pull $DOCKER_IMAGE_NAME 2>/dev/null || true

• Files reviewed: 15/16 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical launch, orchestration, and benchmark-reporting issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (10)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/vllm_dissag/connectors/moriio.sh:278

• The wide-EP command expands model_args at both lines 275 and 278, so every model-specific flag is passed twice. This makes the generated argv harder to reason about and can cause last-occurrence-wins behavior if a recipe later contains a non-idempotent option; keep a single expansion here.

docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile:83

• The Dockerfile says every source is pinned to immutable commits, but the default AITER donor is a mutable registry tag. Rebuilding this image can silently graft different Python packages even when all repository SHAs are unchanged, defeating the reproducibility guarantee. Pin this FROM source by digest (or record and verify the expected image digest).

ARG PROVEN_K3_IMAGE=amdsiloai/vllm:kimi-k3-mi325x-release-v2

docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile:291

• The optional WITH_NIXL=1 build is also not reproducible despite the pinning claim: rocSHMEM checks out the moving develop branch and DeepEP checks out its default branch with no ref at all. A future opt-in build can therefore change independently of this PR; pin both repositories to immutable commits (and record the refs in versions.txt).

        cd rocm-systems && git sparse-checkout set --cone projects/rocshmem && git checkout develop && \
        mkdir -p /tmp/rocshmem-build && cd /tmp/rocshmem-build && \
        /tmp/rocm-systems/projects/rocshmem/scripts/build_configs/all_backends \
          -DUSE_EXTERNAL_MPI=OFF -DGPU_TARGETS="${GFX_COMPILATION_ARCH}" && \
      cd /tmp && git clone https://github.com/ROCm/DeepEP.git && cd DeepEP && \

scripts/sglang_disagg/run_xPyD_models.slurm:263

• cluster_resolve_model_path can select a site-specific MODEL_DIR or MODEL_DIR_CANDIDATES, but the container below only mounts /shared_inference and /mnt/m2m_nobackup. With a custom resolved root, the host-side probe succeeds while every container sees a nonexistent MODEL_PATH, so model loading fails; mount the resolved model root (or reject paths outside the mounted roots) before starting the ranks.

cluster_resolve_model_path || exit 1

scripts/vllm_dissag/benchmark_niah.sh:44

• The wrapper still injects NIAH_MAXTOK=2048 when the variable is unset, so the new Python default of 8192 is never used in launcher runs. The Kimi-K3 cards do not set NIAH_MAXTOK, leaving reasoning traces truncated and producing the false retrieval misses this change was meant to diagnose. Align this wrapper default with the Python default (or set it explicitly on the K3 cards).

NIAH_MODEL="${NIAH_MODEL:-${MODEL_PATH}}" \
NIAH_WORDS="${NIAH_WORDS:-2000,8000,20000,35000}" \
NIAH_SEEDS="${NIAH_SEEDS:-0,1,2}" \
NIAH_MAXTOK="${NIAH_MAXTOK:-2048}" \

scripts/vllm_dissag/run_xPyD_models.slurm:503

• This call has the same separate-shell problem: the srun ... bash -c body has not sourced common/cluster.sh, so cluster_mark_dir is undefined and the command fails on every node instead of writing cache metadata. Source or inline the helper before calling it.

    cluster_mark_dir "$_JIT_CACHE_HOST" "JIT/Triton/AITER kernel cache for one docker image; rebuilt on demand"

scripts/vllm_dissag/run_xPyD_models.slurm:263

• cluster_resolve_model_path can select a site-specific MODEL_DIR or MODEL_DIR_CANDIDATES, but the container below only mounts /shared_inference and /mnt/m2m_nobackup. With a custom resolved root, the host-side probe succeeds while every container sees a nonexistent MODEL_PATH, so model loading fails; mount the resolved model root (or reject paths outside the mounted roots) before starting the ranks.

NIXL_REPO_DIR=$(pwd)

scripts/vllm_multinode/run_multinode.slurm:285

• The site configuration allows MODEL_DIR/MODEL_DIR_CANDIDATES outside the two default roots, and the host-side probe can successfully select such a path. The container then mounts only fixed /shared_inference plus NVME_ROOT, so an explicit model under another root is invisible inside Docker and startup fails; bind the resolved model parent (or the configured shared roots) as well.

    -v /shared_inference:/shared_inference \

scripts/vllm_multinode/run_multinode.slurm:256

• cluster_mark_dir is defined only in the batch shell that sourced common/cluster.sh; this command runs in the separate srun bash -c shell and therefore returns command not found on every node. Because the inner shell is not set -e, the job continues but the promised CACHEDIR.TAG/README.ci metadata is never created. Source the helper inside this shell or inline the marker logic.

    cluster_mark_dir "$_JIT_CACHE_HOST" "JIT/Triton/AITER kernel cache for one docker image; rebuilt on demand"

scripts/vllm_multinode/serve_colocated.sh:94

• This comment says an unset timeout falls back to PyTorch's 1800-second default, but both argv entries explicitly use :-7200 (and cluster.sh also defaults them to 7200). Operators following the comment will underestimate the launcher’s actual wait; document the 7200-second entrypoint default instead.

    # Separate field from the one above: --distributed-timeout-seconds only
    # reaches the device/NCCL groups, while the startup barrier that has been
    # killing colocated runs here is on the gloo CPU group and takes its deadline
    # from this one. Unset, both fall back to PyTorch's stock 1800s.
    --cpu-distributed-timeout-seconds "${CPU_DISTRIBUTED_TIMEOUT_SECONDS:-7200}"

• Files reviewed: 27/29 changed files
• Comments generated: 15
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical launcher issues and unresolved benchmark/reporting problems block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (16)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/vllm_dissag/connectors/moriio.sh:278

• model_args is appended at line 275 and again at line 278. On the wide-EP Kimi path this repeats every model-specific flag, including the quantization config and maximum model length; remove one occurrence so the generated vLLM command has a single model-argument set.
  scripts/vllm_multinode/serve_colocated.sh:94
• CPU_DISTRIBUTED_TIMEOUT_SECONDS is defaulted to 7200 on line 94, so the statement that unset timeouts fall back to PyTorch's stock 1800s is false. Document the 7200-second launcher default or actually leave the variable unset; otherwise the timeout behavior is misleading during startup failures.

benchmark/kimi_k3/mi300x/README.md:278

• The implementation does not currently satisfy this reporting contract: partial seed failures are excluded from the mean and still written with status=SUCCESS, while all-seed failures emit NO-RESULT and produce no row. A timeout regression can therefore be either mislabeled success or silently omitted rather than a FAILURE row with performance 0.

whose request errors is recorded as a `FAILURE` row with performance 0, so a
pass→crash regression shows up instead of silently disappearing.

docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile:291

• The Dockerfile's PINNING section says every source is immutable, but the optional WITH_NIXL=1 path checks out rocm-systems at the moving develop branch and clones DeepEP without any checkout. That build mode is therefore not reproducible and can change or break independently of the recorded pins; add immutable refs and record them in the image metadata.

      cd /tmp && git clone --no-checkout --filter=blob:none https://github.com/ROCm/rocm-systems.git && \
        cd rocm-systems && git sparse-checkout set --cone projects/rocshmem && git checkout develop && \
        mkdir -p /tmp/rocshmem-build && cd /tmp/rocshmem-build && \
        /tmp/rocm-systems/projects/rocshmem/scripts/build_configs/all_backends \
          -DUSE_EXTERNAL_MPI=OFF -DGPU_TARGETS="${GFX_COMPILATION_ARCH}" && \
      cd /tmp && git clone https://github.com/ROCm/DeepEP.git && cd DeepEP && \

scripts/vllm_dissag/benchmark_niah.py:32

• The launcher still invokes this script with NIAH_MAXTOK="${NIAH_MAXTOK:-2048}" in benchmark_niah.sh, so this new 8192 default is overridden for every normal harness run unless the model card sets the variable (the Kimi entries do not). Raise the wrapper default or pass 8192 from the Kimi recipes so the intended reasoning headroom is actually used.

MAXTOK = int(os.environ.get("NIAH_MAXTOK", "8192"))

scripts/vllm_dissag/benchmark_niah.py:142

• When every scored request for a context times out, this branch emits only NO-RESULT; parse_to_csv.parse_niah_log recognizes only mean summary lines, so that context disappears from perf.csv instead of becoming the documented FAILURE/zero row. Emit a parseable failure record or update the parser to consume this case.

        if not vals:
            print("  words=%6d  NO-RESULT (%d/%d timed out or errored — likely cold compile; "
                  "raise NIAH_TIMEOUT or keep NIAH_WARMUP=1)" % (n, n_to, len(scored)), flush=True)
            continue

scripts/vllm_dissag/benchmark_niah.py:149

• The timeout count is written only as a human-readable suffix, while the parser matches the summary prefix and drops that suffix; consequently a context with partial timeouts is emitted with a reduced mean and SUCCESS rather than the documented failure state. Preserve the timeout count in the parsed result and map it to the intended failure/performance behavior.

        extra = ("  [%d timeout/err excluded]" % n_to) if n_to else ""
        if n_trunc:
            # A truncated answer scores low for lack of room, not lack of retrieval.
            extra += "  (TRUNCATED in %d/%d: raise NIAH_MAXTOK)" % (n_trunc, len(vals))

scripts/vllm_dissag/benchmark_niah.sh:44

• The Python NIAH default was raised to 8192 for reasoning models, but this wrapper still supplies 2048 whenever the caller does not set NIAH_MAXTOK. All three new colocated Kimi cards omit that variable, so their requests retain the truncating limit and the intended fix is bypassed.

NIAH_MAXTOK="${NIAH_MAXTOK:-2048}" \

scripts/vllm_dissag/benchmark_niah.sh:55

• Now that this wrapper invokes the NIAH parser, a context size whose every seed times out still emits only NO-RESULT; parse_to_csv.py matches no summary row, so that context silently disappears instead of becoming the documented FAILURE row with performance 0. Emit or parse an explicit failure record for all-error contexts.

python3 "${DIR}/parse_to_csv.py" "${LOG}" --niah \
        --perf-csv "/run_logs/${SLURM_JOB_ID}/perf.csv" \
        --model-name "${MODEL_NAME}" 2>&1 | tee -a "${LOG}"

scripts/vllm_dissag/parse_to_csv.py:359

• The --niah path is not actually narrow: it calls save_niah_perf_csv, which builds the legacy full-schema header and injects _get_run_metadata(). This contradicts the stated multiple_results contract that NIAH rows are workload-only and madengine supplies topology/image metadata, so colocated rows can bypass the authoritative metadata merge.

    parser.add_argument('--narrow', action='store_true',
                        help='Emit a narrow results CSV (model/performance/metric[/status]) for a model card '
                             'declaring multiple_results, letting madengine supply the run metadata. '
                             'Ignored with --niah, which is always narrow.')

scripts/vllm_dissag/run_xPyD_models.slurm:111

• Kimi-K3 is only added to MORI_EP_VALID_MODELS, but validate_model_name checks VALID_MODELS first (line 144). The new pyt_vllm_disagg_mori_kimi-k3 card therefore exits as invalid before it can reach the allowed MoRI wideEP path; add Kimi-K3 to the primary allowlist as well.

    "Kimi-K3" \

scripts/vllm_multinode/run_multinode.slurm:113

• long_context is accepted here, but benchmark_long_context.sh only writes benchmark logs and never creates /run_logs/$SLURM_JOB_ID/perf.csv; the unconditional publish step below then exits 1. Either add CSV generation for this mode or reject it instead of advertising it as a valid selector.

    sweep)        BENCHMARK_SCRIPT_FILE="benchmark_xPyD.sh" ;;
    long_context) BENCHMARK_SCRIPT_FILE="benchmark_long_context.sh" ;;
    niah)         BENCHMARK_SCRIPT_FILE="benchmark_niah.sh" ;;

scripts/vllm_multinode/run_multinode.slurm:86

• cluster.sh initializes MODEL_NAME to the literal None before this expansion, so ${MODEL_NAME:?} never fails when the required variable is omitted; the launcher probes for a model named None instead of reporting the documented missing-env error. Check for empty/None explicitly or remove the sentinel before validating.

MODEL_NAME="${MODEL_NAME:?set MODEL_NAME (key into scripts/vllm_dissag/models.yaml)}"

scripts/vllm_multinode/run_multinode.slurm:334

• The container receives topology and benchmark settings here, but not LOG_WAIT_TIMEOUT_SECONDS, DISTRIBUTED_TIMEOUT_SECONDS, or CPU_DISTRIBUTED_TIMEOUT_SECONDS from cluster.sh. Site-level timeout overrides are therefore silently lost and serve_colocated.sh falls back to its internal 4000/7200-second values unless users know to list them in COLOCATED_FORWARD_ENV; forward these shared settings explicitly.

    -e TP_SIZE=$TP_SIZE \
    -e PP_SIZE=$PP_SIZE \
    -e ENABLE_EP=$ENABLE_EP \
    ${ALL2ALL_BACKEND:+-e ALL2ALL_BACKEND=$ALL2ALL_BACKEND} \

scripts/vllm_multinode/run_multinode.slurm:365

• The site configs require the CSV beside scripts/vllm_multinode, but this destination uses the caller's current working directory. A madengine wrapper can submit the script from its job directory, leaving perf_Kimi-K3.csv outside slurm.results_dir so the collector misses an otherwise valid result. Write to ${SCRIPT_DIR} instead.

_PERF_DST="$(pwd)/perf_${MODEL_NAME}.csv"

scripts/vllm_multinode/serve_colocated.sh:252

• This claim does not match save_niah_perf_csv: NIAH currently writes the full legacy metadata schema, not the narrow workload schema described here and in the Kimi README (for example, it omits benchmark, context_words, tp, pp, and ep_backend). Either emit the documented narrow fields before saying madengine supplies the metadata, or correct the documentation and contract.

#                       Only consulted on the LEGACY full-schema reporting path
#                       (BENCHMARK_SCRIPT=sweep). With BENCHMARK_SCRIPT=niah the CSV
#                       is narrow and madengine supplies these fields itself.

• Files reviewed: 27/29 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved launcher, topology, model-path, and benchmark issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (17)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/vllm_dissag/parse_to_csv.py:359

• The CLI help says NIAH output is “always narrow,” but save_niah_perf_csv writes the legacy 29-column metadata schema (fieldnames at lines 240-248). This makes the documented --niah output contract inaccurate for callers and model-card integration; either make NIAH honor the narrow schema or describe the actual full-schema output.

scripts/common/cluster.sh:92

• cluster.sh is sourced before both launchers choose their topology defaults. Exporting TP_SIZE=GPUS_PER_NODE and PP_SIZE=1 here prevents ${TP_SIZE:-1} in vllm_disagg from defaulting to TP1 (all existing wide-EP cards become TP8), and prevents ${PP_SIZE:-${NUM_NODES}} in run_multinode from defaulting to PP across nodes (a 2-node invocation fails the GPU-shape check unless PP_SIZE is explicitly set). Leave these launcher-specific variables unset here.

export TP_SIZE="${TP_SIZE:-${GPUS_PER_NODE}}"
export PP_SIZE="${PP_SIZE:-1}"

scripts/common/cluster.sh:243

• The REQUIRE_LOCAL_WEIGHTS=1 filter runs before this block, but an explicit MODEL_DIR is appended afterward without the same shared-mount check. A Kimi site config with MODEL_DIR=/shared_inference/... therefore bypasses the local-only policy and can select NFS, contradicting the launcher’s documented guarantee and risking an unfinishable multi-terabyte load. Reject or skip explicit MODEL_DIR values under SHARED_MOUNT when REQUIRE_LOCAL_WEIGHTS is enabled.

    # Append an explicit MODEL_DIR unless the list already covers it.
    if [ -n "${MODEL_DIR:-}" ]; then
        case " ${candidates} " in
            *" ${MODEL_DIR%/} "*) : ;;
            *) candidates="${candidates} ${MODEL_DIR%/}" ;;
        esac

scripts/vllm_dissag/benchmark_niah.py:32

• The new Python default of 8192 is overridden by benchmark_niah.sh, which still sets NIAH_MAXTOK=${NIAH_MAXTOK:-2048} before invoking this script. Since the Kimi cards do not set NIAH_MAXTOK, these runs still use 2048 and can truncate reasoning responses; update the wrapper default as well.

MAXTOK = int(os.environ.get("NIAH_MAXTOK", "8192"))

scripts/vllm_dissag/benchmark_niah.py:145

• When every seed times out or errors, the preceding if not vals branch prints NO-RESULT and continues, so this summary line is never emitted. parse_niah_log only parses summary lines, meaning that context size disappears instead of becoming the documented FAILURE/0 row; an all-failure run produces no perf.csv at all.

        counts = [c for c, _ in vals]
        n_trunc = sum(1 for _, f in vals if f == "length")
        mean = sum(counts) / len(counts)

scripts/vllm_dissag/benchmark_niah.sh:44

• The shell wrapper overrides benchmark_niah.py’s new 8192 default with 2048 whenever NIAH_MAXTOK is unset. The Python comments explicitly note that reasoning models need a larger budget, and all three Kimi model cards omit this variable, so their answers can be truncated and scored inaccurately at the lower limit. Keep the wrapper default aligned with the Python default or set it explicitly in the cards.

NIAH_MAXTOK="${NIAH_MAXTOK:-2048}" \

scripts/vllm_dissag/connectors/moriio.sh:275

• model_args is already appended on line 278 in this same command. Adding it here duplicates every model-specific flag at runtime, including Kimi's --quantization-config and reasoning parser, unlike the dry-run path; duplicate or conflicting single-valued arguments can then change the serve configuration or fail argument parsing. Keep the later append and remove this one.

            "${model_args[@]}" \

scripts/vllm_dissag/run_xPyD_models.slurm:111

• validate_model_name checks VALID_MODELS at lines 125-144 before the connector-specific allowlist is consulted. Kimi-K3 is added only to MORI_EP_VALID_MODELS here, so the new pyt_vllm_disagg_mori_kimi-k3 card is rejected as invalid before it can launch. Add Kimi-K3 to VALID_MODELS as well.

    "Kimi-K3" \

scripts/vllm_dissag/run_xPyD_models.slurm:715

• The existing agentic entries in scripts/vllm_dissag/models.json do not declare multiple_results, and common/benchmark_agentic.sh writes agentic artifacts/JSON rather than /run_logs/$SLURM_JOB_ID/perf.csv. This new unconditional check therefore exits 1 for those previously supported DeepSeek agentic cards even when the agentic benchmark succeeds. Restrict the CSV requirement to benchmark modes that produce it, or add an agentic results adapter.

# Publish the perf CSV where madengine collects it. benchmark_xPyD.sh /
# benchmark_niah.sh write /run_logs/$SLURM_JOB_ID/perf.csv inside the container
# (= $LOG_PATH on the host), but madengine resolves a model's `multiple_results`
# against the JOB DIRECTORY it launched us from - so copy it there under the
# declared name. Harmless for entries that do not declare multiple_results.

scripts/vllm_dissag/run_xPyD_models.slurm:294

• The new resolver accepts an explicit MODEL_DIR/MODEL_PATH, but this launcher only bind-mounts /shared_inference and /mnt/m2m_nobackup into the container. If the documented site config points at another shared or local filesystem, the host probe succeeds while the vLLM container cannot see MODEL_PATH, so the run fails at model load. Bind-mount the explicit model root or reject unsupported paths before starting Docker.

cluster_resolve_model_path || exit 1

scripts/vllm_multinode/run_multinode.slurm:113

• benchmark_long_context.sh does not invoke parse_to_csv or write /run_logs/$SLURM_JOB_ID/perf.csv, while this launcher unconditionally requires that file before succeeding. Selecting the advertised BENCHMARK_SCRIPT=long_context therefore guarantees a nonzero job even when the benchmark completes. Add a reporting path for this benchmark or remove it from the valid choices until one exists.

    long_context) BENCHMARK_SCRIPT_FILE="benchmark_long_context.sh" ;;
    niah)         BENCHMARK_SCRIPT_FILE="benchmark_niah.sh" ;;

scripts/vllm_multinode/run_multinode.slurm:140

• Both the master address and the per-node list select the first hostname -I address, but the allocated nodes are multi-homed and cluster.sh provides FABRIC_SUBNET_PREFIX specifically to select the interconnect. If the first address is the management NIC, the rendezvous and barrier addresses are unreachable from peers and startup hangs. Reuse the fabric-subnet selection and fail-fast handling used by the SGLang launcher.

MASTER_ADDR=$(srun --nodes=1 --ntasks=1 --time=00:20:00 --nodelist="$MASTER_NODE" bash -c 'hostname -I' | awk '{print $1}')

IPS=()
for NODE in $SELECTED_NODES; do
    IP=$(srun --nodes=1 --ntasks=1 --time=00:20:00 --nodelist="$NODE" bash -c 'hostname -I' | awk '{print $1}')

scripts/vllm_multinode/run_multinode.slurm:361

• The command inside the container pipes the launcher through tee without enabling pipefail. If serve_colocated.sh exits nonzero, tee can still return 0 and this rank is reported as successful to Slurm, masking startup failures. Enable set -o pipefail in the nested bash -c script before this pipeline.

          2>&1 | tee /run_logs/${SLURM_JOB_ID}/colocated_bench_NODE${SLURM_PROCID}.log

scripts/vllm_multinode/run_multinode.slurm:315

• cluster_resolve_model_path accepts an explicit MODEL_DIR/MODEL_PATH, but the container only mounts /shared_inference and the NVMe root. A site config that follows the documented “wherever the checkpoint lives” guidance and uses another filesystem will pass the host-side probe, then leave MODEL_PATH invisible inside Docker and fail model loading. Bind-mount the explicit model root (with a directory guard) or reject unsupported paths before launching.

    ${_NVME_MOUNT} \

scripts/vllm_multinode/serve_colocated.sh:258

• The default benchmark_xPyD.sh resolves its parser as $NIXL_COOKBOOK_PATH/parse_to_csv.py, but this colocated container sets neither NIXL_COOKBOOK_PATH nor a replacement. It therefore tries /parse_to_csv.py, produces no perf.csv, and the outer launcher reports failure for the default sweep. Set the variable to ${SHARED_DIR} for this invocation or make the benchmark use its own directory.

bash "${SHARED_DIR}/${BENCHMARK_SCRIPT_FILE:-benchmark_xPyD.sh}"

scripts/vllm_multinode/serve_colocated.sh:160

• This recomputes the local barrier address using the first hostname -I result instead of the rank-ordered address already passed in IPADDRS. On a multi-homed node, peers can dial the fabric address while the barrier binds the management address, so every rank waits indefinitely. Select the address for NODE_RANK from IPADDRS (or use the same fabric resolver).

host_ip=$(hostname -I | awk '{print $1}')

scripts/vllm_multinode/serve_colocated.sh:208

• When a worker’s vLLM process exits, this loop simply falls through and exits successfully without touching the shared shutdown flag or notifying the head. The head only checks its own PID, so it can continue polling readiness for the full 4000-second timeout after a worker failure, holding the allocation instead of reporting the worker error promptly. Propagate a worker failure to the head and terminate the remaining ranks.

    done
    wait "${WORKER_PID}" 2>/dev/null || true
    exit 0

• Files reviewed: 27/29 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect launcher correctness, environment propagation, reporting, and build reproducibility.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (15)

docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile:83

• This donor image is referenced by a mutable tag even though the Dockerfile claims its sources are pinned for reproducible builds. A tag move can silently change the grafted K3 AITER artifacts without any MAD source change, making benchmark results and rebuilds non-reproducible. Pin the donor image by digest (or record and verify an immutable image identifier).

ARG PROVEN_K3_IMAGE=amdsiloai/vllm:kimi-k3-mi325x-release-v2

docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile:291

• The optional WITH_NIXL=1 build checks out rocm-systems from the moving develop branch, and the following DeepEP clone has no checkout at all. That contradicts the Dockerfile's reproducibility claim and can change the transport binaries between identical image builds; pin both repositories to immutable commits (and record them in versions.txt).

      cd /tmp && git clone --no-checkout --filter=blob:none https://github.com/ROCm/rocm-systems.git && \
        cd rocm-systems && git sparse-checkout set --cone projects/rocshmem && git checkout develop && \
        mkdir -p /tmp/rocshmem-build && cd /tmp/rocshmem-build && \
        /tmp/rocm-systems/projects/rocshmem/scripts/build_configs/all_backends \
          -DUSE_EXTERNAL_MPI=OFF -DGPU_TARGETS="${GFX_COMPILATION_ARCH}" && \
      cd /tmp && git clone https://github.com/ROCm/DeepEP.git && cd DeepEP && \

docker/sglang_disagg_inference.ubuntu.amd.Dockerfile:37

• The image pins MoRI to a commit but installs whatever sglang-router is newest at build time. A later router release can change the proxy CLI/API and make the launcher incompatible while the Dockerfile remains unchanged. Pin a tested router version (or commit) alongside the other runtime dependencies.

RUN pip install --upgrade sglang-router

scripts/common/cluster.sh:99

• These shared exports preempt the launcher-specific topology defaults: vllm_dissag/run_xPyD_models.slurm falls back to TP=1, while the colocated launcher falls back to PP=NNODES. With two nodes and no explicit overrides, run_multinode.slurm preserves TP=8/PP=1 from here and fails its 8 != 16 validation; existing disaggregated jobs also silently become TP=8. Leave both variables unset in the shared site file and let each launcher set its own defaults.

export TP_SIZE="${TP_SIZE:-${GPUS_PER_NODE}}"
export PP_SIZE="${PP_SIZE:-1}"

scripts/vllm_dissag/benchmark_niah.py:32

• The Python fallback was raised to 8192, but benchmark_niah.sh always exports NIAH_MAXTOK with a 2048 default before invoking this file. Since the Kimi-K3 cards do not override NIAH_MAXTOK, normal launcher runs never use the new 8192 default; update the wrapper default or remove that override.

MAXTOK = int(os.environ.get("NIAH_MAXTOK", "8192"))

scripts/vllm_dissag/benchmark_niah.sh:55

• This new NIAH reporting call exposes two contract bugs in the existing --niah path: it writes the legacy full-schema CSV even though the Kimi-K3 cards use multiple_results/narrow reporting, and save_niah_perf_csv() hard-codes every emitted row as SUCCESS while timeout-only contexts disappear. A failed or partially failed retrieval run can therefore publish an incompatible and falsely successful perf.csv; add a status-aware narrow NIAH writer or align the cards and documentation with the legacy behavior.

python3 "${DIR}/parse_to_csv.py" "${LOG}" --niah \
        --perf-csv "/run_logs/${SLURM_JOB_ID}/perf.csv" \
        --model-name "${MODEL_NAME}" 2>&1 | tee -a "${LOG}"

scripts/vllm_dissag/connectors/moriio.sh:278

• The wideEP command expands model_args at both lines 275 and 278. Kimi-K3 recipe flags such as --reasoning-parser and --quantization-config are therefore passed twice to every live wideEP server, even though the dry-run branch emits them once. Remove the second expansion.

            "${model_args[@]}" \

scripts/vllm_dissag/parse_to_csv.py:288

• If every seed times out, benchmark_niah.py emits NO-RESULT and parse_niah_log produces an empty result for that context, so this early return writes no CSV row at all. The Kimi-K3 documentation promises a FAILURE row with performance 0 for this case; preserve the failed context through CSV generation instead of silently dropping it.

    if not results:
        print("No results to save to perf.csv.")
        return

scripts/vllm_dissag/run_xPyD_models.slurm:593

• Because the preceding line continues the command, this # starts a shell comment while docker run is being assembled. The newline after the comment terminates the Docker command, so the following -e lines are executed as separate commands and the disaggregated container never receives the remaining Docker arguments. Move this note above docker run or outside the continued command.

    # Same reason as the nodelist above: parse_to_csv.py reads this for the
    # docker_image column and runs inside the container, so without it the column
    # is blank on every row this launcher produces.

scripts/vllm_dissag/run_xPyD_models.slurm:111

• validate_model_name runs before model_allows_mori_ep, so adding Kimi-K3 only to this backend-specific allowlist still rejects MODEL_NAME=Kimi-K3 at line 144. Add it to the global VALID_MODELS list so the new disaggregated card can reach the MoRI-EP validation.

    "Kimi-K3" \

scripts/vllm_multinode/run_multinode.slurm:332

• Because the preceding line continues the command, this # starts a shell comment while docker run is being assembled. The newline after the comment terminates the Docker command, so the following -e lines are executed as separate commands (-e: command not found) and no container starts. Move this provenance note above docker run or outside the continued command.

    # Provenance for the perf CSV. Both parse_to_csv.py implementations already
    # read these two names (_get_run_metadata -> docker_image, machine_name), but
    # they run INSIDE the container, so a name the -e list omits is simply absent
    # and the column comes out blank. scripts/sglang_disagg passes both and its
    # rows carry the image tag and nodelist; this launcher passed neither, which

scripts/vllm_multinode/run_multinode.slurm:314

• cluster.sh allows SHARED_MOUNT to be overridden and resolves MODEL_PATH under that root, but this bind mount is hard-coded to /shared_inference. On a site using another shared root, host-side discovery succeeds while the container sees an unmounted model path (or Docker creates an empty directory), so vLLM cannot load the weights. Bind the configured shared root instead.

    -v /shared_inference:/shared_inference \

scripts/vllm_multinode/run_multinode.slurm:112

• long_context is advertised as a valid selector, but benchmark_long_context.sh ends after the serving loops and never invokes parse_to_csv.py or writes /run_logs/${SLURM_JOB_ID}/perf.csv. The outer launcher unconditionally requires that file before completing, so every long-context run is reported as failed even when the benchmark itself completes. Add its CSV emission or remove this selector.

    long_context) BENCHMARK_SCRIPT_FILE="benchmark_long_context.sh" ;;

scripts/vllm_multinode/run_multinode.slurm:373

• Unlike the disaggregated launcher, this script never changes to SCRIPT_DIR, but the colocated Slurm config sets results_dir to scripts/vllm_multinode and documents that the CSV is copied beside the launcher. $(pwd) depends on the caller (including the madengine wrapper), so the collector can find no perf_Kimi-K3.csv; write to the already-computed script directory instead.

_PERF_DST="$(pwd)/perf_${MODEL_NAME}.csv"

scripts/vllm_multinode/serve_colocated.sh:258

• The colocated cards declare multiple_results, and the documented contract says those workload CSVs are narrow, but this shared invocation provides no narrow-reporting mode. benchmark_xPyD.sh calls parse_to_csv.py without --narrow, so a colocated sweep emits the legacy full-schema CSV instead of the declared contract. Add a colocated narrow mode or align the cards and documentation with legacy reporting.

bash "${SHARED_DIR}/${BENCHMARK_SCRIPT_FILE:-benchmark_xPyD.sh}"

• Files reviewed: 28/30 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved issues affect launcher defaults, routing, model paths, benchmark results, and image reproducibility.

Review details

Suppressed comments (14)

docker/pyt_vllm_kimi_k3_mi300x.ubuntu.amd.Dockerfile:291

• The Dockerfile claims every source is pinned to an immutable SHA, but the optional WITH_NIXL=1 path checks out rocm-systems at the moving develop branch and clones DeepEP without any ref. Rebuilding that supported variant can silently change the image even when this Dockerfile is unchanged; pin immutable commits (and record them in versions.txt) for these sources as well.

        cd rocm-systems && git sparse-checkout set --cone projects/rocshmem && git checkout develop && \
        mkdir -p /tmp/rocshmem-build && cd /tmp/rocshmem-build && \
        /tmp/rocm-systems/projects/rocshmem/scripts/build_configs/all_backends \
          -DUSE_EXTERNAL_MPI=OFF -DGPU_TARGETS="${GFX_COMPILATION_ARCH}" && \
      cd /tmp && git clone https://github.com/ROCm/DeepEP.git && cd DeepEP && \

scripts/common/cluster.sh:99

• These shared exports override the launchers' own defaults: run_xPyD_models.slurm later uses ${TP_SIZE:-1}, so every existing disaggregated run now inherits TP_SIZE=GPUS_PER_NODE (8) instead of TP1, while run_multinode.slurm can never fall back to PP_SIZE=NNODES because this sets PP1. That changes existing disagg topology and makes colocated jobs that omit PP_SIZE fail the GPU-count check. Keep TP_SIZE/PP_SIZE unset here and let each launcher own its defaults.

export TP_SIZE="${TP_SIZE:-${GPUS_PER_NODE}}"
export PP_SIZE="${PP_SIZE:-1}"

scripts/common/cluster.sh:307

• REQUIRE_LOCAL_WEIGHTS=1 is filtered before this block, but an explicit MODEL_DIR is appended afterward without the same filter. The Kimi cards set this flag, so pointing the site template's MODEL_DIR at /shared_inference silently re-enables the 1.5-TB NFS load despite the guard; reject or ignore shared MODEL_DIR values when local weights are required.

    if [ -n "${MODEL_DIR:-}" ]; then
        case " ${candidates} " in
            *" ${MODEL_DIR%/} "*) : ;;
            *) candidates="${candidates} ${MODEL_DIR%/}" ;;
        esac

scripts/vllm_dissag/benchmark_niah.py:32

• The implementation default changed to 8192, but the module's NIAH_MAXTOK environment documentation still says the default is 2048. Once the wrapper default is corrected, direct invocations will still be documented incorrectly; update the contract comment alongside this code change.

MAXTOK = int(os.environ.get("NIAH_MAXTOK", "8192"))

scripts/vllm_dissag/benchmark_niah.py:32

• This new Python fallback is not used by the normal harness: benchmark_niah.sh still exports NIAH_MAXTOK="${NIAH_MAXTOK:-2048}" before invoking Python. The Kimi cards do not set NIAH_MAXTOK, so their reasoning responses still run with 2048 tokens and can be truncated, despite this change documenting that 8192 is needed. Raise the shell wrapper default or set the value in the Kimi cards as well.

MAXTOK = int(os.environ.get("NIAH_MAXTOK", "8192"))

scripts/vllm_dissag/benchmark_niah.py:149

• The warning here is only text: parse_niah_log matches the summary prefix and save_niah_perf_csv writes every parsed row with status=SUCCESS. A context with some timeouts is therefore published as a successful mean, while a context whose requests all time out disappears from the CSV entirely. Emit an explicit failure/NaN result (or propagate the timeout status) so a degraded NIAH run cannot be reported as successful.

        if n_trunc:
            # A truncated answer scores low for lack of room, not lack of retrieval.
            extra += "  (TRUNCATED in %d/%d: raise NIAH_MAXTOK)" % (n_trunc, len(vals))

scripts/vllm_dissag/benchmark_niah.sh:44

• The Python default was raised to 8192, but this wrapper still exports NIAH_MAXTOK with a 2048 default. The Kimi cards do not set NIAH_MAXTOK, so normal Kimi runs continue to hit the 2048-token truncation that the new finish-reason diagnostic calls out. Raise this wrapper default (or set it explicitly in every reasoning-model card).

NIAH_MODEL="${NIAH_MODEL:-${MODEL_PATH}}" \
NIAH_WORDS="${NIAH_WORDS:-2000,8000,20000,35000}" \
NIAH_SEEDS="${NIAH_SEEDS:-0,1,2}" \
NIAH_MAXTOK="${NIAH_MAXTOK:-2048}" \

scripts/vllm_dissag/connectors/moriio.sh:275

• The new model_args expansion is duplicated by the existing expansion at line 278 in every non-dry-run wideEP command. This makes the real vLLM argv differ from DRY_RUN and passes every model-specific flag twice (including Kimi's quantization and reasoning flags); remove one of the two expansions.

            "${model_args[@]}" \

scripts/vllm_dissag/connectors/moriio.sh:219

• For Kimi's TP2 disaggregated shape, dp_size here is the pool-wide size (2 nodes × 4 DP ranks = 8), while each master has only DP_PARALLEL_SIZE_LOCAL=4. The comparison therefore leaves --api-server-count=8; clamp against the local DP count, otherwise the frontend can create routes for ranks not served by that master.

            local _api_servers="${_GPUS_PER_NODE}"
            [ "${dp_size}" -lt "${_api_servers}" ] && _api_servers="${dp_size}"
            extra_args+=(--api-server-count=${_api_servers})

scripts/vllm_dissag/run_xPyD_models.slurm:111

• validate_model_name runs against VALID_MODELS before it reaches MORI_EP_VALID_MODELS, but Kimi-K3 was added only to the latter list. Consequently the new pyt_vllm_disagg_mori_kimi-k3 card always exits as an invalid model before starting its MoRI-EP path. Add Kimi-K3 to the primary valid-model list as well, and keep the gate test mirror in sync.

    "Kimi-K3" \

scripts/vllm_multinode/run_multinode.slurm:113

• This selector exposes benchmark_long_context.sh, but that script only writes its log and never calls parse_to_csv.py to create /run_logs/$SLURM_JOB_ID/perf.csv. The mandatory post-run check below will therefore fail every BENCHMARK_SCRIPT=long_context colocated job even when the benchmark itself completes. Add the CSV emission to that benchmark or remove this selector until it produces the declared result.

    long_context) BENCHMARK_SCRIPT_FILE="benchmark_long_context.sh" ;;
    niah)         BENCHMARK_SCRIPT_FILE="benchmark_niah.sh" ;;

scripts/vllm_multinode/run_multinode.slurm:328

• cluster_resolve_model_path can select an explicit MODEL_DIR or a customized SHARED_MOUNT, but the container only mounts the hard-coded /shared_inference and the optional NVME root. With the site templates' documented custom MODEL_DIR, the host-side probe succeeds while the container cannot see MODEL_PATH, so vLLM fails with a missing model. Mount the resolved model root/configured shared root on every node before launching the container.

    -v /shared_inference:/shared_inference \
    ${_NVME_MOUNT} \

scripts/vllm_multinode/serve_colocated.sh:79

• Serving the colocated instance under MODEL_NAME breaks the shared throughput benchmarks: benchmark_xPyD.sh and benchmark_long_context.sh still invoke vllm bench serve --model "$MODEL_PATH", while only the NIAH wrapper overrides its request model via NIAH_MODEL. Sweep and long-context requests therefore use a model id the server does not serve and fail with 404s. Preserve a single served/request model id across all benchmark scripts.
  scripts/vllm_multinode/serve_colocated.sh:258
• The shared throughput harness resolves its parser as $NIXL_COOKBOOK_PATH/parse_to_csv.py, but this colocated launcher sets only SHARED_DIR and never defines NIXL_COOKBOOK_PATH. With BENCHMARK_SCRIPT=sweep, parsing consequently targets /parse_to_csv.py and no perf.csv is produced, so the valid sweep fails at the launcher copy check. Export NIXL_COOKBOOK_PATH="$SHARED_DIR" before invoking the shared script, or update the harness to use SHARED_DIR.

• Files reviewed: 29/31 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical and moderate launch, model-resolution, benchmark, and validation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (12)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/vllm_dissag/connectors/moriio.sh:275

• model_args is appended both here and again at line 278. This duplicates every model-specific flag in the actual WIDE_EP launch, including Kimi's quantization and reasoning arguments; the DRY_RUN command does not show the duplicate, so the current argv test will not catch it. Keep only the existing later expansion.

benchmark/kimi_k3/README.md:15

• The overview table contains three framework/model entries (vLLM, SGLang, and ATOM), not four, so this statement is inaccurate. Say “all three” or list the missing fourth model to avoid confusing the MI300X coverage.

> **On MI300X (gfx942)?** All four models above carry `skip_gpu_arch: gfx942` —

scripts/common/cluster.sh:99

• Because this file is sourced before either launcher applies its own fallback, these exports override both contracts: a default disaggregated run now gets TP_SIZE=8 instead of TP1, while a default colocated 2-node run gets PP_SIZE=1 and then fails the TP*PP == total-GPUs check. Leave these launcher-specific variables unset here.

export TP_SIZE="${TP_SIZE:-${GPUS_PER_NODE}}"
export PP_SIZE="${PP_SIZE:-1}"

scripts/common/cluster.sh:307

• When REQUIRE_LOCAL_WEIGHTS=1, shared candidates are removed above, but this unconditional append reintroduces an explicit MODEL_DIR under SHARED_MOUNT. A Kimi run with MODEL_DIR=/shared_inference/... will therefore fall back to the NFS checkpoint despite the card's local-only hard-fail requirement. Apply the same local-only filter to this explicit candidate.

    if [ -n "${MODEL_DIR:-}" ]; then
        case " ${candidates} " in
            *" ${MODEL_DIR%/} "*) : ;;
            *) candidates="${candidates} ${MODEL_DIR%/}" ;;
        esac

scripts/vllm_dissag/benchmark_niah.py:145

• When all seeds for a context time out, this branch emits NO-RESULT and continues without a summary line. The new benchmark_niah.sh parser only recognizes summary lines, so that context is omitted from perf.csv; with partial timeouts, the remaining row is still written as SUCCESS. Emit a failure row/status for missing contexts so a long-context regression cannot pass by omission.

        counts = [c for c, _ in vals]
        n_trunc = sum(1 for _, f in vals if f == "length")
        mean = sum(counts) / len(counts)

scripts/vllm_dissag/benchmark_niah.py:32

• This new Python default is never used by the normal NIAH launcher: benchmark_niah.sh always sets NIAH_MAXTOK=${NIAH_MAXTOK:-2048} before invoking this file. Therefore Kimi-K3 still runs with 2048 max tokens unless every card overrides NIAH_MAXTOK, despite this change claiming to give reasoning models headroom.

MAXTOK = int(os.environ.get("NIAH_MAXTOK", "8192"))

scripts/vllm_dissag/benchmark_niah.sh:55

• This newly enabled CSV path does not preserve the failure semantics described by the surrounding documentation: parse_niah_log only recognizes mean summary lines, so an all-timeout context is omitted and a partial-timeout context is still written as SUCCESS by save_niah_perf_csv. Emit an explicit failure/zero row for those contexts, or do not claim that request errors are recorded.

python3 "${DIR}/parse_to_csv.py" "${LOG}" --niah \
        --perf-csv "/run_logs/${SLURM_JOB_ID}/perf.csv" \
        --model-name "${MODEL_NAME}" 2>&1 | tee -a "${LOG}"

scripts/vllm_dissag/parse_to_csv.py:359

• --narrow is documented here as producing a narrow NIAH CSV, but the --niah path calls save_niah_perf_csv, whose fieldnames are the full legacy metadata schema and whose rows are always SUCCESS. The new NIAH launcher therefore does not produce the narrow multiple_results contract described in the Kimi README; make the NIAH path honor the same schema or correct the contract/documentation.

    parser.add_argument('--narrow', action='store_true',
                        help='Emit a narrow results CSV (model/performance/metric[/status]) for a model card '
                             'declaring multiple_results, letting madengine supply the run metadata. '
                             'Ignored with --niah, which is always narrow.')

scripts/vllm_multinode/run_multinode.slurm:335

• cluster_resolve_model_path accepts an explicit MODEL_DIR and configurable shared roots, and the site README documents placing the checkpoint wherever the site needs. The container only binds /shared_inference and /mnt/m2m_nobackup, so a valid model selected from any other root is invisible inside Docker and vLLM fails to load it. Mount the selected model root (or reject unsupported paths) before launching.
  scripts/vllm_multinode/run_multinode.slurm:374
• benchmark_xPyD.sh resolves parse_to_csv.py through NIXL_COOKBOOK_PATH, but this container invocation never sets that variable. The default sweep therefore tries /parse_to_csv.py and produces no perf.csv; pass the mounted vllm_dissag directory as NIXL_COOKBOOK_PATH for the benchmark.
  scripts/vllm_multinode/run_multinode.slurm:112
• This option selects benchmark_long_context.sh, but that script only runs vllm bench and never invokes parse_to_csv.py. The launcher later unconditionally requires ${LOG_PATH}/${SLURM_JOB_ID}/perf.csv, so selecting the documented long_context mode always ends as a failure for a missing results file.
  scripts/vllm_multinode/serve_colocated.sh:79
• The server advertises only MODEL_NAME (for example, Kimi-K3), but the shared throughput and long-context benchmarks send MODEL_PATH as their --model request id. Thus every non-NIAH selection receives model-not-found/404 responses; only benchmark_niah overrides its request model. Use one common model id or pass the served name through those benchmark scripts.

• Files reviewed: 31/33 changed files
• Comments generated: 4
• Review effort level: Lite