Conversation
📝 WalkthroughWalkthroughAdds a new GitHub Actions job Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/multi-gpu-e2e.yml(1 hunks).github/workflows/tests.yml(1 hunks)src/axolotl/cli/train.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
src/axolotl/cli/train.py
32-32: Local variable x is assigned to but never used
Remove assignment to unused variable x
(F841)
🪛 Flake8 (7.2.0)
src/axolotl/cli/train.py
[error] 32-32: local variable 'x' is assigned to but never used
(F841)
⏰ 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). (8)
- GitHub Check: PyTest (3.11, 2.7.1)
- GitHub Check: PyTest from Source Dist (3.11, 2.7.0)
- GitHub Check: PyTest from Source Dist (3.11, 2.7.1)
- GitHub Check: PyTest from Source Dist (3.11, 2.6.0)
- GitHub Check: PyTest (3.11, 2.6.0)
- GitHub Check: PyTest (3.11, 2.7.0)
- GitHub Check: test-axolotl-multigpu (126, 12.6.3, 3.11, 2.6.0, 2, true)
- GitHub Check: test-axolotl-multigpu (126, 12.6.3, 3.11, 2.7.0, 2, true)
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
shouldn't it have skipped the e2e tests for the latest commit?
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
.github/workflows/tests.yml (1)
1-333: Re-enable the CI workflow and implement robust “[skip-e2e]” gatingAll lines in .github/workflows/tests.yml are currently commented out, disabling every job (actionlint: “workflow is empty”) and breaking CI. Please restore the workflow’s core structure and add skip-logic that:
- Works on both push and pull_request events
- Honors “[skip-e2e]” and “[skip e2e]” in PR title, body, or labels
- Checks head_commit.message for non-PR events
- Applies the draft guard only on pull_request
Locations needing attention:
- File: .github/workflows/tests.yml (lines 1–333)
• Uncomment the top-levelname:,on:,jobs:, and all job definitions
• Restore the pre-commit, pytest, pytest-sdist, and docker-e2e jobsReplace each docker-e2e job’s
if:with:- if: ${{ ! contains(github.event.head_commit.message, '[skip-e2e]') && github.repository_owner == 'axolotl-ai-cloud' && !github.event.pull_request.draft }} + if: ${{ !( + (github.event_name == 'pull_request' && ( + contains(github.event.pull_request.title, '[skip-e2e]') || + contains(github.event.pull_request.body, '[skip-e2e]') || + contains(github.event.pull_request.labels.*.name, 'skip-e2e') || + contains(github.event.pull_request.title, '[skip e2e]') || + contains(github.event.pull_request.body, '[skip e2e]') || + contains(github.event.pull_request.labels.*.name, 'skip e2e') + )) || + (github.event_name != 'pull_request' && ( + contains(github.event.head_commit.message || '', '[skip-e2e]') || + contains(github.event.head_commit.message || '', '[skip e2e]') + )) + ) + && github.repository_owner == 'axolotl-ai-cloud' + && (github.event_name != 'pull_request' || !github.event.pull_request.draft) }}Apply the same block to both
docker-e2e-tests-1standdocker-e2e-testsjobs..github/workflows/multi-gpu-e2e.yml (1)
1-76: Critical: Workflow fully disabled; re-enable and harden skip gatingThe multi-GPU E2E workflow in
.github/workflows/multi-gpu-e2e.ymlis currently entirely commented out, so no jobs run—even when they should. Rather than hiding the problem, restore the workflow and update itsif:condition to reliably skip only when intended:• File:
.github/workflows/multi-gpu-e2e.yml
• Scope: job-levelif:fortest-axolotl-multigpuSuggested diff (replace the commented-out
if:at line 24):-# if: ${{ ! contains(github.event.commits[0].message, '[skip e2e]') && github.repository_owner == 'axolotl-ai-cloud' && (github.event_name != 'pull_request' || !github.event.pull_request.draft) }} + if: ${{ !( + (github.event_name == 'pull_request' && ( + contains(github.event.pull_request.title, '[skip-e2e]') || + contains(github.event.pull_request.body, '[skip-e2e]') || + contains(github.event.pull_request.labels.*.name, 'skip-e2e') || + contains(github.event.pull_request.title, '[skip e2e]') || + contains(github.event.pull_request.body, '[skip e2e]') || + contains(github.event.pull_request.labels.*.name, 'skip e2e') + )) || + (github.event_name != 'pull_request' && ( + contains(github.event.head_commit.message || '', '[skip-e2e]') || + contains(github.event.head_commit.message || '', '[skip e2e]') + )) + ) + && github.repository_owner == 'axolotl-ai-cloud' + && (github.event_name != 'pull_request' || !github.event.pull_request.draft) + }}Notes:
github.event.commits[0].messageis undefined for PRs and unreliable on pushes.- Use PR title/body/labels for PRs;
head_commit.messagefor pushes.After re-enabling, open a PR with only a
[skip-e2e]label (no code changes) to confirm the job skips, and verify it still runs onworkflow_dispatchor schedule when no skip marker is present.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/debug-commit-message.yml(1 hunks).github/workflows/multi-gpu-e2e.yml(1 hunks).github/workflows/tests.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SalmanMohammadi
PR: axolotl-ai-cloud/axolotl#2994
File: index.qmd:7-8
Timestamp: 2025-07-31T11:48:46.313Z
Learning: In documentation workflow testing PRs, temporary test content like "test test" may be intentionally added to trigger documentation preview actions and validate workflow fixes, rather than being accidental debug text.
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
1-1: workflow is empty
(syntax-check)
.github/workflows/debug-commit-message.yml
12-12: "github.event.head_commit.message" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
.github/workflows/multi-gpu-e2e.yml
1-1: workflow is empty
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/debug-commit-message.yml
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: lint
.github/workflows/debug-commit-message.yml
[error] 1-1: End-of-file-fixer: Files were modified to fix end-of-file newline.
[error] 1-1: Trailing-whitespace: Trailing whitespace removed by pre-commit hook.
Yes. It should be working now |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
201-217: Harden skip detection: include PR labels and accept “[skip e2e]” (legacy) tokenCovers more authoring styles without false positives, aligns with prior discussion, and avoids brittle reliance on commit message alone.
Apply this diff:
- const token = /\[skip-e2e\]/i; + const token = /\[skip-e2e\]/i; + const legacyToken = /\[skip e2e\]/i; let msg = ''; if (context.eventName === 'push') { msg = context.payload.head_commit?.message || ''; } else if (context.eventName === 'pull_request') { const { owner, repo } = context.repo; const prNumber = context.payload.pull_request.number; const commits = await github.paginate( github.rest.pulls.listCommits, { owner, repo, pull_number: prNumber, per_page: 100 } ); msg = commits.at(-1)?.commit?.message || ''; } const title = context.payload.pull_request?.title || ''; const body = context.payload.pull_request?.body || ''; - const skip = token.test(msg) || token.test(title) || token.test(body); + const labels = (context.payload.pull_request?.labels || []).map(l => l.name || ''); + const inLabels = labels.some(n => token.test(n) || legacyToken.test(n)); + const skip = + token.test(msg) || token.test(title) || token.test(body) || + legacyToken.test(msg) || legacyToken.test(title) || legacyToken.test(body) || + inLabels; core.setOutput('skip', String(skip));
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
192-192: Compute skip decision as early as possible by removing unnecessary needsThe gate job doesn’t require results from pre-commit/pytest to compute its output. Dropping this dependency yields earlier feedback and doesn’t change downstream gating (those jobs already need pre-commit/pytest).
Apply this diff:
- needs: [pre-commit, pytest, pytest-sdist] + # No needs here so we compute skip ASAP in parallel
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
226-226: label "modal" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
279-279: label "modal" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (4)
.github/workflows/tests.yml (4)
221-224: Good guard on PR-only fields and proper consumption of the gate outputCondition safely handles non-PR events and correctly skips when the gate indicates so.
228-229: Correct dependency wiringAdding gate-skip-e2e to needs ensures availability of its outputs for gating execution.
274-277: Consistent gating across the second E2E jobMirrors the same safe conditions as the first job; looks good.
283-283: Dependency ordering is soundRequiring the gate and the first e2e job prevents wasting GPU cycles when the gate decides to skip or when the first e2e fails.
|
Should it be the opposite for these tests? Should we only run it after an approve (via a comment by maintainer) rather than skip? |
The current behaviour isn't changed, this PR just allows for the slow tests to be skipped if they're not needed. |
|
Should we document this option to |
|
📖 Documentation Preview: https://689c813b7d1f3a81d35fa972--resonant-treacle-0fd729.netlify.app Deployed on Netlify from commit 1f66e06 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/CONTRIBUTING.md (2)
71-72: Replace placeholder code style linkThe
{codestyle}and{URLofCodestyle}placeholders appear unfinished. Replace with the actual style guide (e.g., Black/Ruff, or a local doc), or remove if not applicable.I can update this to reference your actual tooling (e.g., Black, Ruff, isort) if you confirm what the project uses.
81-82: Duplicate placeholder linkSame placeholder duplication here; replace with the real link or remove.
Happy to update once you confirm the intended target.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/CONTRIBUTING.md(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). (9)
- GitHub Check: pre-commit
- GitHub Check: PyTest from Source Dist (3.11, 2.7.1)
- GitHub Check: PyTest from Source Dist (3.11, 2.6.0)
- GitHub Check: PyTest (3.11, 2.7.1)
- GitHub Check: PyTest (3.11, 2.7.0)
- GitHub Check: PyTest from Source Dist (3.11, 2.7.0)
- GitHub Check: PyTest (3.11, 2.6.0)
- GitHub Check: preview
- GitHub Check: pre-commit
|
It might be worth adding this to the multi-gpu yaml as well |
3 participants
Summary by CodeRabbit
Chores
Tests
Documentation