[AMD] feat: Add mi355x distributed inference test CI workflow#401
[AMD] feat: Add mi355x distributed inference test CI workflow#401
Conversation
Summary of ChangesHello @cquil11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a new continuous integration workflow to validate distributed inference performance on AMD MI355x accelerators. It incorporates Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CI workflow for distributed inference testing on MI355x hardware. The changes include a new YAML configuration for the test suite, a benchmark script for sglang-disagg, and significant modifications to the launch_mi355x-amd.sh runner script to handle multi-node SLURM jobs. My review focuses on improving the maintainability, robustness, and security of the new scripts. Key areas for improvement include reducing duplication in the YAML configuration using anchors, avoiding dependencies on personal GitHub repositories in CI, removing the use of sudo for better security, and fixing a fragile file processing loop in the runner script.
![cursor[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| bash benchmarks/"${EXP_NAME%%_*}_${PRECISION}_mi355x_${FRAMEWORK}_slurm.sh" | ||
|
|
||
| # Wait for job to complete | ||
| JOB_ID=$(squeue -u $USER --noheader --format='%i') |
There was a problem hiding this comment.
Missing validation that job was submitted successfully
Medium Severity
The benchmark script execution on line 46 has no return code check, and there's no validation that JOB_ID is non-empty before proceeding. If the benchmark script fails (e.g., git clone fails in the called script), execution continues and JOB_ID captures whatever jobs happen to be in the queue. If JOB_ID is empty but other user jobs exist, the loop on lines 56-63 becomes infinite because grep -q "" matches any output, preventing the error condition from triggering while the expected log file never appears.
3 participants
This patch is to add mi355x distributed inference test CI workflow and integrate recipes https://github.com/billishyahao/sglang_disagg into inferencemax github action
Note
Adds multinode disaggregated benchmarking for DeepSeek-R1 FP8 on MI355X using SGLang.
dsr1-fp8-mi355x-sglang-disaggconfig inamd-master.yamlwith prefill/decode worker splits, EP/DP toggles, and both MTP and non-MTP acrossisl/oslpairs (1k1k, 8k1k, 1k8k)benchmarks/dsr1_fp8_mi355x_sglang-disagg_slurm.shthat clonessglang_disagg, sets env, and submits disagg Slurm jobsrunners/launch_mi355x-amd.shgains multinode path:scancel_sync, Slurm env, job submit, log tailing, result collection, and cleanup; retains single-node flowperf-changelog.yamldocumenting the new MI355X 1P2D disaggregation (with/without speculative decoding)Written by Cursor Bugbot for commit 1460803. This will update automatically on new commits. Configure here.