[Feature] Support environments built by TheRock build system - #172
Conversation
This reverts commit 1909936.
There was a problem hiding this comment.
Pull request overview
This PR improves ROCm/HIP compatibility (including TheRock-built environments) by refactoring ROCm environment detection, making CUDA-specific imports conditional, and improving HIP compilation/header compatibility, while also adding a test-time GPU memory cleanup fixture.
Changes:
- Refactor ROCm detection utilities to support TheRock layouts and centralize
ROCM_HOMEresolution. - Make CUDA-only imports in
prefill.pyconditional to avoid import-time failures on ROCm/CPU-only builds. - Update ROCm sampling extension to include the appropriate PyTorch generator header based on availability.
- Add an autouse pytest fixture to opportunistically clear GPU memory between tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/conftest.py | Adds an autouse GPU memory cleanup fixture and adjusts cache-clearing threshold logic. |
| flashinfer/prefill.py | Gates CUDA-only imports behind torch.version.cuda to improve ROCm/CPU importability. |
| flashinfer/jit/cpp_ext_hip.py | Switches ROCm home discovery to flashinfer.hip_utils.get_rocm_home() and simplifies ninja generation. |
| flashinfer/hip_utils.py | Adds TheRock detection + multiple ROCm version detection methods; refactors compatibility matrix construction. |
| flashinfer/csrc_rocm/sampling.cu | Selects HIP vs CUDA generator header using __has_include for better cross-torch compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/conftest.py:232
pytest_runtest_callis implemented as a generator (usesyield) but the hook decorator no longer setswrapper=True. Withoutwrapper=True, pytest will not treat this as a hook wrapper and this will break test execution/exception handling. Setwrapper=Trueagain (you can keeptryfirst=Trueif needed).
@pytest.hookimpl(tryfirst=True)
def pytest_runtest_call(item):
# skip OOM error and missing JIT cache errors
try:
yield
except (torch.cuda.OutOfMemoryError, RuntimeError) as e:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import os | ||
| import re | ||
| import subprocess | ||
|
|
||
| # Method 1: Try /opt/rocm/.info/version (most reliable) | ||
| rocm_path = os.environ.get("ROCM_PATH", "/opt/rocm") | ||
| version_file = os.path.join(rocm_path, ".info", "version") | ||
| return os.environ.get("ROCM_PATH") or os.environ.get("ROCM_HOME") or "/opt/rocm" |
There was a problem hiding this comment.
get_rocm_home() falls back to /opt/rocm without attempting any discovery when neither ROCM_PATH nor ROCM_HOME is set. This can break environments where ROCm is installed in a non-standard prefix (including TheRock wheel installs) unless users also set one of those env vars. Consider adding an additional fallback that derives the prefix from PYTORCH_AMDCLANG (if set) and/or from a hipcc/amdclang++ found on PATH, before defaulting to /opt/rocm.
| return version | ||
| print(f"ROCm version not found using {method.__name__}. Trying next method...") | ||
|
|
||
| return None | ||
|
|
||
|
|
There was a problem hiding this comment.
get_system_rocm_version() prints a message for every failed detection method attempt. This function is part of core validation and may run in library contexts where stdout noise is undesirable (e.g., tests, JIT compilation). Please route this through the project's logger at DEBUG level or gate it behind an explicit verbose/env flag instead of unconditional print().
ff0328e
into
AMD-Ecosystem:amd-integration
📌 Description
This PR introduces ROCm/HIP compatibility improvements:
hip_utils.pyfor supporting environments built by TheRock build systemOBS!
export PYTORCH_AMDCLANG=<path-to-amdclang++>.🧪 Tests
Currently failing tests:
Above similarity scores fall just below the 0.99 threshold (< 2% deviation).
Test environment: