[CI] Upgrade to latest casual-conv1d and fix triton build for 3.4.x #551
Conversation
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
Warning Rate limit exceeded@zhiyuan1i has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update modifies four GitHub Actions workflow files. The build workflow now explicitly pulls the correct manylinux Docker image based on architecture and prunes unused images, removing an environment variable. The CI test workflow changes the Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions Workflow
participant Docker as Docker Daemon
participant PyPI as PyPI Repository
%% Triton Build Workflow
Workflow->>Docker: Pull manylinux image (x86_64 or aarch64)
Docker-->>Workflow: Image ready
Workflow->>Docker: Prune unused images
%% CI Test Workflow
Workflow->>PyPI: pip install -U causal-conv1d (if runner is nvidia-h100)
PyPI-->>Workflow: causal-conv1d installed/upgraded
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/reusable-build-triton.yml(1 hunks).github/workflows/reusable-ci-tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test H100 (PyTorch 2.7) / test-models
- GitHub Check: check-pt-python-compatibility
| # Image selection based on architecture input | ||
| if [[ "${{ inputs.arch }}" == 'x86_64' ]]; then | ||
| export CIBW_MANYLINUX_X86_64_IMAGE="quay.io/pypa/manylinux_2_28_x86_64:latest" | ||
| docker pull "${CIBW_MANYLINUX_X86_64_IMAGE}" | ||
| else | ||
| export CIBW_MANYLINUX_AARCH64_IMAGE="quay.io/pypa/manylinux_2_28_aarch64:latest" | ||
| docker pull "${CIBW_MANYLINUX_AARCH64_IMAGE}" | ||
| fi | ||
| docker image prune -f | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate arch and pin Docker images for reproducible builds
- Any value other than the literal string
x86_64will silently fall-through to theelsebranch and fetch anaarch64image, which is almost certainly unintended. - Using
:latestmakes the build non-deterministic; tomorrow’slatestmay contain breaking changes.
Consider:
+# Fail fast for unknown architectures
+case "${{ inputs.arch }}" in
+ x86_64)
+ export CIBW_MANYLINUX_X86_64_IMAGE="quay.io/pypa/manylinux_2_28_x86_64@sha256:<digest>"
+ docker pull "${CIBW_MANYLINUX_X86_64_IMAGE}"
+ ;;
+ aarch64)
+ export CIBW_MANYLINUX_AARCH64_IMAGE="quay.io/pypa/manylinux_2_28_aarch64@sha256:<digest>"
+ docker pull "${CIBW_MANYLINUX_AARCH64_IMAGE}"
+ ;;
+ *)
+ echo "::error::Unsupported arch: '${{ inputs.arch }}'"; exit 1 ;;
+esac
-# current unconditional prune remains fine
docker image prune -fPinning by digest guarantees byte-for-byte identical build roots, and the explicit case guard prevents accidental misuse.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/reusable-build-triton.yml around lines 184 to 193, the
current script uses a simple if-else to select Docker images based on the arch
input, defaulting to aarch64 for any non-x86_64 value, and uses the :latest tag
which is non-deterministic. To fix this, replace the if-else with a case
statement that explicitly handles only supported architectures and errors on
invalid inputs, and replace the :latest tags with pinned image digests to ensure
reproducible builds.
| $CONDA_BIN_PATH/pip install -U causal-conv1d --no-build-isolation | ||
| echo "Installing flash-attn for NVIDIA" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unpinned causal-conv1d may break CI reproducibility
Switching from a commit-pinned Git URL to pip install -U causal-conv1d installs whatever the PyPI tip is at run-time. A sudden upstream release (e.g., incompatible with the targeted CUDA or Torch version) will break all H100 CI jobs.
Recommend pinning an explicit version or constraint:
-$CONDA_BIN_PATH/pip install -U causal-conv1d --no-build-isolation
+$CONDA_BIN_PATH/pip install causal-conv1d==0.3.1 --no-build-isolation # ← adjust to the tested versionOptionally keep the -U flag with ~= or >=,< bounds if minor bumps are safe.
Ensure the chosen version is published for both CUDA 12.x wheels and source builds.
🏁 Script executed:
#!/bin/bash
# Search for occurrences of causal-conv1d in workflow files and codebase
echo "Searching for causal-conv1d in workflows:"
rg -n "causal-conv1d" .github/workflows
echo ""
echo "Searching for causal-conv1d in the entire repository:"
rg -n "causal-conv1d" .Length of output: 1452
Pin causal-conv1d in CI workflow to maintain reproducibility
The GitHub Actions step at .github/workflows/reusable-ci-tests.yml:333 currently does an unpinned install:
-$CONDA_BIN_PATH/pip install -U causal-conv1d --no-build-isolation
+$CONDA_BIN_PATH/pip install causal-conv1d==<tested-version> --no-build-isolation # e.g. 1.4.0• Replace -U causal-conv1d with an explicit ==<version> (or a bounded specifier like >=1.4.0,<1.5.0)
• Confirm that the chosen version has published CUDA 12.x wheels (or falls back to a compatible source build)
This change will lock CI to a known working release and prevent unexpected breakage from upstream PyPI updates.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $CONDA_BIN_PATH/pip install -U causal-conv1d --no-build-isolation | |
| echo "Installing flash-attn for NVIDIA" | |
| $CONDA_BIN_PATH/pip install causal-conv1d==<tested-version> --no-build-isolation # e.g. 1.4.0 | |
| echo "Installing flash-attn for NVIDIA" |
🤖 Prompt for AI Agents
In .github/workflows/reusable-ci-tests.yml at lines 333-334, the pip install
command for causal-conv1d uses an unpinned version, risking CI instability.
Modify the command to specify an explicit version or a bounded version range
(e.g., ==1.4.0 or >=1.4.0,<1.5.0) that is known to have CUDA 12.x compatible
wheels or a compatible source build. This will ensure reproducible and stable CI
runs.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.github/workflows/triton-nightly.yml (2)
49-49: Typo breaks the skip pattern –cp13tis not a valid wheel tag
cibw-skip: 'cp{35,36,37,38,39,13t}-*'will expand tocp13t-*, whichcibuildwheelwill just ignore, leaving CPython 3.13 wheels unskipped unintentionally.
It should becp313-*(or omit entirely if 3.13 is intended to be built).- cibw-skip: 'cp{35,36,37,38,39,13t}-*' + cibw-skip: 'cp{35,36,37,38,39}-*' # or add cp313 if you really mean to skip it
49-49: Skip list is inconsistent across the two jobsThe nightly job skips
cp35-cp39(plus the erroneouscp13t) while the manual single-version job skips onlycp35-cp39.
A single source of truth avoids diverging behaviour.Suggestion: extract the skip pattern into a reusable workflow input or YAML anchor so both jobs stay in lock-step.
Also applies to: 102-103
🧹 Nitpick comments (1)
.github/workflows/triton-builder.yml (1)
94-95: Hard-coding supported Python versions in multiple files invites driftYou now list
cp3{10,11,12,13}here, while other workflows repeat similar literals. When 3.14 lands you’ll need to touch every file.Consider defining an environment variable or reusable composite action (e.g.
python_supported_matrix) and referencing it via YAML anchors or workflow inputs.Keeps maintenance low and prevents accidental mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/triton-builder.yml(1 hunks).github/workflows/triton-nightly.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check-pt-python-compatibility
🔇 Additional comments (1)
.github/workflows/triton-nightly.yml (1)
39-40: Double-check the massive timeout bumpTimeouts jump from 2 h → 6 h (x86_64) and 12 h → 20 h (aarch64). Such long runners can hog the self-hosted fleet and silently mask infinite-loop failures.
Consider whether the build really needs that headroom and, if so, enforce a job-level
timeout-minutes:to fail fast when the limit is exceeded rather than keeping the runner busy for 20 hours.
1 participant
Summary by CodeRabbit