Skip to content

feat(rocm): Add AMD ROCm/MI300X support — FP8 hipBLASLt, MIGraphX backend, AMD quantization configs#1990

Open
zhihuidu-amd wants to merge 1 commit into
NVIDIA:mainfrom
zhihuidu-amd:rocm/amd-fp8-support
Open

feat(rocm): Add AMD ROCm/MI300X support — FP8 hipBLASLt, MIGraphX backend, AMD quantization configs#1990
zhihuidu-amd wants to merge 1 commit into
NVIDIA:mainfrom
zhihuidu-amd:rocm/amd-fp8-support

Conversation

@zhihuidu-amd

@zhihuidu-amd zhihuidu-amd commented Jul 17, 2026

Copy link
Copy Markdown

Summary

Add AMD ROCm support to Model-Optimizer, targeting MI300X/MI325X (gfx942/CDNA3) GPUs with FP8 acceleration via hipBLASLt.

Performance Results (AMD MI300X, gfx942, ROCm 7.0)

Shape Batch Size Speedup vs FP16
Standard FFN (H=4096, I=16384) 256 1.90–1.95×
LLaMA-70B FFN (H=8192, I=28672) 1 1.81× (single-token decode)
LLaMA-70B FFN 4 1.83× (peak)
hipBLASLt FP8 peak (single GEMM) 662 TFLOPS

Changes (all purely additive — only config.py modified)

New: modelopt/_rocm_compat.py

  • FP8Linear: drop-in nn.Linear replacement → torch._scaled_mm → hipBLASLt
  • Full deployment pipeline: calibrate → extract scales → convert → warmup → deploy
  • Selective quantization: FFN-only for small BS, full FP8 for large BS
  • KV cache FP8 quantization (50% memory reduction)
  • QLoRA fine-tuning over frozen FP8 base weights

New: MIGraphX Deploy Backend (modelopt/torch/_deploy/_runtime/migrachx/)

  • migrachx_client.py: INT8/FP8 ONNX inference via AMD MIGraphX

Modified: modelopt/torch/quantization/config.py

  • AMD_FP8_DEFAULT_CFG: FP8 E4M3FNUZ per-tensor config for CDNA3
  • AMD_INT8_DEFAULT_CFG: INT8 per-channel weight config for hipBLASLt (1,205 TOPS peak)

New: YAML Presets, Tests, Docs

  • modelopt_recipes/configs/ptq/presets/model/amd_fp8.yaml / amd_int8.yaml
  • tests/amd/: 32 tests — 25 unit + 7 integration, all green on MI300X
  • README_AMD_DEPLOYMENT.md, quickstart example, CI benchmark script

Testing

Validated on AMD MI300X, gfx942:sramecc+:xnack-, ROCm 7.0.51831, PyTorch 2.9:

pytest tests/amd/test_amd_rocm.py          # 25/25 PASS
pytest tests/amd/test_amd_integration.py   # 7/7 PASS
bash scripts/amd_benchmark_ci.sh           # PASS — 1.90-1.95× at BS=256

Key Design Notes

  • Uses torch.float8_e4m3fnuz (AMD CDNA exponent bias, differs from NVIDIA float8_e4m3fn)
  • FP8 dispatch via torch._scaled_mm → hipBLASLt (no Triton JIT overhead)
  • Static input scales required for speedup (dynamic amax adds ~0.08ms overhead)
  • Do not combine torch.compile with FP8Linear (compile overhead negates GEMM gains)

AMD-AIOSS source: https://github.com/AMD-AIOSS/ROCm-Model-Optimizer

Summary by CodeRabbit

  • New Features

    • Added AMD ROCm support for FP8 and INT8 quantization, calibration, model conversion, benchmarking, and deployment.
    • Added MIGraphX runtime integration for ONNX compilation, profiling, and inference.
    • Added FP8 KV-cache optimization, scale management, checkpoint support, and model reporting.
    • Added AMD quantization presets and executable deployment and benchmarking examples.
  • Documentation

    • Added ROCm setup guidance and an AMD MI300X FP8 deployment guide.
  • Tests

    • Added ROCm, FP8, MIGraphX, export, inference, accuracy, and performance validation coverage.

…, AMD quantization configs

Add AMD ROCm support targeting MI300X/MI325X (gfx942/CDNA3):

## New AMD APIs (modelopt/_rocm_compat.py)
- FP8Linear: drop-in nn.Linear replacement using torch._scaled_mm → hipBLASLt
- convert_to_static_fp8(): calibrate → extract scales → deploy pipeline
- AMD_FP8_DEFAULT_CFG / AMD_INT8_DEFAULT_CFG: quantization presets for CDNA3
- warmup_for_llama(): pre-warm hipBLASLt for LLaMA-7B/13B/34B/70B shapes
- get_quantization_strategy(): recommends FFN-only vs full FP8 based on batch size
- save/load_fp8_scales(), save/load_amd_fp8_checkpoint(): persistence helpers
- KV cache FP8 quantization (50% memory reduction)
- QLoRA fine-tuning over frozen FP8 weights

## MIGraphX Deploy Backend (modelopt/torch/_deploy/_runtime/migrachx/)
- migrachx_client.py: MIGraphX runtime client for INT8/FP8 ONNX inference
- Enables AMD GPU INT8 deployment via hipBLASLt kernels

## AMD Quantization Configs (modelopt/torch/quantization/config.py)
- AMD_FP8_DEFAULT_CFG: FP8 E4M3FNUZ per-tensor config for CDNA3
- AMD_INT8_DEFAULT_CFG: INT8 per-channel weight config for hipBLASLt

## YAML Presets
- modelopt_recipes/configs/ptq/presets/model/amd_fp8.yaml
- modelopt_recipes/configs/ptq/presets/model/amd_int8.yaml

## AMD Test Suite (tests/amd/)
- test_amd_rocm.py: 25 unit tests (ROCm detection, FP8/INT8 pipeline)
- test_amd_integration.py: 7 integration tests (full calibrate→deploy pipeline)
- test_amd_migrachx.py: MIGraphX ONNX export tests

## Performance Results (AMD MI300X, gfx942, ROCm 7.0)
- FFN (H=4096, I=16384): 1.90-1.95x FP16 at BS=256
- LLaMA-70B FFN (H=8192, I=28672): 1.81x at BS=1 (single-token decode)
- hipBLASLt FP8 peak: 662 TFLOPS single GEMM

## Docs & Examples
- README_AMD_DEPLOYMENT.md: deployment guide with benchmark tables
- examples/amd_fp8_quickstart.py: end-to-end MI300X quickstart
- examples/amd_llama_fp8_inference.py: LLaMA attention+FFN FP8 example
- scripts/amd_benchmark_ci.sh: automated CI benchmark (1.4x threshold)

Co-developed-by: AMD DCGPU AI Solutions Team
@zhihuidu-amd
zhihuidu-amd requested review from a team as code owners July 17, 2026 21:23
@copy-pr-bot

copy-pr-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds AMD ROCm support across quantization, FP8/INT8 deployment, MIGraphX execution, examples, benchmarks, documentation, and ROCm-gated tests.

Changes

AMD ROCm FP8 deployment

