Skip to content

[AMD] Update test_gpt_oss_1gpu.py#23829

Closed
yctseng0211 wants to merge 2 commits into
mainfrom
fix_amd_stage_c_0427
Closed

[AMD] Update test_gpt_oss_1gpu.py#23829
yctseng0211 wants to merge 2 commits into
mainfrom
fix_amd_stage_c_0427

Conversation

@yctseng0211
Copy link
Copy Markdown
Collaborator

Motivation

Modifications

Accuracy Tests

Speed Tests and Profiling

Checklist

Review and Merge Process

  1. Ping Merge Oncalls to start the process. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • Common commands include /tag-and-rerun-ci, /tag-run-ci-label, /rerun-failed-ci
  4. After green CI and required approvals, ask Merge Oncalls or people with Write permission to merge the PR.

@yctseng0211 yctseng0211 changed the title Update test_gpt_oss_1gpu.py [AMD] Update test_gpt_oss_1gpu.py Apr 27, 2026
Copy link
Copy Markdown
Contributor

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

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 updates the estimated execution time for CUDA CI and modifies the test_mxfp4_20b test to conditionally set the SGLANG_USE_AITER environment variable when running on HIP-compatible hardware. Feedback suggests replacing the manual environment variable management with the more idiomatic unittest.mock.patch.dict to improve code readability and ensure proper cleanup.

Comment on lines +14 to +31
prev = os.environ.get("SGLANG_USE_AITER")
if is_hip():
os.environ["SGLANG_USE_AITER"] = "1"
try:
self.run_test(
model_variant="20b",
quantization="mxfp4",
expected_score_of_reasoning_effort={
"low": 0.34,
"medium": 0.34,
"high": 0.27, # TODO investigate
},
)
finally:
if prev is None:
os.environ.pop("SGLANG_USE_AITER", None)
else:
os.environ["SGLANG_USE_AITER"] = prev
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

The manual management of environment variables using try...finally is verbose and can be simplified using unittest.mock.patch.dict. This approach is more idiomatic for Python tests and ensures the environment is correctly restored even if exceptions occur during setup or execution. It also prevents potential environment leakage to subsequent tests.

        from unittest.mock import patch
        env_updates = {"SGLANG_USE_AITER": "1"} if is_hip() else {}
        with patch.dict(os.environ, env_updates):
            self.run_test(
                model_variant="20b",
                quantization="mxfp4",
                expected_score_of_reasoning_effort={
                    "low": 0.34,
                    "medium": 0.34,
                    "high": 0.27,  # TODO investigate
                },
            )

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant