Repository navigation
Add RFC0014 for code coverage infrastructure in TheRock - #6967
Merged
Merged
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
therock-pr-bot
Bot
added
the
Not ready to Review
|
🎉 All checks passed! This PR is ready for review. |
reboss
force-pushed
the
users/jorobbin/code-cov-rfc
branch
from
July 29, 2026 17:00
9b63d52 to
805a2e8
Compare
ScottTodd
self-requested a review
July 29, 2026 17:28
ScottTodd
reviewed
Jul 31, 2026
ScottTodd
left a comment
ScottTodd
left a comment
Member
There was a problem hiding this comment.
My main reservations here are about what the CI job graph and uploaded artifacts will look like if we need to rebuild each component project with different flags. Some metrics from experiments and graphs/diagrams of the proposed architecture could help in discussing that.
malcolmroberts
suggested changes
Aug 7, 2026
malcolmroberts
left a comment
malcolmroberts
left a comment
Contributor
There was a problem hiding this comment.
There are some dangerous assumptions in this document.
ScottTodd
self-requested a review
August 11, 2026 17:19
malcolmroberts
suggested changes
Aug 19, 2026
malcolmroberts
left a comment
malcolmroberts
left a comment
Contributor
There was a problem hiding this comment.
Some discussion left on this.
reboss
force-pushed
the
users/jorobbin/code-cov-rfc
branch
from
August 20, 2026 18:02
69ea204 to
5e395bd
Compare
malcolmroberts
self-requested a review
August 20, 2026 21:09
malcolmroberts
approved these changes
Aug 20, 2026
malcolmroberts
left a comment
malcolmroberts
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the edits!
I think that this is workable now. I do feel like we need phased implementation to demonstrate that we have the infrastructure available (eg multi-gpu, multi-arch, mocked upstream dependencies) before things are gating.
ScottTodd
approved these changes
Aug 24, 2026
ScottTodd
left a comment
ScottTodd
left a comment
Member
There was a problem hiding this comment.
Generally LGTM, but I'd like to see more focus in the implementation/rollout plan on the standard ordering from low frequency to high frequency workflow runs:
- on-demand (workflow_dispatch)
- nightly (schedule)
- push
- pull_request
Until we have concrete data on how long these jobs take to run it's hard to commit to running them on every pull request.
reboss
force-pushed
the
users/jorobbin/code-cov-rfc
branch
from
August 26, 2026 18:05
8da4ee1 to
13c218e
Compare
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
Proposes per-project coverage builds with isolation to prevent profraw contamination from upstream dependencies, maintaining separate coverage pipeline from pre-checkin testing.
Emphasize that coverage in one component doesn't affect coverage in downstream components, balancing the upstream contamination argument.
Add detail that -object flag doesn't accept wildcards (test_*), requiring explicit expansion. Propose declarative approach with standard test naming convention as an open discussion topic.
Focus RFC on design description rather than step-by-step process. Conform to existing RFC format in repo.
Explain how script detects changed projects, handles multi-component PRs, uses whitelist, and outputs per-project coverage metadata with pinned builds.
Explain schema fields (llvm_profile_file, ignore-filename-regex, test_names), how they can be calculated declaratively, mixed library type handling, and error handling when profraw files are missing.
Note that codecov.io is already configured in Math CI, and specific provider choice/configuration is outside scope of this RFC.
Note single-architecture scope, per-project node allocation, and that specific sizing/timing metrics are open discussion topics.
Coverage test failures block PRs like any other test failure. Component owners can disable flaky tests.
Consolidated all detail additions from today's work session.
Address PR feedback about architecture-specific code coverage needs. Define 5-phase rollout from single default arch to intelligent multi-arch testing with change detection and report aggregation.
Separate three distinct specialized coverage scenarios: - Multi-architecture: different GPU architectures (gfx90a vs gfx942) - Multi-GPU: code requiring multiple GPUs - Mock-based: error handling paths needing upstream error injection Mock coverage can integrate with default arch; multi-GPU requires separate detection and node allocation.
Explain four approaches for passing coverage flags from TheRock to subprojects. Recommend list-based options (3/4) for CI integration since they accept case-insensitive project names and TheRock handles conversion internally, removing formatting burden from callers.
The old script has been replaced by post_stage_upload.py for multi-arch CI.
Add -coverage suffix to coverage artifacts to prevent confusion with regular builds, similar to -asan suffix for ASAN builds.
- Add coverage aggregation job that merges profraw from all test shards - Test shards upload profraw artifacts, final node aggregates and reports - Update node allocation to reflect multi-stage builds and test sharding - Coverage builds can leverage multi-arch CI pipeline with staged builds
Code coverage depends on amd-llvm's profiling implementation. A change in 7.14 broke coverage generation across all projects. Require smoke tests to gatekeep amd-llvm changes before merge.
Use consistent matrix.shard variable reference instead of SHARD_INDEX placeholder in artifact naming documentation.
Nightly runs follow phased approach: - Phase 1: Full coverage on default architecture - Phase 2: Change-based component selection - Phase 3: Multi-architecture expansion Nightly uses hybrid artifacts: - Single instrumented stack build (all components) - Separate test jobs per component - Each test pulls: instrumented-under-test + non-instrumented-dependencies - Maintains per-project isolation while amortizing build cost Added coverage-for-all flags: - THEROCK_COVERAGE_ROCM_LIBRARIES_ALL - THEROCK_COVERAGE_ROCM_SYSTEMS_ALL - THEROCK_COVERAGE_ALL PR coverage remains unchanged: only instrument changed projects.
Nightly coverage builds instrument entire stack but test components separately. Each test job fetches: - Instrumented component from coverage build (-coverage suffix) - Non-instrumented dependencies from regular nightly build Uses artifact_manager.py with: - Target family suffix: gfx942-coverage vs gfx942 - --artifact-names / --exclude-artifact-names for selective fetch PR coverage doesn't need this - dependencies naturally non-instrumented.
… questions BuildTopology defines grouped artifacts (blas, prim) but coverage requires per-project isolation (rocblas, hipblaslt, hiprand, rocrand, etc.). Options: per-project artifacts, component filtering, or parallel system. Removed answered questions: - Nightly frequency (addressed: phased rollout) - Coverage thresholds (component-level decision) - Test naming (declarative discovery handles this) - Artifact retention (standard ops concern) - Report aggregation dashboard (out of scope) - Multi-arch nightly cadence (addressed in Phase 3)
Two viable nightly coverage architectures: Option A: Extend regular nightly (same run-id) - Both regular + coverage builds in single nightly run - Suffix REQUIRED for same-directory artifact differentiation - Pros: guaranteed consistency, simpler coordination, no baseline tracking - Cons: longer runtime, resource contention, coupled execution Option B: Separate workflow (different run-id) - Independent coverage workflow references regular nightly baseline - Suffix recommended but technically optional (different S3 dirs) - Pros: independent execution, flexible scheduling, isolated failures - Cons: baseline coordination needed, potential inconsistency Phased roadmap: - Phase 1: Option B1 (manual baseline_run_id input) - Phase 2: Option B2/B3 (automated via GitHub API or orchestrator) - Phase 3: Option A (merge with regular nightly if beneficial) PR coverage: Follows ASAN pattern (separate workflow, no suffix needed) Addresses PR comment about why ASAN doesn't use artifact suffixes.
Added detailed workflow structure showing how downstream trigger passes baseline_run_id automatically. Documents adaptation to nightly instability (coverage runs even when regular nightly tests fail) and trade-offs vs Options A and B.
…tegy Nightly coverage no longer needs -coverage suffix since Option C uses downstream trigger with separate run IDs. Documented multi-step extraction: fetch all non-instrumented artifacts, then fetch grouped instrumented component, extract only specific project files (lib/dev/test), overwrite non-instrumented version. Works with BUILD_TOPOLOGY grouped artifacts while achieving per-project instrumentation isolation.
Flag has been default-enabled for months and is now retired. Removed from amd-llvm requirements and coverage build example.
Status: draft → accepted Discussion: in progress → complete
reboss
force-pushed
the
users/jorobbin/code-cov-rfc
branch
from
September 14, 2026 17:11
0aff662 to
cb2f58e
Compare
therock-pr-bot
Bot
removed
the
Not ready to Review
reboss
changed the title
saadrahim
reviewed
Sep 21, 2026
|
|
||
| ### Problematic: Header-Only Static Libraries | ||
|
|
||
| Some components are header-only static libraries (e.g., rocPRIM, hipCUB) and do not produce a simple `.so` file for `llvm-cov show -object <lib>.so`. |
Member
There was a problem hiding this comment.
Suggested change
| Some components are header-only static libraries (e.g., rocPRIM, hipCUB) and do not produce a simple `.so` file for `llvm-cov show -object <lib>.so`. | |
| Some components are header-only libraries (e.g., rocPRIM, hipCUB) and do not produce a simple `.so` file for `llvm-cov show -object <lib>.so`. |
not sure if static is needed here
Member
Author
There was a problem hiding this comment.
I think the use of static is appropriate. It suggests that they are .a static headers, and not .so dynamically linked headers
Member
Author
There was a problem hiding this comment.
Suggested change
| Some components are header-only static libraries (e.g., rocPRIM, hipCUB) and do not produce a simple `.so` file for `llvm-cov show -object <lib>.so`. | |
| Some components are header-only static libraries (e.g., rocPRIM, hipCUB) that produce `.a` files, and do not produce a simple `.so` file for `llvm-cov show -object <lib>.so`. |
Would that be more clear?
Member
There was a problem hiding this comment.
I don't think there is even .a files? Maybe just ask @stanleytsang-amd
saadrahim
approved these changes
Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposes per-project coverage builds with isolation to prevent profraw contamination from upstream dependencies, maintaining separate coverage pipeline from pre-checkin testing.
#6572