add random range ratio that is appropriate#402
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 integrates a new 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 adds support for the RANDOM_RANGE_RATIO environment variable to the Slurm benchmark script. The variable is now checked for existence and passed to the submit_disagg.sh script. The change also updates the git repository URL for sglang_disagg. My review includes one suggestion to clean up a duplicated variable in the environment check.
| PREFILL_NUM_WORKERS PREFILL_TP PREFILL_EP PREFILL_DP_ATTN \ | ||
| DECODE_NUM_WORKERS DECODE_TP DECODE_EP DECODE_DP_ATTN \ | ||
| PREFILL_NODES DECODE_NODES SGL_SLURM_JOBS_PATH # SGL_SLURM_JOBS_PATH FIXME | ||
| PREFILL_NODES DECODE_NODES SGL_SLURM_JOBS_PATH RANDOM_RANGE_RATIO SGL_SLURM_JOBS_PATH |
There was a problem hiding this comment.
The SGL_SLURM_JOBS_PATH variable is listed twice in the call to check_env_vars. This is redundant and likely a copy-paste error. Please remove the duplicate entry for better code clarity.
| PREFILL_NODES DECODE_NODES SGL_SLURM_JOBS_PATH RANDOM_RANGE_RATIO SGL_SLURM_JOBS_PATH | |
| PREFILL_NODES DECODE_NODES SGL_SLURM_JOBS_PATH RANDOM_RANGE_RATIO |
![cursor[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| PREFILL_NUM_WORKERS PREFILL_TP PREFILL_EP PREFILL_DP_ATTN \ | ||
| DECODE_NUM_WORKERS DECODE_TP DECODE_EP DECODE_DP_ATTN \ | ||
| PREFILL_NODES DECODE_NODES SGL_SLURM_JOBS_PATH # SGL_SLURM_JOBS_PATH FIXME | ||
| PREFILL_NODES DECODE_NODES SGL_SLURM_JOBS_PATH RANDOM_RANGE_RATIO SGL_SLURM_JOBS_PATH |
There was a problem hiding this comment.
Duplicate SGL_SLURM_JOBS_PATH in environment variable check
Low Severity
SGL_SLURM_JOBS_PATH appears twice in the check_env_vars call. The original line had a comment # SGL_SLURM_JOBS_PATH FIXME that was accidentally converted into actual code when adding RANDOM_RANGE_RATIO. While this won't break the script (the variable is just checked twice), it's clearly unintentional duplication from removing the comment incorrectly.
* Revert "Revert "[AMD] Add mi355x distributed inference test CI workflow (#348)" (#400) [skip-sweep]" This reverts commit a075f2e. * add random range ratio that is appropriate (#402) * comment out 1k8k 8k1k * change recipe back to upstream * revert comment out 1k8k 8k1k * Add empty line at the beginning of the script --------- Co-authored-by: billishyahao <[email protected]>
* add random range ratio that is appropriate (#402) * comment out 1k8k 8k1k * change recipe back to upstream * revert comment out 1k8k 8k1k * Add empty line at the beginning of the script --------- Co-authored-by: billishyahao <[email protected]> Co-authored-by: ichbinblau [email protected] Co-authored-by: Duyi-Wang [email protected] Co-authored-by: inkcherry [email protected]
1 participant
This is a followup to https://github.com/InferenceMAX/InferenceMAX/pull/348 .
This PR referenced another repo that called the benchmark serving script without the appropriate random range ratio. Here are the associated changes (comparing to the original branch): https://github.com/billishyahao/sglang_disagg/pull/8/changes. Note that since I do not have write access to the upstream sglang_disagg repository, I changed to my fork in this PR. But you can see the changes above.
Note
Introduces a new runtime parameter and updates the source repo for the benchmark job launcher.
RANDOM_RANGE_RATIOviacheck_env_varsand as an argument tosubmit_disagg.shsglang_disaggclone URL tohttps://github.com/cquil11/sglang_disagg.git(branchsa-260107)Written by Cursor Bugbot for commit 82e555f. This will update automatically on new commits. Configure here.