run codecov action at end of CI; only_pulls: true #2664
Conversation
djsaunde
force-pushed
the
codecov-pulls-only
branch
from
9e98a81 to
fb4ce2a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
WalkthroughThe continuous integration workflow was restructured to aggregate and upload test coverage data in a dedicated post-processing job, rather than uploading from individual test jobs. Supporting scripts were updated to extract, return, and persist coverage reports, while direct coverage uploads from within scripts were removed. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
.github/workflows/tests.yml(4 hunks)cicd/cicd.sh(0 hunks)
💤 Files with no reviewable changes (1)
- cicd/cicd.sh
🔇 Additional comments (3)
.github/workflows/tests.yml (3)
210-215: Artifact upload for PyTest job is correctly configured
The newactions/upload-artifactstep will reliably capturecoverage.xmlperpytorch_versionandgithub.run_id, ensuring unique artifact names. Retention of 1 day is sufficient for downstream aggregation.
338-345: Artifact upload for first Docker E2E tests is correctly configured
TheUpload coverage artifactsstep withif: always()guarantees coverage files from the initial E2E run (e2e-coverage.xml) are always preserved, even on failures.
400-407: Artifact upload for Docker E2E tests is correctly configured
Similarly, this upload step capturese2e-coverage.xmlfor the main E2E matrix. The naming convention includes CUDA and PyTorch versions, ensuring uniqueness.
djsaunde
force-pushed
the
codecov-pulls-only
branch
from
2b12260 to
639ddef
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
355-361: Condition still runs on pushes tomain– contradicts PR goal
Same issue flagged earlier: the job should execute only for PRs per the PR title/description.- if: github.event_name == 'pull_request' || github.ref == 'refs/heads/main' + if: github.event_name == 'pull_request'
🧹 Nitpick comments (2)
cicd/multigpu.py (1)
80-85: Path hard-coding is acceptable but consider centralising the constantThe logic correctly returns coverage data when present.
If more GPU test scripts adopt the same pattern, factor the coverage path (/workspace/axolotl/multigpu-coverage.xml) into a shared constant or helper to avoid divergence in the future.docs/config-reference.qmd (1)
6-25: Move type hints out of value position to keep the snippet valid YAMLPlacing
str | None,bool | None = False, or(required)after the colon makes the file invalid for anyone who tries to load it with a YAML parser.
Recommend:
- Keep just the default value (or
null) after the colon.- Convert the type information into preceding
#comments or a rendered table.- Add a short, visible note at the top that this block is illustrative and not meant to be parsed verbatim.
Example pattern:
# strict : bool | None # default: false strict: falseThis preserves readability while staying machine-parsable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/tests.yml(4 hunks)cicd/cicd.sh(0 hunks)cicd/e2e_tests.py(2 hunks)cicd/multigpu.py(1 hunks)docs/config-reference.qmd(1 hunks)
💤 Files with no reviewable changes (1)
- cicd/cicd.sh
🧰 Additional context used
🧬 Code Graph Analysis (2)
cicd/multigpu.py (1)
cicd/e2e_tests.py (2)
main(28-34)cicd_pytest(16-24)
cicd/e2e_tests.py (1)
cicd/single_gpu.py (1)
run_cmd(63-68)
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
374-374: input "carryforward" is not defined in action "codecov/codecov-action@v5". available inputs are "binary", "codecov_yml_path", "commit_parent", "directory", "disable_file_fixes", "disable_safe_directory", "disable_search", "dry_run", "env_vars", "exclude", "fail_ci_if_error", "files", "flags", "gcov_args", "gcov_executable", "gcov_ignore", "gcov_include", "git_service", "handle_no_reports_found", "job_code", "name", "network_filter", "network_prefix", "os", "override_branch", "override_build", "override_build_url", "override_commit", "override_pr", "plugins", "report_code", "report_type", "root_dir", "skip_validation", "slug", "swift_project", "token", "url", "use_legacy_upload_endpoint", "use_oidc", "verbose", "version", "working-directory"
(action)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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 from Source Dist (3.11, 2.5.1)
- GitHub Check: PyTest (3.11, 2.6.0)
- GitHub Check: PyTest (3.11, 2.7.1)
- GitHub Check: PyTest (3.11, 2.5.1)
- GitHub Check: pre-commit
- GitHub Check: pre-commit
- GitHub Check: preview
🔇 Additional comments (6)
cicd/multigpu.py (1)
89-94: Remote result handling looks good
coverage = cicd_pytest.remote()is consistent with other Modal invocations and the file-write block safely guards against missing coverage. No issues spotted.cicd/e2e_tests.py (2)
20-25: Mirrors multigpu pattern – OKCoverage extraction logic is identical to
cicd/multigpu.py, keeping behaviour consistent across test suites. Looks good.
29-34: LGTM – file write guard is sufficientLocal persistence of the XML is gated behind the truthiness check, preventing empty files. No further action required.
.github/workflows/tests.yml (3)
109-115: Artifact upload step works but relies on a singlecoverage.xmlRunning three
pytestinvocations in the same job produces a single mergedcoverage.xml, so the path is fine. Just ensure future splits (e.g., sharded tests) keep writing to the same file or adjust the artifact list accordingly.
236-243: Good use ofif: always()This ensures coverage is still collected for failing E2E runs – nice touch.
307-314: Consistent artefact namingIncluding
run_idavoids collisions between concurrent CI runs. Looks solid.
Description
Motivation and Context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Social Handles (Optional)
Summary by CodeRabbit