JOB_ID=$(squeue -u $USER --noheader --format='%i') is not a robust way to get the job ID. It will retrieve all job IDs for the current user, which can lead to incorrect behavior if the user has other jobs running. If submit_disagg.sh submits multiple jobs, this will capture all of them, likely breaking subsequent commands that expect a single ID (e.g., LOG_FILE creation).

The job submission script (submit_disagg.sh, called from the benchmark script) should be modified to return the SLURM job ID(s) it creates, and this script should capture that output. For example, if sbatch is used, its output can be captured and printed.

Example of what could be done in the submission script:

# inside submit_disagg.sh
JOB_ID=$(sbatch my_job.slurm | awk '{print $4}')
echo $JOB_ID

Then in launch_mi355x-amd.sh:

JOB_ID=$(bash benchmarks/"${EXP_NAME%%_*}_${PRECISION}_mi355x_${FRAMEWORK}_slurm.sh")

The script clones a repository from a personal GitHub account (billishyahao) and a specific branch (sa-260107). This introduces a significant risk to the CI/CD pipeline's stability and security. Personal branches can be force-pushed, rebased, or deleted at any time, which would break this script. It's strongly recommended to use a versioned release, a tag, or a commit hash from an organization-controlled repository to ensure reproducibility and stability.

JOB_ID=$(squeue -u $USER -h -o %A | head -n1) is not a robust way to get the job ID. It relies on the job being the most recent one submitted by the user, which can be fragile in a shared environment. salloc prints the job ID to standard output when an allocation is granted. You should capture this output directly.

The search-space configuration is duplicated three times for different isl/osl combinations. This makes the file very long and hard to maintain. You can use YAML anchors and aliases to define the search-space once and reuse it. This will significantly reduce the file size and improve readability.

For example:

# At the top level of your YAML, define an anchor
x-dsr1-search-space: &dsr1_search_space
  # MTP configurations
  # "Top of curve" (1 prefill worker at DEP8 and 1 decode worker at DEP16)
  - spec-decoding: "mtp"
    conc-list: [ 2048 ]
    # ... rest of the search space items

ds1-fp8-mi355x-sglang-disagg:
  # ... other properties
  seq-len-configs:
  - isl: 1024
    osl: 1024
    search-space: *dsr1_search_space # Reuse with an alias
  - isl: 8192
    osl: 1024
    search-space: *dsr1_search_space # Reuse with an alias
  - isl: 1024
    osl: 8192
    search-space: *dsr1_search_space # Reuse with an alias

The environment variable SGL_SLURM_JOBS_PATH is listed twice in the call to check_env_vars. While this is harmless, it's redundant and should be removed for better code clarity.

The script uses sudo rm -rf ... to clean up log directories (here and on line 126). Using sudo within a CI script can be a security risk and may indicate underlying permissions issues. It would be better to investigate why sudo is needed and fix the permissions of the files and directories being created, so that the runner user can remove them without elevated privileges. If sudo is unavoidable, please add a comment explaining why.

The for result_file in $(find ...) loop is not safe for filenames that contain spaces or other special characters. While it might not be an issue with the current file naming scheme, it's a good practice to write robust shell scripts. Consider using a while read loop with find -print0 to handle all possible filenames correctly. This also allows for safe quoting of variables and removal of a redundant file check.

Missing head filter causes multi-job JOB_ID capture failure

High Severity

In the multinode code path, JOB_ID is assigned from squeue without | head -n1, unlike the non-multinode path (line 150) and all other launch scripts (launch_b200-nv.sh, launch_h100-cw.sh, launch_h200-cw.sh, etc.). If multiple jobs exist for the user, JOB_ID will contain multiple IDs separated by newlines, causing LOG_FILE to have an invalid path with embedded newlines and breaking subsequent operations like the grep -q "$JOB_ID" check and scancel_sync $JOB_ID call.

Silent git clone failure leads to stale code execution

Medium Severity

The launcher only removes $SGL_SLURM_JOBS_PATH/logs but the benchmark script clones the entire sglang_disagg directory. On subsequent runs, the git clone fails silently (no set -e in the benchmark script) because the directory already exists, but cd "$SGL_SLURM_JOBS_PATH" succeeds and the script proceeds with potentially stale code. This could cause benchmarks to run with outdated versions of submit_disagg.sh without any error or warning, leading to incorrect results. The cleanup at line 44 and 126 needs to remove the entire sglang_disagg directory, not just the logs subdirectory.