Skip to content

feat(ci): add code coverage component test workflow and installer - #7828

Merged
madkasul merged 9 commits into
mainfrom
users/madkasul/code-coverage-nightly
Sep 17, 2026
Merged

madkasul merged 9 commits into
mainfrom
users/madkasul/code-coverage-nightly

Conversation

@madkasul

@madkasul madkasul commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Nightly code-coverage runs need to test a normal (non-instrumented)
TheRock build in which only specific components (e.g. rocBLAS, rocSOLVER)
are swapped for their instrumented counterparts, then collect llvm
profraw files from the test run. This lets coverage be measured for a
targeted component without paying the cost of a fully instrumented build. GitHub Issue: #6572

Technical Details

  • Add .github/workflows/test_code_coverage_component.yml, a reusable
    (workflow_call) job mirroring the standard component test workflow with
    coverage-specific additions:
    • Installs the coverage build via install_rocm_code_coverage_build.py
      instead of install_rocm_from_artifacts.py.
    • coverage_enabled input sets LLVM_PROFILE_FILE (with %p/%m so
      forked/multi-lib shards don't overwrite profraw files) and uploads the
      profraw as a workflow artifact, keyed by coverage_artifact_prefix.
    • Uploads coverage even on test failure (partial shard data is still useful).
  • Add build_tools/install_rocm_code_coverage_build.py:
    • Installs the generic build via install_rocm_from_artifacts.main, then
      downloads instrumented replacement artifacts from the code-coverage run.
    • --replace-rocblas / --replace-rocsolver map to the blas / solver
      artifacts; only library paths matching the keyword are swapped, leaving
      co-packaged libs (e.g. hipBLAS) untouched.
    • Flattens replacement archive members (file/dir/symlink/hardlink) into the
      install tree, preserving exec bits and link structure.

Test Plan

Test Result

Submission Checklist

 ## Motivation
Nightly code-coverage runs need to test a normal (non-instrumented)
TheRock build in which only specific components (e.g. rocBLAS, rocSOLVER)
are swapped for their instrumented counterparts, then collect llvm
profraw files from the test run. This lets coverage be measured for a
targeted component without paying the cost of a fully instrumented build.

 ## Technical Details
- Add `.github/workflows/test_code_coverage_component.yml`, a reusable
  (`workflow_call`) job mirroring the standard component test workflow with
  coverage-specific additions:
  - Installs the coverage build via `install_rocm_code_coverage_build.py`
    instead of `install_rocm_from_artifacts.py`.
  - `coverage_enabled` input sets `LLVM_PROFILE_FILE` (with `%p`/`%m` so
    forked/multi-lib shards don't overwrite profraw files) and uploads the
    profraw as a workflow artifact, keyed by `coverage_artifact_prefix`.
  - Uploads coverage even on test failure (partial shard data is still useful).
- Add `build_tools/install_rocm_code_coverage_build.py`:
  - Installs the generic build via `install_rocm_from_artifacts.main`, then
    downloads instrumented replacement artifacts from the code-coverage run.
  - `--replace-rocblas` / `--replace-rocsolver` map to the `blas` / `solver`
    artifacts; only library paths matching the keyword are swapped, leaving
    co-packaged libs (e.g. hipBLAS) untouched.
  - Flattens replacement archive members (file/dir/symlink/hardlink) into the
    install tree, preserving exec bits and link structure.
@therock-pr-bot

therock-pr-bot Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot therock-pr-bot Bot added the Not ready to Review PR has unresolved policy failures — reviews blocked label Sep 2, 2026
@therock-pr-bot

therock-pr-bot Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@madkasul
madkasul marked this pull request as draft September 2, 2026 06:15
Comment thread rocm-systems Resolved

@reboss reboss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ARTIFACT_LIBRARY_KEYWORD issue is a fairly major flaw, that may require a redesign of download_replacement_artifacts and other areas too perhaps. So I won't continue with my review until that is addressed

Comment thread rocm-systems Resolved

@reboss reboss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed a bit more this time, but the map reversing issue is still present. I don't think it's necessary to keep, can you refactor it accordingly?

@reboss reboss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I suspect this will be the final review. The python LGTM, despite leaving a few comments. You can choose whether or not you want to address those.

The main concerns here are in the yaml. Once you resolve those, I will approve.

Comment on lines +12 to +14
code_coverage_run_id:
type: string
default: ""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed, and anywhere it is used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes True in case of workflow_call. But this keeps an sample for adding workflow_dispatch and enables us to launch this workflow independently providing these options.
I am thinking, this should be ready for manual independent run of test job only providing pre-build code coverage build run-id.

repository: ${{ inputs.repository || github.repository }}
ref: ${{ inputs.ref }}

#- name: Install Non-Instrumental TheRock Build with Replaced Instrumental Component

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if: ${{ failure() && steps.test.outcome == 'failure' }}
run: |
python ./build_tools/github_actions/reproduce_test_failure.py \
--run-id "${{ env.CODE_COVERAGE_RUN_ID }}" \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
--run-id "${{ env.CODE_COVERAGE_RUN_ID }}" \
--run-id "${{ github.run_id }}" \

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have little confusion here. Lets drop this printing of repro steps for code coverage case. Because we have change the way of rocm installation. so, these repro steps will not be valid in our case.

build_tools/github_actions/reproduce_test_failure.py +219

    fetch_cmd = (
        f"$env:GITHUB_REPOSITORY='{args.repository}'; "
        f"python build_tools/install_rocm_from_artifacts.py "
        f"--run-id {args.run_id} "
        f"--amdgpu-family {args.amdgpu_family}"
    )

QUARTZ_REPORTING_WORKFLOW: test_component.yml
QUARTZ_WORKFLOW_POSTFIX: "- test ROCm component"
VENV_DIR: ${{ github.workspace }}/.venv
CODE_COVERAGE_RUN_ID: "${{ inputs.code_coverage_run_id != '' && inputs.code_coverage_run_id || github.run_id }}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes True in case of workflow_call. But this keeps an sample for adding workflow_dispatch and enables us to launch this workflow independently providing these options.

same as above.


if not artifacts:
log("No --replace-* components specified; nothing to download.")
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return non-zero exit code here? If not, when no --replace-* components are specified, this returns 0 for fully non-instrumented build, the coverage job will still return success

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I missed it thanks.

@madkasul
madkasul marked this pull request as ready for review September 10, 2026 15:47
run:
shell: bash
env:
QUARTZ_REPORTING_WORKFLOW: test_component.yml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this workflow run part of the normal release nightly?
if yes we need quartz, but we need to register the workflow correctly under its own name.

if no, please dont add quartz to it. we currently only instrument workflows that are reachable via multi_arch_release.yml.

please let me know which case it is so i can advice further

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. We don't need quartz support as of now. So removed all quartz related code.

jainprad added a commit that referenced this pull request Sep 16, 2026
…rkflow

The merge of #7783 and #7828 left two implementations of the same job.
multi_arch_ci_coverage_linux.yml called test_component.yml, whose coverage
support installs a baseline run and overlays the project under test with
overlay_coverage_artifacts.py, while test_code_coverage_component.yml and
install_rocm_code_coverage_build.py did the same thing by swapping the
component's files into a generic install -- and nothing called them.

Point the test job at test_code_coverage_component.yml, so the installer
path is the one that runs. baseline_run_id, baseline_release_type and
baseline_repository map onto generic_run_id, generic_release_type and
generic_repository; the component to swap is passed explicitly as the
registry key rather than left to the test matrix's job_name, because that
is what COMPONENT_MAP is keyed on.

The installer resolved both runs from one RELEASE_TYPE and one
--run-github-repo, which cannot describe the nightly: coverage publishes
to ci while the baseline is a nightly, and a release nightly comes from
ROCm/rockrel rather than from here. Whichever of the two lookups did not
match would have read the wrong bucket. It now takes the coverage run's
channel and repository separately, and create_backend_from_env grows a
release_type override to make that expressible.

Also carries over two things the copy was missing: the coverage test
timeout floor, without which an instrumented shard hits the 15 minute
wall long before finishing, and timeout-minutes on its two checkout
steps, which workflow_step_timeouts_test.py requires.

A test now asserts every selectable coverage project has a COMPONENT_MAP
entry naming an artifact the registry agrees on. The two lists have to
stay in step for --replace-<project> to resolve, and nothing else keeps
them that way.

Co-authored-by: Cursor <cursoragent@cursor.com>

@amd-shiraz amd-shiraz left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks in better shape now to merge. @madkasul pls fix PR Bot comment before merging. issue number is missing from description. also update submission checklist going through the steps. thx!

@madkasul madkasul left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have incorporated all the review comments.

@therock-pr-bot therock-pr-bot Bot removed the Not ready to Review PR has unresolved policy failures — reviews blocked label Sep 17, 2026
Comment on lines +127 to +134
- name: Checking repository
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
timeout-minutes: 15
with:
repository: ${{ inputs.repository || github.repository }}
ref: ${{ inputs.ref }}

- name: Install TheRock Code Coverage Build
@madkasul
madkasul merged commit eb5650e into main Sep 17, 2026
34 checks passed
@madkasul
madkasul deleted the users/madkasul/code-coverage-nightly branch September 17, 2026 19:14
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Sep 17, 2026
ScottTodd added a commit that referenced this pull request Oct 5, 2026
## Motivation

* Progress on #8272
* Follow-up to #7828
* Follow-up to #8275
* Follow-up to #7002

See https://docs.zizmor.sh/audits/#template-injection and
https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/github_actions_style_guide.md#security---avoid-template-injection.

## Test Plan

* `.github/workflows/test_code_coverage_component.yml` untested.
* `.github/workflows/test_rocm_wheels.yml` will run on CI.

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants