Repository navigation
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
reboss
left a comment
There was a problem hiding this comment.
Seems like .github/scripts/export_coverage_metadata.py is going to need to change quite a bit to follow the test filter standard, so I left it out of my review. You should be able to make use of the test runners in each of the project folders, whether through extension or adding conditionals in the individual test runners themselves, as we enable coverage for each of the projects
| # onto BUILD_CODE_COVERAGE so the header-only library and its tests are instrumented. | ||
| option(HIPCUB_ENABLE_COVERAGE "Enable code coverage (alias for BUILD_CODE_COVERAGE)" OFF) | ||
| if(HIPCUB_ENABLE_COVERAGE) | ||
| set(BUILD_CODE_COVERAGE ON) |
There was a problem hiding this comment.
I'd be careful doing this, since other projects having BUILD_CODE_COVERAGE may cause them to instrument their code inadvertently
There was a problem hiding this comment.
Addressed — hipCUB coverage is now gated on HIPCUB_ENABLE_COVERAGE directly at the instrumentation site (no shared BUILD_CODE_COVERAGE toggled), so enabling it can't inadvertently instrument other projects. (Header-only coverage is also currently disabled in the flow.)
There was a problem hiding this comment.
Looks like these are failing now. You may have missed some places
There was a problem hiding this comment.
removed hipcub from this PR. will take care in header only PR.
| rocPRIM: | ||
| flags: [rocPRIM] | ||
| target: 80% |
There was a problem hiding this comment.
This is needed, but is out-of-scope for this PR. Can you please open up a smaller PR with just these changes
There was a problem hiding this comment.
Please open another PR with these changes, and link it here, so we don't forget
| - name: Stage coverage objects and llvm tools | ||
| run: | | ||
| set -euo pipefail | ||
| # Use $GITHUB_WORKSPACE (container path) rather than the github.workspace | ||
| # expression (host path) so the staged dir matches what upload-artifact reads. | ||
| STAGE="${GITHUB_WORKSPACE}/coverage-objects" | ||
| DIST="TheRock/build-coverage/dist/rocm" | ||
| LLVM_BIN="${DIST}/lib/llvm/bin" | ||
| LIB_NAME="lib$(echo '${{ inputs.cmake_target }}' | tr '[:upper:]' '[:lower:]')" | ||
| mkdir -p "${STAGE}/lib" "${STAGE}/bin" | ||
| # Instrumented shared libraries for the target component (used as llvm-cov -object). | ||
| find "${DIST}" -type f -name "${LIB_NAME}*.so*" -exec cp -av {} "${STAGE}/lib/" \; | ||
| # Version-matched coverage tooling, so the report job does not depend on the dist. | ||
| cp -av "${LLVM_BIN}/llvm-profdata" "${STAGE}/bin/" | ||
| cp -av "${LLVM_BIN}/llvm-cov" "${STAGE}/bin/" | ||
| # The llvm tools dynamically link libLLVM plus TheRock's bundled sysdeps | ||
| # (e.g. librocm_sysdeps_z). Stage the whole llvm lib dir and all sysdeps | ||
| # next to the tools so the report job can run them via LD_LIBRARY_PATH. | ||
| if [[ -d "${DIST}/lib/llvm/lib" ]]; then | ||
| find "${DIST}/lib/llvm/lib" -maxdepth 1 -name '*.so*' -exec cp -av {} "${STAGE}/bin/" \; | ||
| fi | ||
| # Note: do NOT use -type f here, otherwise the SONAME symlinks (e.g. | ||
| # librocm_sysdeps_z.so.1 -> librocm_sysdeps_z.so.1.3.2) are skipped and | ||
| # the loader cannot resolve the dependency. | ||
| find "${DIST}" -name 'libLLVM.so*' -exec cp -av {} "${STAGE}/bin/" \; | ||
| find "${DIST}" -name 'librocm_sysdeps_*.so*' -exec cp -av {} "${STAGE}/bin/" \; | ||
| # Demangler used by llvm-cov show for readable HTML. | ||
| cp -av "${LLVM_BIN}/llvm-cxxfilt" "${STAGE}/bin/" 2>/dev/null || true | ||
| echo "Staged coverage objects:" | ||
| ls -lR "${STAGE}" | ||
| echo "llvm-profdata dependencies (in build env):" | ||
| ldd "${STAGE}/bin/llvm-profdata" || true |
There was a problem hiding this comment.
What is the need for all of this?
There was a problem hiding this comment.
The report job runs llvm-cov/llvm-profdata that must be raw-profile-format compatible with the compiler that instrumented the libraries, so we stage those exact tools (plus libLLVM.so* and the rocm_sysdeps libs they load) from the coverage build's own dist. That's the strictest version-match guarantee. It can be replaced by fetching the same amd-llvm artifact from this run instead — see my detailed reply on the fetch_artifact_args thread below.
There was a problem hiding this comment.
this is incredibly dense and hard to read. could we write this in a python script that handles this?
also, your reply should be included in the code itself, so an engineer can view it and understand why it is needed
There was a problem hiding this comment.
We don't need a python script at all. We just need to use fetch_artifacts.py to get llvm-* tools, and then the .so file will be on the test node already, so we don't need to upload it here as well
| - name: Upload coverage objects and llvm tools | ||
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 | ||
| with: | ||
| name: coverage-objects-${{ inputs.project_name }}-${{ inputs.amdgpu_families }} | ||
| # Relative path resolves against the container workspace, matching the staging step. | ||
| path: coverage-objects | ||
| if-no-files-found: error | ||
| retention-days: 7 |
There was a problem hiding this comment.
This might require some more discussion/design considerations, if we need to upload the llvm binaries to the test node
There was a problem hiding this comment.
Dug into this. The tools/libLLVM don't go to the test node (the test job only emits profraw) — they're needed in the report job. Rather than uploading them, the report job can fetch the version-matched amd-llvm artifact from this same run. Full mechanism, exact fetch_artifacts.py invocation, and the tradeoff are in my reply on the fetch_artifact_args thread below.
There was a problem hiding this comment.
we currently use AWS S3 to store our artifacts. we should convert to that, so if an engineer wants to download artifacts locally, they can use our scripts to do so rather than introducing new storage which will result in more retrieval scripts
There was a problem hiding this comment.
this will take a bit more discussion too, i agree
| name: Test (${{ inputs.amdgpu_families }}) | ||
| needs: [therock-build-linux] | ||
| # TODO(TheRock#3288): Re-enable gfx950-dcgpu runners once we get more capacity | ||
| if: ${{ inputs.amdgpu_families != 'gfx950-dcgpu' }} |
There was a problem hiding this comment.
Not sure why this is removed. Might have been me that removed it by accident. Please add back
There was a problem hiding this comment.
Done (b8ecfdd) — the if: ${{ inputs.amdgpu_families != 'gfx950-dcgpu' }} guard is restored in therock-ci-linux.yml.
There was a problem hiding this comment.
this is not done. please restore it
There was a problem hiding this comment.
therock-ci-linux.yml is not part of this PR code.
There was a problem hiding this comment.
This needs to be reverted. It is still present from my work. I'm not sure why I disabled it, but it must be reverted before merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8656 +/- ##
============================================
- Coverage 69.35% 63.43% -5.92%
============================================
Files 2777 1105 -1672
Lines 457902 137802 -320100
Branches 67514 16879 -50635
============================================
- Hits 317548 87405 -230143
+ Misses 117221 46045 -71176
+ Partials 23133 4352 -18781
🚀 New features to boost your workflow:
|
| target_link_libraries(${BENCHMARK_TARGET} PRIVATE amd_smi) | ||
| # primbench uses AMD SMI for optional GPU monitoring. Treat it as a soft | ||
| # dependency: enable monitoring only when the amd-smi dev header is | ||
| # available, otherwise build the benchmark without monitoring instead of | ||
| # failing to compile. This keeps narrowed component builds that do not stage | ||
| # the amd-smi dev headers (e.g. code-coverage builds) working. | ||
| find_path(ROCRAND_AMDSMI_INCLUDE_DIR amd_smi/amdsmi.h HINTS ENV ROCM_PATH PATH_SUFFIXES include) | ||
| if(ROCRAND_AMDSMI_INCLUDE_DIR) | ||
| target_include_directories(${BENCHMARK_TARGET} PRIVATE ${ROCRAND_AMDSMI_INCLUDE_DIR}) | ||
| target_link_libraries(${BENCHMARK_TARGET} PRIVATE amd_smi) | ||
| else() | ||
| message(STATUS "AMD SMI headers not found; building ${BENCHMARK_TARGET} without primbench GPU monitoring") | ||
| target_compile_definitions(${BENCHMARK_TARGET} PRIVATE PRIMBENCH_NO_MONITORING) | ||
| endif() |
There was a problem hiding this comment.
This seems out of scope for this PR, and has the potential to affect non-code-coverage builds moving forward. If you can go without it, I'd remove it. Otherwise, you may need to seek review from the rocRAND team. Tagging @stanleytsang-amd for visibility.
There was a problem hiding this comment.
Done in 1ec0936 - reverted projects/rocrand/benchmark/CMakeLists.txt to develop, so the AMD-SMI soft-dependency change is no longer part of this PR. Heads-up: this change is what let the narrowed RAND-only coverage build compile the rocRAND benchmark (which links amd_smi); with it removed, if the coverage build still builds the benchmark it may fail to compile again. If needed I can exclude the benchmark from the coverage build, or this can be re-introduced via a rocRAND-team-reviewed PR (cc @stanleytsang-amd).
There was a problem hiding this comment.
Re-applied in 585f39b. Removing this broke the RAND-group coverage builds: hipRAND, rocRAND and rocFFT all build the RAND group, and shared/primbench/primbench.hpp includes + links amd_smi unconditionally, but the narrowed coverage build stages no amd-smi dev headers -> fatal error: 'amd_smi/amdsmi.h' file not found -> no profraw, no report for all three.
The benchmark can't be disabled from rocm-libraries either: TheRock pins -DBUILD_BENCHMARK=${THEROCK_BUILD_TESTING} for rocRAND (math-libs/CMakeLists.txt), and coverage needs THEROCK_BUILD_TESTING=ON to build the tests.
The soft dependency only changes behavior when the amd-smi header is absent (narrowed/coverage builds); normal builds have the header, so they link amd_smi + enable monitoring exactly as before -> no impact on non-coverage builds. Happy to move it to a rocRAND-team-reviewed PR if you'd prefer, but it's required for the coverage build to compile.
There was a problem hiding this comment.
I saw some issue while removing it. keeping it for end. I will revisit it.
This PR is scoped to the header-only libraries (rocPRIM / hipCUB / rocThrust). Restore the shared-object components' CMakeLists (hipRAND / rocRAND / rocFFT / rocBLAS) to develop verbatim so they are not part of this PR - they are covered in the shared-object PR (#8656). Co-authored-by: Cursor <cursoragent@cursor.com>
…build) The hipRAND coverage build compiles rocRAND (a dependency), including its benchmarks. Reverting rocrand/benchmark/CMakeLists.txt to develop dropped the "AMD SMI soft dependency" fix, so the narrowed coverage build (no amd-smi dev headers) failed with: primbench.hpp: fatal error: 'amd_smi/amdsmi.h' file not found. Restore that one file (it is a build prerequisite for coverage builds, not a rocRAND coverage feature). All other rocBLAS/rocFFT/rocRAND changes remain reverted; the PR stays hipRAND-only for coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
…r set -e) The Test step derived the ctest dir by grepping the component's test_script for --test-dir, but after the develop merge test_script is now 'python .../test_runner.py' (no --test-dir). grep found nothing and exited 1, which under `set -e -o pipefail` killed the coverage Test step before coverage_runner.py ran (build passed, test shard failed instantly). Derive the ctest dir by finding build/bin/<Component> case-insensitively (hipRAND vs hiprand) and guard the command substitutions with `|| true`. Co-authored-by: Cursor <cursoragent@cursor.com>
…build) The coverage build produced a non-instrumented libhiprand.so (0 __llvm_profile symbols) even though -fprofile-instr-generate was in the compile command and the sub-project was configured with coverage. Root cause: ccache (direct mode, 96% hit rate) does not include -fprofile-instr-generate in its hash, so the coverage compile hit a cached NON-coverage hiprand.cpp object. Add a coverage-only compile define (HIPRAND_COVERAGE_INSTRUMENTED=1) so ccache keys the coverage build separately and recompiles hiprand.cpp fresh (instrumented). This busts only hipRAND's cache entry -- LLVM/HIP/deps still hit ccache, avoiding a multi-hour full rebuild (and avoids the shared-cache pollution risk of CCACHE_RECACHE). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Jobbins <mulvanyrobbinsj@gmail.com>
…ck@enable-codecov) Point the coverage build's TheRock checkout back at ROCm/TheRock at the pinned inputs.therock_ref instead of the personal reboss/TheRock@enable-codecov branch. Co-authored-by: Cursor <cursoragent@cursor.com>
…mary Add therock-ci-coverage back to the therock_ci_summary needs list so coverage is part of the CI gate again. Coverage now runs green (build instruments hipRAND; coverage_runner.py skips gracefully when no profraw is produced), so it no longer needs to be non-blocking. Co-authored-by: Cursor <cursoragent@cursor.com>
Coverage is now a gating job, so an empty profraw result must surface instead of passing as a no-op. Replace the graceful "no .profraw -> skip/return" with a logged error + non-zero exit, so a non-instrumented library / no coverage data fails the coverage job (red) rather than going green. Co-authored-by: Cursor <cursoragent@cursor.com>
43610f5 to
abf3462
Compare
…rtifact layer) TheRock ships a non-instrumented rand_lib artifact, so the test job downloads a libhiprand.so with 0 __llvm_profile symbols and produces no profraw. Work around it: - Build job: upload the instrumented TheRock/build-coverage/dist/rocm/lib/libhiprand.so* as artifact coverage-instrumented-lib-<amdgpu_families>. - Test job: download it and overwrite ./build/lib/libhiprand.so.1.1 (the real file the tests load via RPATH) before running the tests, so ctest exercises the instrumented library and emits profraw. Keyed by amdgpu_families so each GPU family gets its own arch-specific instrumented lib. Co-authored-by: Cursor <cursoragent@cursor.com>
…test node The swap-in now produces real coverage, but llvm-cov show emitted "error: .../hiprand.cpp: No such file" because the build bakes the source path at <workspace>/projects/... (rocm-libraries at the build workspace root) while the test node checks rocm-libraries out under <workspace>/rocm-libraries. Add a --path-equivalence option to coverage_runner.py (applied to llvm-cov report/show/export) and pass -path-equivalence=<ws>,<ws>/rocm-libraries from the test workflow so sources resolve and the HTML report annotates them. Co-authored-by: Cursor <cursoragent@cursor.com>
…tifact
After coverage_runner.py generates the report, upload ${GITHUB_WORKSPACE}/coverage-report
(HTML, code_cov_<proj>.report, coverage.info, <proj>.profdata) as artifact
coverage-report-<project>-<amdgpu_family>-shard<N> so it can be downloaded from the run.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Closing because there are 10 of these open. |
ScottTodd
left a comment
There was a problem hiding this comment.
Drive-by comments since this filtered down to some PRs in TheRock (ROCm/TheRock#3761)
There are some pretty large architectural issues with the direction taken here. I also see 170+ comments and 130+ commits already on this PR that has been open for over 1 month.
Please provide a link to the design proposal for this work. At this level of cross-project complexity, this should have started from a plan that gets review/approval before jumping to a pull request with code. If there have been design discussions offline, sharing the meeting notes could help too.
There was a problem hiding this comment.
Why is this a separate workflow? I'd prefer to integrate coverage directly into existing build and test jobs if possible.
Do you have a design/plan written down somewhere that we could review?
| project_name: | ||
| type: string |
There was a problem hiding this comment.
One workflow job running cmake --build TheRock/build-coverage --target therock-artifacts therock-dist per project isn't going to be viable for a few reasons:
- The artifacts built by each job are overlapping, resulting in excessive CPU builder usage
- Those jobs then upload those overlapping artifacts to S3 and there will likely be conflicts (between the "coverage"/"non-coverage" CI runs and between each instance of this where the base artifacts are rebuilt for each project). Those conflicts can result in downstream test and packaging workflows using a nondeterministic set of files based on which jobs complete in what order
- applying a different namespace for each job's artifacts/logs/etc. could help here but we really shouldn't be rebuilding unless actually necessary
See example logs on this PR
There was a problem hiding this comment.
Multi-gpu (and multi-process) libraries need to have code coverage executed on those configurations as well, with the results used to give an overall code coverage report. Is there any plan on how to do this? This had been discussed in math-ci, but therock-ci may be unaware of these efforts.
hipRAND coverage instruments a real shared library (libhiprand.so), so the header-only test-binary machinery and a few other paths were never exercised. Remove them so there is a single source of truth: - coverage_runner.py: fix stale docstring (it IS wired into CI via therock-test-component.yml); resolve shared-library objects only; drop the unused --skip-tests and --profraw-dir arguments. - export_coverage_metadata.py: drop test-binary discovery/staging (_looks_like_executable, find_test_binaries, --cmake-target/--stage-dir) and the legacy combined-file layout (resolve_project_key); libraries-only metadata. - therock_configure_coverage.py / therock-ci.yml / therock-ci-coverage.yml: remove the coverage_config input that was threaded through but never read. No functional change to the hipRAND coverage flow. Co-authored-by: Cursor <cursoragent@cursor.com>
malcolmroberts
left a comment
There was a problem hiding this comment.
If this is code coverage for hiprand, it should not do code coverage for rocfft.
Ports the code coverage changes from PR #8656 (jainprad/codecoverage_lib_v2): - Coverage CI workflows and runner/configuration scripts - CMake coverage wiring for hiprand, rocfft, rocrand Co-authored-by: Cursor <cursoragent@cursor.com>
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
|
This seem stale, so I'm closing. Feel free to re-open. |
code coverage (duplicate of #8427 for experimentation)
This branch is an exact copy of the head of #8427, opened as a separate PR so
that #8427 can be preserved as a backup while this one is used for further
experimentation.
Adds a TheRock-based code coverage CI flow for hipRAND:
.github/scripts/therock_configure_coverage.py— per-project coverage matrix (hipRAND), pinned to RAND-only build/test.github/workflows/therock-ci-coverage.yml— build -> test (via therock-test-packages) -> merge/report.github/workflows/therock-ci.yml,therock-ci-linux.yml,therock-test-packages.yml,therock-test-component.yml— coverage wiring + opt-in coverage flagprojects/hiprand/CMakeLists.txt,projects/hiprand/test/CMakeLists.txt— hipRAND coverage supportMade with Cursor