Layer / File(s) Summary
Quantization presets and calibration
modelopt/torch/quantization/*, modelopt/torch/kernels/quantization/gemm/*, modelopt_recipes/configs/ptq/presets/model/*
Adds AMD FP8/INT8 presets, calibration loops, and fused FP8 GEMM fallback behavior.
ROCm compatibility and quantized layers
modelopt/_rocm_compat.py
Adds ROCm detection, FP8/INT8 linear layers, scale conversion, warmup, and inference preparation.
AMD deployment and lifecycle utilities
modelopt/_rocm_compat.py
Adds KV-cache handling, sharding, profiling, persistence, ONNX tooling, strategy selection, HuggingFace workflows, checkpointing, and fine-tuning preparation.
MIGraphX runtime backend
modelopt/torch/_deploy/_runtime/migrachx/*
Registers MIGraphX and implements ONNX compilation, profiling, and inference.
Examples, benchmarks, and documentation
README_AMD_DEPLOYMENT.md, README_ROCM.md, examples/*, scripts/*
Documents AMD deployment and adds quickstart, LLaMA, CI, Slurm, and benchmark workflows.
ROCm and MIGraphX validation
tests/amd/*
Adds ROCm-gated tests for quantization, deployment, performance, persistence, ONNX export, and MIGraphX execution.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant ModelOpt
  participant AMDModelOptimizer
  participant MIGraphXLocalClient
  participant MIGraphX
  User->>ModelOpt: provide calibration data
  ModelOpt->>AMDModelOptimizer: calibrate and extract scales
  AMDModelOptimizer->>AMDModelOptimizer: convert to static FP8 and warm up
  User->>MIGraphXLocalClient: submit ONNX deployment
  MIGraphXLocalClient->>MIGraphX: compile and execute model
  MIGraphX-->>MIGraphXLocalClient: return inference outputs
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Security Anti-Patterns ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main ROCm/MI300X additions: FP8 support, MIGraphX backend, and AMD quantization configs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 2

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (31)
modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py-133-137 (1)

133-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Remove the unsupported fp16_mode kwarg from quantize_fp16
migraphx.quantize_fp16() only accepts prog and ins_names; passing fp16_mode=True will raise TypeError on the FP16 path. Call mgx.quantize_fp16(model) directly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py` around lines 133
- 137, Update the precision handling around quantize_fp16 and quantize_bf16 to
call mgx.quantize_fp16(model) directly for FP16, removing the fp16_mode
quant_args and avoiding unsupported keyword arguments; preserve the existing
BF16 quantization behavior.
modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py-166-169 (1)

166-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use migraphx.save_buffer/migraphx.load_buffer here instead of BytesIO. migraphx.save and migraphx.load take filenames, so these calls won’t work at the three in-memory sites.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py` around lines 166
- 169, The in-memory serialization paths incorrectly use filename-based
migraphx.save/load with BytesIO. In
modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py at lines 166-169,
199-200, and 253-254, replace those calls with migraphx.save_buffer and
migraphx.load_buffer while preserving the existing byte-buffer data flow.
scripts/amd_benchmark_ci.sbatch-9-12 (1)

9-12: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Remove user-specific cluster paths from the launcher.

The hardcoded /scratch/users/zhdu/... paths make this CI script work only for one account. Accept them through required environment variables or derive the workspace from SLURM_SUBMIT_DIR.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/amd_benchmark_ci.sbatch` around lines 9 - 12, Update the launcher
variables DOCKER_CONFIG and WORK to remove the hardcoded user-specific paths:
require callers to provide the paths through environment variables or derive
WORK from SLURM_SUBMIT_DIR. Preserve the existing variable names and ensure the
script fails clearly when required configuration is unavailable.
tests/amd/test_amd_integration.py-88-93 (1)

88-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Require an actual speedup in the speedup test.

The test claims to verify FP8 speedup but passes with speedup == 0.85, allowing a 15% regression. Use the documented conservative speedup threshold or rename this as a severe-regression smoke test.

As per path instructions, tests must exercise the behavior they claim to validate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_integration.py` around lines 88 - 93, Update the speedup
assertion in the AMD integration test to require the documented conservative
1.3x minimum, so the test actually validates an FP8 performance improvement.
Keep the existing speedup calculation and diagnostic failure message aligned
with the new threshold.

Source: Path instructions

tests/amd/test_amd_integration.py-126-150 (1)

126-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Exercise amd_deploy_model in the one-call API test.

This test never imports or calls the API named in its title; it manually calibrates and converts the model. Consequently, regressions in the actual one-call deployment API remain undetected.

As per path instructions, tests must exercise the behavior they claim to validate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_integration.py` around lines 126 - 150, Update
test_amd_deploy_model_one_call to import and invoke the actual one-call
deployment API instead of directly calling mtq.quantize and
convert_to_static_fp8. Preserve the FP8 support guard and assertions, but make
the test validate the complete calibrate-and-convert flow named by the test.

Source: Path instructions

examples/amd_llama_fp8_inference.py-107-133 (1)

107-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use identical weights for calibration, FP16, and FP8 models.

The three models are independently randomized, so scales are applied to unrelated FP8 weights and the benchmark does not demonstrate a valid calibrated deployment. Create one base model and deep-copy it for calibration, FP16, and FP8 conversion.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/amd_llama_fp8_inference.py` around lines 107 - 133, Update the model
construction around the calibration flow so one base LLaMALayer Sequential model
is created once, then deep-copied for the calibration model, FP16 reference
model, and FP8 conversion model. Ensure calibration extracts scales from that
copy and convert_to_static_fp8 operates on the corresponding identical-weight
copy, preserving matching parameters across all three models.
tests/amd/test_amd_rocm.py-160-178 (1)

160-178: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use matching module names when testing calibrated scales.

Calibration produces scale keys such as fc1 and fc2, while the fresh nn.Sequential uses 0 and 2. convert_to_static_fp8 therefore falls back to scale 1.0, and this test never validates calibrated-scale application. Recreate type(small_ffn)() and assert the resulting scale_x values.

As per path instructions, tests must exercise the behavior they claim to validate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_rocm.py` around lines 160 - 178, Update
test_convert_to_static_fp8 to instantiate a fresh type(small_ffn)() so its
module names match the calibrated scales extracted from small_ffn. Pass those
scales to convert_to_static_fp8 and assert the converted FP8Linear modules’
scale_x values match the corresponding calibrated values, rather than only
asserting that conversion occurred.

Source: Path instructions

tests/amd/test_amd_integration.py-221-234 (1)

221-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add a real FP8 accuracy assertion.

The test only checks that the FP16 output is nonzero and shapes match. torch.nan_to_num also lets NaN/Inf FP8 output pass silently. Assert finite outputs and a documented error/cosine-similarity bound, or call validate_fp8_accuracy.

As per path instructions, tests must exercise the behavior they claim to validate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_integration.py` around lines 221 - 234, Add substantive
FP8 accuracy validation to the comparison around ffn_fp16 and ffn_fp8: assert
both outputs are finite before any sanitization, then enforce the project’s
documented error or cosine-similarity threshold (prefer reusing
validate_fp8_accuracy if available). Keep the existing shape and nonzero checks,
but do not use torch.nan_to_num to mask invalid FP8 results.

Source: Path instructions

tests/amd/test_amd_integration.py-38-272 (1)

38-272: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Move unconditional imports to module scope.

These test modules repeatedly import standard-library and required project modules inside test methods without circular-dependency or optional-dependency justification.

  • tests/amd/test_amd_integration.py#L38-L272: move time, copy, os, ModelOpt, and ROCm helper imports to the top.
  • tests/amd/test_amd_migrachx.py#L52-L199: move required imports to the top; retain optional onnx, migraphx, and NumPy imports locally only with explanatory comments.
  • tests/amd/test_amd_rocm.py#L46-L299: move ModelOpt and ROCm helper imports to the top.

As per path instructions, imports inside tests require an explicit optional-dependency or circular-import justification so import errors surface during collection.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_integration.py` around lines 38 - 272, Move unconditional
imports in tests/amd/test_amd_integration.py (lines 38-272),
tests/amd/test_amd_migrachx.py (lines 52-199), and tests/amd/test_amd_rocm.py
(lines 46-299) to module scope, including standard-library, ModelOpt, and ROCm
helper imports. In test_amd_migrachx.py, retain local imports only for optional
onnx, migraphx, and NumPy dependencies, with explanatory comments; remove other
method-level imports so missing required dependencies fail during collection.

Sources: Coding guidelines, Path instructions

tests/amd/test_amd_migrachx.py-52-64 (1)

52-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make the importability test assert importability.

The test catches ImportError and then unconditionally executes assert True, so it cannot detect a missing or broken MIGraphX installation. Use the existing fixture/skip behavior or assert has_migrachx.

As per path instructions, checked-in tests should protect against regressions rather than only report status.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_migrachx.py` around lines 52 - 64, Update
test_migrachx_importable so it fails when the migrachx import raises ImportError
instead of unconditionally asserting True. Reuse the existing fixture or skip
behavior if available; otherwise assert has_migrachx after the import attempt
while preserving the diagnostic version output.

Source: Path instructions

tests/amd/test_amd_rocm.py-195-203 (1)

195-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not mask invalid KV-cache reconstruction.

Replacing NaN/Inf values and zeroing their reference positions hides catastrophic quantization failures. Additionally, an error bound of < 2.0 for inputs in [-2, 2] allows an all-zero reconstruction to pass. Assert finite output first and use a meaningful absolute/relative tolerance.

As per path instructions, tests must exercise the behavior they claim to validate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_rocm.py` around lines 195 - 203, Update the KV-cache
roundtrip test around dequantize_kv_cache_fp8 to avoid nan_to_num and
reference-value masking: assert the reconstructed tensor is finite, then compare
it directly against k using meaningful absolute and relative tolerances that
reject an all-zero reconstruction. Preserve the test’s claimed FP8 roundtrip
coverage and ensure the assertions validate actual reconstruction accuracy.

Source: Path instructions

scripts/run_amd_benchmark.sh-3-3 (1)

3-3: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Implement the documented CLI options or remove them from usage.

--full, --llama-size, and --batch-sizes are never parsed; FULL is unused. For example, --llama-size 70b still benchmarks the default 7B configuration.

Also applies to: 9-12

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/run_amd_benchmark.sh` at line 3, Update scripts/run_amd_benchmark.sh
so the documented --full, --llama-size, and --batch-sizes options are actually
parsed and applied to the benchmark configuration, including using the requested
llama size instead of the default 7B value and wiring --full into the existing
FULL behavior; alternatively remove these options from the usage text if they
are not intended to be supported.

Source: Linters/SAST tools

scripts/amd_benchmark_ci.sh-19-25 (1)

19-25: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not make unit-test failures non-fatal.

|| echo ... converts any pytest failure into success, allowing the benchmark job to pass with broken functionality. Remove the fallback or explicitly preserve and return the pytest exit status.

Proposed correction
-    -q 2>&1 | tail -30 || echo "Some tests failed (non-fatal)"
+    -q 2>&1 | tail -30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/amd_benchmark_ci.sh` around lines 19 - 25, Make the pytest invocation
in the unit-test section fail the benchmark job when tests fail. Remove the
trailing non-fatal `|| echo "Some tests failed (non-fatal)"` fallback, or
otherwise preserve and return pytest’s exit status while retaining the existing
test arguments and output handling.
README_AMD_DEPLOYMENT.md-99-104 (1)

99-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Correct the LLaMA-70B KV-cache saving.

For these arguments, kv_cache_memory_savings calculates approximately 0.67 GB, not 6.7 GB, saved at batch size 1.

Proposed correction
-   # LLaMA-70B: saves ~6.7 GB per request at BS=1
+   # LLaMA-70B: saves ~0.67 GB per request at BS=1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README_AMD_DEPLOYMENT.md` around lines 99 - 104, Correct the LLaMA-70B
KV-cache savings comment in the README example from approximately 6.7 GB to 0.67
GB for the shown kv_cache_memory_savings arguments at batch size 1; leave the
code and surrounding explanation unchanged.
examples/amd_fp8_quickstart.py-77-96 (1)

77-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Benchmark equivalent FP16 and FP8 operations.

The FP16 loop times the entire three-GEMM FFN, while the FP8 loop times only gate. This guarantees an inflated speedup and invalid TFLOPS comparison. Either time model_fp16.gate(x) or deploy and time the complete FP8 FFN.

Minimal single-GEMM correction
-    for _ in range(WARMUP): model_fp16(x)
+    for _ in range(WARMUP): model_fp16.gate(x)
...
-    for _ in range(ITERS): model_fp16(x)
+    for _ in range(ITERS): model_fp16.gate(x)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/amd_fp8_quickstart.py` around lines 77 - 96, Update the FP16
benchmark in the timing section to call only model_fp16.gate(x), matching the
single-GEMM FP8 torch._scaled_mm operation. Keep warmup, synchronization,
iteration timing, and TFLOPS calculation otherwise unchanged so both paths
measure equivalent operations.
modelopt/torch/quantization/amd_model_card.py-52-66 (1)

52-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Calculate model size from actual tensor byte sizes.

All non-FP8/INT8 parameters are counted as FP16. FP32 parameters—and persistent quantization buffers excluded by named_parameters()—make the displayed size and memory reduction inaccurate. Sum actual stored tensor byte sizes, or explicitly validate and document the restricted dtype set.

Also applies to: 84-86

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_model_card.py` around lines 52 - 66, Update
the model-size calculation around fp8_params, int8_params, and total_size_gb to
sum each parameter’s actual stored bytes using tensor element counts and element
sizes, rather than treating every non-FP8/INT8 parameter as FP16. Include all
relevant model tensors or explicitly preserve and validate the supported dtype
set, ensuring FP32 parameters and persistent quantization buffers are
represented accurately in the displayed size and reduction calculations.
modelopt/torch/quantization/amd_calibration.py-42-42 (1)

42-42: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Move required ROCm compatibility imports to module scope.

These imports are neither optional nor documented circular dependencies, so import failures should surface when the module is loaded.

As per coding guidelines, “Keep imports at the top of Python source and test files; use local imports only for justified circular dependencies, optional dependencies, or unusually heavy imports.”

Also applies to: 81-81

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_calibration.py` at line 42, Move the ROCm
compatibility imports used by the calibration module, including is_rocm and
is_fp8_supported, to the module-level import section at the top of
amd_calibration.py. Remove the corresponding local imports while preserving all
existing call sites and behavior.

Source: Coding guidelines

modelopt/torch/quantization/amd_calibration.py-14-18 (1)

14-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Honor or remove the batch_size parameter.

The loop forwards each dataloader element unchanged, so batch_size has no effect despite being part of the public API. Chunk tensors accordingly or remove the argument.

Also applies to: 46-58

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_calibration.py` around lines 14 - 18, The
amd_calibrate function exposes a batch_size parameter that is currently ignored.
Update its dataloader processing loop to split each tensor into batches of
batch_size before forwarding, or remove batch_size from the public API and all
related usage; preserve the existing calibration behavior for each resulting
batch.
modelopt/_rocm_compat.py-1563-1618 (1)

1563-1618: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Implement or remove quantize_fp8.

The argument and docstring promise FP8 node insertion, but the value is never read and the function only runs generic ONNX optimizer passes. Callers can receive an unquantized model while believing FP8 optimization occurred.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 1563 - 1618, Update
optimize_onnx_for_migraphx to either implement the promised FP8 node insertion
when quantize_fp8 is enabled, or remove the quantize_fp8 parameter and its
related docstring promise. Ensure the function’s documented behavior matches the
actual optimization performed and callers cannot assume FP8 processing that does
not occur.
modelopt/_rocm_compat.py-2772-2785 (1)

2772-2785: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Mask overlapping tokens in the sliding-window perplexity calculation.

With stride < max_length, complete overlapping chunks are scored repeatedly. This biases NLL and perplexity; only newly introduced target tokens should contribute to each window.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 2772 - 2785, Update the sliding-window
loop in the perplexity calculation so each chunk’s loss counts only target
tokens newly introduced beyond the previous window, excluding overlapping tokens
when stride is less than max_length. Adjust the loss mask or token weighting
while preserving the existing handling for chunks shorter than two tokens and
the total_nll/total_tokens accumulation.
modelopt/_rocm_compat.py-1550-1555 (1)

1550-1555: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle partial hardware-counter dictionaries safely.

get_amd_hardware_counters() deliberately returns partial results, but formatting a missing value as {'N/A':.1f} raises ValueError. Format only numeric values.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 1550 - 1555, Update the
hardware-counter display block to format gpu_util_pct, vram_used_gb,
vram_total_gb, temperature_c, and power_w conditionally: apply numeric
formatting only when each value is numeric, otherwise render “N/A” without
numeric format specifiers. Preserve the existing labels, units, and
partial-result handling in get_amd_hardware_counters().
modelopt/_rocm_compat.py-235-251 (1)

235-251: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Implement the promised FP8Linear export conversion.

The comment says FP8 layers are temporarily converted for ONNX export, but no conversion or restoration occurs. Models containing _scaled_mm-based FP8Linear layers can therefore reach the exporter unchanged.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 235 - 251, Implement the promised
temporary FP8Linear conversion around the torch.onnx.export call: identify the
model’s FP8Linear layers, replace them with regular Linear-compatible layers for
export, and restore the original modules and state afterward, including when
export fails. Keep the conversion scoped to the export path and preserve the
model unchanged after completion.
modelopt/_rocm_compat.py-903-914 (1)

903-914: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Provide a fallback for small INT8 matrices.

The module documents that _int_mm requires M > 16, but INT8Linear.forward() calls it unconditionally. Single-token and small-batch inference can fail at runtime.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 903 - 914, The INT8Linear.forward
method calls torch._int_mm unconditionally even when its row count is too small.
Add a fallback for matrices with M <= 16 that performs the equivalent INT8
linear computation without _int_mm, while preserving the existing _int_mm path
for larger matrices and keeping output reshaping, dequantization, and bias
handling unchanged.
modelopt/_rocm_compat.py-430-447 (1)

430-447: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not construct the FP16 baseline with type(self.model)().

Most models require constructor arguments, and a newly initialized model does not preserve the original weights. Capture an FP16 baseline before calibration or require one explicitly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 430 - 447, Update the FP16 baseline
setup in the benchmarking method to avoid constructing a new model via
type(self.model)(). Reuse an FP16 baseline captured before calibration, or
require an explicitly supplied baseline, while preserving the existing baseline
timing and optimized-model timing flows.
modelopt/torch/quantization/amd_calibration.py-44-55 (1)

44-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not describe this round-trip as FP8 hardware execution.

The input is cast to FP8 and immediately restored to its original dtype before model(...); the model therefore executes its normal-dtype operators. Either use the actual FP8 kernel path or document this as FP8 input-rounding simulation.

Also applies to: 83-95

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_calibration.py` around lines 44 - 55, Update
the FP8 branch in the calibration loop to describe and treat the
cast-to-FP8-then-restored input as FP8 input-rounding simulation, not
hardware-accurate FP8 execution. Rename the misleading comment and related
identifiers such as use_fp8 if needed, while preserving the model(batch_fp8)
calibration flow; alternatively, replace it with an actual FP8 kernel path if
one already exists.
modelopt/_rocm_compat.py-872-893 (1)

872-893: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve the configured per-channel INT8 weight scales.

AMD_INT8_DEFAULT_CFG calibrates weights with axis: 0, but INT8Linear stores one scalar scale_w and re-quantizes using a global maximum. The deployment conversion therefore discards the preset’s per-output-channel calibration and can materially degrade accuracy.

Also applies to: 918-920, 932-958

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 872 - 893, The INT8Linear conversion
currently collapses per-output-channel calibration into one global scale. Update
INT8Linear.from_linear and its scale_w storage/use, including the related
forward or conversion paths around the referenced methods, to accept and
preserve axis-0 scale vectors from AMD_INT8_DEFAULT_CFG; only compute a global
fallback when no configured scales are provided, and broadcast channel scales
consistently during quantization and dequantization.
modelopt/_rocm_compat.py-2231-2284 (1)

2231-2284: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Complete each module’s explicit public API.

  • modelopt/_rocm_compat.py#L2231-L2284: add public APIs defined after this list, including FusedFP8Linear, convert_to_fused_fp8_linear, checkpoint, HuggingFace, attention, and validation helpers.
  • modelopt/torch/quantization/amd_calibration.py#L14-L19: export amd_calibrate.
  • modelopt/torch/quantization/amd_calibration.py#L62-L66: export get_amd_forward_loop.
  • modelopt/torch/kernels/quantization/gemm/amd_fp8_fused_mm.py#L30-L49: define __all__ for the public availability and matmul functions.

As per coding guidelines, “Define each module's public API with __all__ = [...].”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 2231 - 2284, Complete the explicit
public APIs: in modelopt/_rocm_compat.py lines 2231-2284, extend __all__ with
every public symbol defined afterward, including FusedFP8Linear,
convert_to_fused_fp8_linear, checkpoint, HuggingFace, attention, and validation
helpers; in modelopt/torch/quantization/amd_calibration.py lines 14-19 and
62-66, add amd_calibrate and get_amd_forward_loop to __all__; in
modelopt/torch/kernels/quantization/gemm/amd_fp8_fused_mm.py lines 30-49, define
__all__ containing the module’s public availability and matmul functions.

Source: Coding guidelines

modelopt/_rocm_compat.py-2147-2164 (1)

2147-2164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not implement “percentile” calibration as a pairwise mean.

The branch neither computes the documented 99th percentile nor a stable arithmetic mean; results depend on batch order. Implement percentile aggregation or reject unsupported algorithms.

Also applies to: 2206-2213

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 2147 - 2164, Update
calibrate_fp8_scales_batched so the "percentile" algorithm computes the
documented 99th-percentile aggregation across collected calibration values,
rather than averaging pairwise batch results. Ensure aggregation is
order-independent; alternatively, explicitly reject unsupported algorithm values
instead of using an incorrect fallback. Keep the existing "max" behavior
unchanged.
modelopt/_rocm_compat.py-1993-1997 (1)

1993-1997: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Correct the batch-size crossover scaling.

The stated examples require an inverse-square relationship: halving hidden size should increase the crossover from 64 to 256. The current formula produces 16 instead and selects full FP8 too early.

Proposed fix
-    est_breakeven_bs = max(1, int(64 * (H / 8192) ** 2))
+    est_breakeven_bs = max(1, int(64 * (8192 / H) ** 2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 1993 - 1997, Update the
est_breakeven_bs calculation in the crossover-selection logic to use
inverse-square scaling with hidden size, so H=8192 yields approximately 64 and
H=4096 yields approximately 256. Preserve the existing minimum batch-size guard
and ensure the resulting threshold does not select full FP8 prematurely.
modelopt/_rocm_compat.py-58-64 (1)

58-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fix the NVIDIA compute-capability comparison.

SM90 has major=9, minor=0, so the current condition returns false. Compare the tuple instead.

Proposed fix
-        return props.major >= 8 and props.minor >= 9
+        return (props.major, props.minor) >= (8, 9)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 58 - 64, Update the NVIDIA branch of
the FP8 capability check in the visible compatibility function to compare
(props.major, props.minor) lexicographically against (8, 9), so SM89 and
newer—including SM90—return true. Leave the ROCm architecture handling
unchanged.
modelopt/_rocm_compat.py-1943-1959 (1)

1943-1959: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Restrict “FFN-only” conversion to known FFN layers.

After attention names are skipped, leaf_name not in _ATTN_LAYER_NAMES is always true, so every remaining linear layer—including unknown projections and output heads—is converted.

Proposed fix
-        if leaf_name in _FFN_LAYER_NAMES or leaf_name not in _ATTN_LAYER_NAMES:
+        if leaf_name in _FFN_LAYER_NAMES:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 1943 - 1959, Restrict the conversion
condition in the FFN conversion block to leaf names explicitly present in
_FFN_LAYER_NAMES; remove the leaf_name not in _ATTN_LAYER_NAMES fallback.
Preserve the earlier attention-layer skip and convert only known FFN layers
through FP8Linear.from_linear.
🟡 Minor comments (8)
modelopt/torch/_deploy/_runtime/migrachx/__init__.py-5-7 (1)

5-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Re-export through the child module’s curated API.

Use from .migrachx_client import * and import its __all__ rather than duplicating a named export here. As per coding guidelines, “Re-export package submodules using from .module import *, relying on each module’s curated __all__.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/_deploy/_runtime/migrachx/__init__.py` around lines 5 - 7,
Update the package initializer to re-export the child module through its curated
API: replace the explicit MIGraphXLocalClient import and duplicated __all__ with
a wildcard import from migrachx_client, relying on that module’s __all__ while
preserving registry initialization.

Source: Coding guidelines

modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py-195-196 (1)

195-196: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Validate profiling iteration counts.

bench_iters=0 divides by zero; negative values produce an invalid latency. Reject negative warmup counts and require a positive benchmark count at this public boundary.

Proposed fix
         warmup = args.get("warmup_iters", 10)
         iters  = args.get("bench_iters", 100)
+        if not isinstance(warmup, int) or warmup < 0:
+            raise ValueError("warmup_iters must be a non-negative integer")
+        if not isinstance(iters, int) or iters <= 0:
+            raise ValueError("bench_iters must be a positive integer")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py` around lines 195
- 196, Validate the public profiling arguments in the code that reads
warmup_iters and bench_iters: reject negative warmup counts and require
bench_iters to be greater than zero before profiling proceeds. Preserve the
existing defaults and fail clearly at this boundary rather than allowing invalid
values into latency calculation.
tests/amd/test_amd_rocm.py-6-9 (1)

6-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the pytest paths.

The file is located at tests/amd/test_amd_rocm.py, but every example omits the amd/ directory.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/amd/test_amd_rocm.py` around lines 6 - 9, Correct all pytest command
examples in the test documentation to reference tests/amd/test_amd_rocm.py,
preserving their existing options and test-selection behavior.
README_AMD_DEPLOYMENT.md-3-4 (1)

3-4: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align the headline speedup with the reported results.

The table includes 1.63× and 1.64× results at batch size 256, contradicting the claimed 1.73–1.91× range for batch sizes ≥256. Qualify the workload subset or correct the range.

Also applies to: 46-53

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README_AMD_DEPLOYMENT.md` around lines 3 - 4, Update the
README_AMD_DEPLOYMENT.md headline and corresponding results summary so the
stated FP8/INT8 speedup accurately matches all reported batch-size ≥256
measurements, including the 1.63× and 1.64× entries; either broaden the range or
explicitly qualify the workload subset that achieves 1.73–1.91×.
modelopt/torch/quantization/amd_model_card.py-80-83 (1)

80-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include the INT8 layer count in the summary.

count_fp8_layers() reports int8_linear, but the card omits it despite presenting FP8/INT8 quantization details. Add an INT8 row so the layer breakdown matches the reported coverage and size.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_model_card.py` around lines 80 - 83, Update
the model card summary near the existing FP8 Linear layers row to include an
INT8 Linear layers row using layer_counts['int8_linear']. Keep the existing
formatting and neighboring FP16 and coverage rows unchanged.
modelopt/torch/quantization/amd_model_card.py-49-49 (1)

49-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not hardcode MI300X metadata.

arch is computed but ignored. This emits MI300X/ROCm results for MI325X and for NVIDIA FP8 devices accepted by is_fp8_supported(). Render detected platform/architecture dynamically, or reject non-ROCm callers at the interface.

Also applies to: 72-74, 100-103

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_model_card.py` at line 49, Update the
model-card generation logic around arch and the related metadata fields to use
the detected platform and GPU architecture rather than hardcoded MI300X/ROCm
values. Ensure NVIDIA FP8 devices and MI325X render their actual detected
metadata, or validate and reject non-ROCm callers at the public interface while
preserving valid ROCm behavior.
modelopt/_rocm_compat.py-1437-1470 (1)

1437-1470: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Always shut down AMD SMI after initialization.

The no-handle return and unexpected exceptions bypass amdsmi_shut_down(). Wrap post-initialization work in try/finally.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 1437 - 1470, Update the AMD SMI
handling around initialization and post-initialization work so
amdsmi.amdsmi_shut_down() always executes after amdsmi.amdsmi_init(), including
when no handles are returned or an unexpected exception occurs. Use a
try/finally structure while preserving the existing result collection and
empty-dictionary fallback behavior.
modelopt/_rocm_compat.py-1061-1074 (1)

1061-1074: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate quant_dtype before selecting cfg.

Any value other than "fp8" or "int8" leaves cfg unbound and crashes later.

Proposed fix
     import modelopt.torch.quantization as mtq
+
+    if quant_dtype not in {"fp8", "int8"}:
+        raise ValueError("quant_dtype must be 'fp8' or 'int8'")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 1061 - 1074, Validate quant_dtype in
the build_amd_inference_model flow before selecting cfg, accepting only "fp8" or
"int8" after the existing FP8 fallback. For any other value, raise a clear
validation error immediately rather than allowing cfg to remain unbound;
preserve the existing supported-type configuration and calibration behavior.
🧹 Nitpick comments (2)
modelopt/torch/quantization/amd_model_card.py (2)

17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Declare the module’s public API.

Add __all__ = ["generate_amd_model_card"] so package re-exports have an explicit contract.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_model_card.py` around lines 17 - 23, Add the
module-level __all__ declaration containing only generate_amd_model_card, making
this function the explicit public API while leaving the function signature and
implementation unchanged.

Source: Coding guidelines


44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document or remove the local imports.

These imports are not accompanied by the required reason for deferring them. Move them to module scope, or add a brief comment documenting the circular, optional, or heavy-import constraint.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/torch/quantization/amd_model_card.py` around lines 44 - 45, Update
the imports used by amd_model_card.py: move torch and the modelopt._rocm_compat
symbols to module scope, or retain them locally only with a brief comment
explaining the circular, optional, or heavy-import constraint that requires
deferral.

Source: Coding guidelines


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 852da568-ce31-4f5b-83a5-2b9bed92e341

📥 Commits

Reviewing files that changed from the base of the PR and between 9392dfe and d5bfb05.

📒 Files selected for processing (19)
  • README_AMD_DEPLOYMENT.md
  • README_ROCM.md
  • examples/amd_fp8_quickstart.py
  • examples/amd_llama_fp8_inference.py
  • modelopt/_rocm_compat.py
  • modelopt/torch/_deploy/_runtime/migrachx/__init__.py
  • modelopt/torch/_deploy/_runtime/migrachx/migrachx_client.py
  • modelopt/torch/kernels/quantization/gemm/amd_fp8_fused_mm.py
  • modelopt/torch/quantization/amd_calibration.py
  • modelopt/torch/quantization/amd_model_card.py
  • modelopt/torch/quantization/config.py
  • modelopt_recipes/configs/ptq/presets/model/amd_fp8.yaml
  • modelopt_recipes/configs/ptq/presets/model/amd_int8.yaml
  • scripts/amd_benchmark_ci.sbatch
  • scripts/amd_benchmark_ci.sh
  • scripts/run_amd_benchmark.sh
  • tests/amd/test_amd_integration.py
  • tests/amd/test_amd_migrachx.py
  • tests/amd/test_amd_rocm.py

Comment thread modelopt/_rocm_compat.py
Comment on lines +614 to +644
# Use static input scale if available (set by set_input_scale or calibration)
if hasattr(self, "scale_x") and self.scale_x is not None:
sx = self.scale_x
else:
# Dynamic scale: expensive amax computation (for calibration only)
sx = x.float().abs().max() / 448.0
sx = sx.clamp(min=1e-8)

# Fast path: direct FP8 cast (training-range inputs are already in [-448, 448])
# For deployment: inputs are in calibrated range, no overflow expected.
# For tests with random weights: use safe_mode=True (see set_input_scale)
# Handle N-D inputs (transformer models pass [B, T, H])
orig_shape = x.shape
if x.dim() > 2:
x = x.reshape(-1, x.shape[-1])

x_fp8 = x.contiguous().to(torch.float8_e4m3fnuz)

# torch._scaled_mm requires dims divisible by 16; fall back if not
if x_fp8.shape[0] % 16 == 0 and self.weight_fp8.shape[0] % 16 == 0:
out = torch._scaled_mm(
x_fp8, self.weight_fp8.T,
scale_a=sx.to(self.weight_fp8.device),
scale_b=self.scale_w,
out_dtype=torch.float16
)
else:
# Fallback: dequantize and use float16 matmul (non-aligned shapes)
w_f16 = self.weight_fp8.float() * float(self.scale_w)
x_f16 = x_fp8.float() * float(sx)
out = (x_f16 @ w_f16.T).to(torch.float16)

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.

🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

Apply inverse scales before casting to FP8.

These paths cast raw values to FP8 and then pass scale_a/scale_b as dequantization factors. Inputs must first be quantized as clamp(x / scale); otherwise calibrated scales multiply values that were never divided, corrupting outputs.

  • modelopt/_rocm_compat.py#L614-L644: divide and clamp x by sx before the FP8 cast in FP8Linear.
  • modelopt/torch/kernels/quantization/gemm/amd_fp8_fused_mm.py#L35-L43: perform the same quantization in the _scaled_mm fallback.
  • modelopt/_rocm_compat.py#L150-L159: inverse-scale raw x and W when the helper performs their FP8 conversion.
📍 Affects 2 files
  • modelopt/_rocm_compat.py#L614-L644 (this comment)
  • modelopt/torch/kernels/quantization/gemm/amd_fp8_fused_mm.py#L35-L43
  • modelopt/_rocm_compat.py#L150-L159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelopt/_rocm_compat.py` around lines 614 - 644, Apply inverse scaling
before every FP8 conversion: in modelopt/_rocm_compat.py lines 614-644, quantize
FP8Linear input as clamped x divided by sx before the cast; in
modelopt/torch/kernels/quantization/gemm/amd_fp8_fused_mm.py lines 35-43, apply
the same input quantization in the _scaled_mm fallback; and in
modelopt/_rocm_compat.py lines 150-159, inverse-scale both raw x and W before
the helper converts them to FP8.

#SBATCH --output=logs/ci-bench-%j.out
#SBATCH --error=logs/ci-bench-%j.err
IMAGE="aisdkshared/private:cluster2026-repro-verl-rocm7.0-20260518"
export DOCKER_CONFIG="/scratch/users/zhdu/.docker-auth"

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.

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Do not expose Docker registry credentials to the benchmark container.

Docker authenticates while pulling the image, before the container starts. Mounting ${DOCKER_CONFIG} at /root/.docker lets the image and checked-out PR code read registry credentials and potentially exfiltrate them over --network=host.

Proposed correction
-    -v "${DOCKER_CONFIG}:/root/.docker:ro" \

Also applies to: 16-19

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/amd_benchmark_ci.sbatch` at line 10, Remove the DOCKER_CONFIG export
and any related credential mount or environment propagation in the benchmark
submission setup, including the commands covering lines 16–19. Keep Docker
authentication available only to the host-side image pull, ensuring the launched
benchmark container cannot access registry credentials.

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