Skip to content

Rocm changes to tests/e2e/megatron/test_glm5_744b_a40b_4layer.py#1613

Open
sreerohi wants to merge 1 commit into
radixark:mainfrom
sreerohi:rocm/glm5_744b_a440_4layer_changes
Open

Rocm changes to tests/e2e/megatron/test_glm5_744b_a40b_4layer.py#1613
sreerohi wants to merge 1 commit into
radixark:mainfrom
sreerohi:rocm/glm5_744b_a440_4layer_changes

Conversation

@sreerohi

Copy link
Copy Markdown

Supersedes #1172

…ark#1122 , radixark#1123):

- Drop DeepEP; use alltoall dispatcher
- Swap flashmla_sparse NSA backend for tilelang
- Set sglang-page-size to 1 (required by tilelang NSA)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces ROCm compatibility to the GLM5 training script by dynamically detecting ROCm environments and adjusting DeepEP settings, NSA backends, KV cache types, page sizes, and environment variables accordingly. The feedback recommends refactoring the construction of sglang_args to avoid mixing implicit string concatenation with explicit + operators, which will improve code readability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 351 to 359
sglang_args += (
# use flashmla backend for better precision
"--sglang-nsa-decode-backend flashmla_sparse "
"--sglang-nsa-prefill-backend flashmla_sparse "
"--sglang-kv-cache-dtype bf16 "
"--sglang-attention-backend nsa "
"--sglang-page-size 64 "
f"--sglang-nsa-decode-backend {nsa_backend} "
f"--sglang-nsa-prefill-backend {nsa_backend} "
+ ("--sglang-kv-cache-dtype bf16 " if not IS_ROCM else "")
+ "--sglang-attention-backend nsa "
f"--sglang-page-size {1 if IS_ROCM else 64} "
f"--sglang-cuda-graph-max-bs {sglang_decode_max_bs} "
# concurrency
f"--sglang-max-running-requests 512 "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Mixing implicit string literal concatenation and explicit + operators inside a parenthesized expression is highly unreadable and error-prone. It is much cleaner and more maintainable to split the string construction or use separate additions.

    sglang_args += (
        f"--sglang-nsa-decode-backend {nsa_backend} "
        f"--sglang-nsa-prefill-backend {nsa_backend} "
    )
    if not IS_ROCM:
        sglang_args += "--sglang-kv-cache-dtype bf16 "
    sglang_args += (
        "--sglang-attention-backend nsa "
        f"--sglang-page-size {1 if IS_ROCM else 64} "
        f"--sglang-cuda-graph-max-bs {sglang_decode_max_bs} "
        # concurrency
        f"--sglang-max-running-requests 512 "
    )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant