Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces ROCm CI support, refactors the GPU CI scripts to be more configurable, updates Dockerfiles and project dependencies, and adds tests for CUDA SM90 backend prioritization. The reviewer feedback focuses on improving the robustness of the CI scripts, such as installing the correct optional dependencies (cuda and rocm) during testing, pre-installing flashinfer in the CUDA Docker image to avoid slow compilation, handling shell failures under set -euo pipefail when detecting Python, and preventing Git fetch failures when running scripts locally. Additionally, the reviewer suggests making ROCm test arguments configurable and robustifying the device capability mock in tests.
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.
| git fetch origin "$PR_SHA" | ||
| git checkout --detach "$PR_SHA" | ||
| "$PY" -m pip install -U pip setuptools wheel | ||
| "$PY" -m pip install -e ".[test]" |
There was a problem hiding this comment.
The GPU CI script currently installs .[test], which omits the cuda optional dependencies (such as flashinfer and nvidia-ml-py). To ensure that the CUDA-specific optimized backends are installed and tested, we should install .[cuda,test] instead.
| "$PY" -m pip install -e ".[test]" | |
| "$PY" -m pip install -e ".[cuda,test]" |
| FLASH_AUTO_INSTALL="${RL_KERNEL_ROCM_FLASH_ATTN_AUTO_INSTALL:-1}" | ||
|
|
||
| "$PY" -m pip install -U pip setuptools wheel | ||
| "$PY" -m pip install -e ".[test]" |
There was a problem hiding this comment.
The ROCm CI script currently installs .[test], which omits the rocm optional dependencies (such as aiter). To ensure that the ROCm-specific optimized backends are installed and tested, we should install .[rocm,test] instead.
| "$PY" -m pip install -e ".[test]" | |
| "$PY" -m pip install -e ".[rocm,test]" |
| RUN python -m pip install -U pip setuptools wheel \ | ||
| && python -m pip install -r requirements.txt \ | ||
| && python -m pip install \ | ||
| black \ | ||
| isort \ | ||
| mypy \ | ||
| packaging \ | ||
| pre-commit \ | ||
| psutil \ | ||
| pytest \ | ||
| ruff |
There was a problem hiding this comment.
flashinfer does not publish pre-built wheels to PyPI directly, only source distributions. If it is not pre-installed in the Docker image, running pip install will attempt to compile it from source, which takes a very long time and often fails or times out in CI. Pre-installing it in the Docker image using the official wheel index for CUDA 12.4 and PyTorch 2.4 avoids this issue completely.
RUN python -m pip install -U pip setuptools wheel \
&& python -m pip install -r requirements.txt \
&& python -m pip install flashinfer -f https://flashinfer.ai/whl/cu124/torch2.4/index.html \
&& python -m pip install \
black \
isort \
mypy \
packaging \
pre-commit \
psutil \
pytest \
ruff
| PR_SHA="${PR_SHA:-$(date +%s)}" | ||
| POD_NAME="rl-kernel-ci-${PR_SHA:0:7}" | ||
| READY_RETRIES=60 | ||
| PROFILE_SLUG=$(printf "%s" "${RUNPOD_PROFILE_NAME:-gpu}" | tr -c "[:alnum:]-" "-") | ||
| POD_NAME="rl-kernel-ci-${PR_SHA:0:7}-${PROFILE_SLUG}" |
There was a problem hiding this comment.
When running this script locally or manually without PR_SHA set, the fallback defaults to a Unix timestamp. This causes git fetch origin <timestamp> to fail later in the script because a timestamp is not a valid git ref. We can use a separate variable for the unique pod name while defaulting PR_SHA to main or HEAD for the git checkout.
| PR_SHA="${PR_SHA:-$(date +%s)}" | |
| POD_NAME="rl-kernel-ci-${PR_SHA:0:7}" | |
| READY_RETRIES=60 | |
| PROFILE_SLUG=$(printf "%s" "${RUNPOD_PROFILE_NAME:-gpu}" | tr -c "[:alnum:]-" "-") | |
| POD_NAME="rl-kernel-ci-${PR_SHA:0:7}-${PROFILE_SLUG}" | |
| PR_SHA="${PR_SHA:-}" | |
| if [ -z "$PR_SHA" ]; then | |
| PR_SHA_FOR_POD=$(date +%s) | |
| PR_SHA="main" | |
| else | |
| PR_SHA_FOR_POD="$PR_SHA" | |
| fi | |
| PROFILE_SLUG=$(printf "%s" "${RUNPOD_PROFILE_NAME:-gpu}" | tr -c "[:alnum:]-" "-") | |
| POD_NAME="rl-kernel-ci-${PR_SHA_FOR_POD:0:7}-${PROFILE_SLUG}" |
| PY=$(command -v python3.11 || command -v python3) | ||
| if [ -z "$PY" ]; then echo "[remote] FATAL: python not found in PATH"; exit 127; fi |
There was a problem hiding this comment.
Under set -euo pipefail, if neither python3.11 nor python3 is found, the command substitution $(command -v python3.11 || command -v python3) will return a non-zero exit code and cause the shell to exit immediately. This prevents the script from reaching the if [ -z "$PY" ] check and printing the helpful error message. Appending || true inside the command substitution resolves this.
| PY=$(command -v python3.11 || command -v python3) | |
| if [ -z "$PY" ]; then echo "[remote] FATAL: python not found in PATH"; exit 127; fi | |
| PY=$(command -v python3.11 || command -v python3 || true) | |
| if [ -z "$PY" ]; then echo "[remote] FATAL: python not found in PATH"; exit 127; fi |
| PY="${PYTHON:-$(command -v python3 || command -v python)}" | ||
| if [ -z "$PY" ]; then | ||
| echo "[rocm-ci] FATAL: python not found in PATH" | ||
| exit 127 | ||
| fi |
There was a problem hiding this comment.
Similar to the GPU CI script, under set -euo pipefail, if neither python3 nor python is found, the command substitution will fail and cause the script to exit immediately, bypassing the helpful error message. Appending || true inside the command substitution ensures the error check is reached.
| PY="${PYTHON:-$(command -v python3 || command -v python)}" | |
| if [ -z "$PY" ]; then | |
| echo "[rocm-ci] FATAL: python not found in PATH" | |
| exit 127 | |
| fi | |
| PY="${PYTHON:-$(command -v python3 || command -v python || true)}" | |
| if [ -z "$PY" ]; then | |
| echo "[rocm-ci] FATAL: python not found in PATH" | |
| exit 127 | |
| fi |
| "$PY" -m pytest \ | ||
| rl_engine/tests/test_dispatch.py \ | ||
| tests/test_kernel_registry.py \ | ||
| tests/test_attention_correctness.py \ | ||
| tests/test_linear_logp.py \ | ||
| tests/test_ratio_kl.py \ | ||
| -q -rs |
There was a problem hiding this comment.
The list of test files is currently hardcoded in the ROCm CI script. Allowing PYTEST_ARGS to be overridden or appended to (similar to run_gpu_ci.sh) makes the script much more flexible for local development and custom CI runs.
| "$PY" -m pytest \ | |
| rl_engine/tests/test_dispatch.py \ | |
| tests/test_kernel_registry.py \ | |
| tests/test_attention_correctness.py \ | |
| tests/test_linear_logp.py \ | |
| tests/test_ratio_kl.py \ | |
| -q -rs | |
| PYTEST_ARGS="${PYTEST_ARGS:-rl_engine/tests/test_dispatch.py tests/test_kernel_registry.py tests/test_attention_correctness.py tests/test_linear_logp.py tests/test_ratio_kl.py -q -rs}" | |
| "$PY" -m pytest $PYTEST_ARGS |
| monkeypatch.delenv("RL_KERNEL_ROCM_ATTN_BACKEND", raising=False) | ||
| monkeypatch.setattr(registry_module.device_ctx, "device_type", "cuda") | ||
| monkeypatch.setattr(registry_module.device_ctx, "is_rocm", False) | ||
| monkeypatch.setattr(torch.cuda, "get_device_capability", lambda: capability) |
There was a problem hiding this comment.
The mock for torch.cuda.get_device_capability is defined as lambda: capability, which takes 0 arguments. However, PyTorch's get_device_capability can accept a device argument (e.g., torch.cuda.get_device_capability(device)). Defining the mock with *args, **kwargs makes it more robust against future changes or other call sites.
| monkeypatch.setattr(torch.cuda, "get_device_capability", lambda: capability) | |
| monkeypatch.setattr(torch.cuda, "get_device_capability", lambda *args, **kwargs: capability) |
ad05648 to
0b6b8f1
Compare
Security (Critical): - rocm-self-hosted now checks out base branch (trusted scripts) instead of fork HEAD; run_rocm_ci.sh clones PR code into /tmp at runtime via PR_REPO_URL + PR_SHA — mirrors the cuda-runpod isolation pattern, so fork-controlled code never executes on the self-hosted runner directly. Correctness: - Restore torch.distributed.run for multi-GPU pytest: GPU_COUNT > 1 now runs `python -m torch.distributed.run --nproc_per_node=$GPU_COUNT -m pytest` so distributed code paths are actually exercised. - Install .[cuda,test,hf] instead of .[cuda,test] so transformers is present and test_stateless_hf_integration.py runs instead of silently skipping. Reliability: - Add set -e to run_gpu_ci.sh outer script (was set -uo pipefail only); pre-SSH failures now abort immediately instead of continuing with stale state. - Restore ENV TORCH_CUDA_ARCH_LIST="8.6" in Dockerfile.cuda so headless / CPU-only docker builds have a sensible default and don't fail or compile fat binaries for every arch. - Add cache-to to build-pr job in build-ci-image.yml so PR Docker builds warm the GHA cache; previously only main-branch pushes wrote the cache, leaving all PR builds cold. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Blocking: - Dockerfile.cuda: fix TORCH_CUDA_ARCH_LIST --build-arg passthrough (ENV ignored build-arg; now uses ARG+ENV pair) - pyproject.toml + setup.py: remove wrong 'aiter' PyPI package from rocm extra (that package is an unrelated async-iterator library); amd-aiter must be installed from source — see installation docs - ci/run_rocm_ci.sh: drop .[rocm,test] since rocm extra is now empty; install .[test] only - ci.yml: add CPU-only operator tests — test_linear_logp.py native tests and test_cpu_hal.py now run in every PR (regressions in NativeLinearLogpOp and HAL routing were previously undetected) Quality: - run_gpu_ci.sh: remove torch.distributed.run wrapping around pytest; tests use CUDA directly, not dist — running under distributed.run caused the full suite to execute once per rank (wasted cost and GPU contention) - run_gpu_ci.sh: tighten fallback POD_ID regex to stay anchored to "id" field, preventing accidental SHA/field matches - run_rocm_ci.sh: add informative error messages on git clone/fetch failure - Dockerfile.rocm: add ninja to pip installs (needed by flash-attn build) - docs/contributing/testing.md: fix Docker CUDA run example to use .[cuda,test]; add required secrets table (RUNPOD_API_KEY, RUNPOD_SSH_PRIVATE_KEY) with RunPod SSH key registration note - docs/getting_started/installation.md: add Testing section linking to contributing/testing.md so source-build contributors find the Docker docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GitHub blocks GHA cache writes from fork-triggered pull_request events. The build-pr job only needs cache-from; cache-to belongs in build-and-push (trusted push-to-main context) where it already exists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds CUDA and ROCm source-build images, label-gated hardware workflows, and local docs for running the same checks.\n\nThis fork PR is for validating the GitHub-hosted workflows before opening upstream.