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 |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved mixed-input validation and insufficient direct coverage for the run-level rejection remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates rocprofv3 to reject --kernel-replay-beta-enabled unless kernel replay is selected, while preserving supported input-file behavior.
Changes:
- Adds symmetric validation and diagnostics.
- Adds CLI regression tests.
- Updates documentation and changelog entries.
File summaries
| File | Summary |
|---|---|
projects/rocprofiler-sdk/tests/rocprofv3/kernel-replay/test_kernel_replay_cli.py |
Adds kernel-replay CLI tests. |
projects/rocprofiler-sdk/source/docs/quick-reference/rocprofv3-cli-options.rst |
Updates flag documentation. |
projects/rocprofiler-sdk/source/docs/how-to/using-rocprofv3.rst |
Clarifies acknowledgement behavior. |
projects/rocprofiler-sdk/source/docs/how-to/using-kernel-replay-rocprofv3.rst |
Documents kernel-replay requirements. |
projects/rocprofiler-sdk/source/docs/conceptual/kernel_replay/kernel_replay_callback_api.md |
Updates kernel-replay guidance. |
projects/rocprofiler-sdk/source/bin/rocprofv3.py |
Adds validation and updated help text. |
projects/rocprofiler-sdk/CHANGELOG.md |
Records the behavior change. |
Review details
Suppressed comments (2)
projects/rocprofiler-sdk/source/docs/how-to/using-kernel-replay-rocprofv3.rst:74
- This wording is too absolute for the supported input-file path covered by the new test: with the acknowledgement on the command line and
"replay_mode": "kernel"in a job, no--replay-mode kernelappears on the command line but the invocation is accepted. Qualify the rejection condition so the documentation matches theinp_argsexception inmain().
``--kernel-replay-beta-enabled`` only acknowledges the beta; it does not select kernel replay.
Passing it without ``--replay-mode kernel`` is rejected rather than ignored, so a command line that
asks only for the acknowledgement cannot quietly fall back to application replay.
projects/rocprofiler-sdk/source/docs/how-to/using-rocprofv3.rst:1181
- The new note omits the input-file exception implemented and tested here: a command-line acknowledgement is valid when an input-file job supplies
"replay_mode": "kernel", even though the command line lacks--replay-mode kernel. Please qualify this sentence so users of input files are not told that their supported configuration is rejected.
- ``--replay-mode kernel`` requires ``--pmc`` and ``--kernel-replay-beta-enabled``. The reverse also holds: ``--kernel-replay-beta-enabled`` acknowledges the beta but does not select kernel replay, so passing it without ``--replay-mode kernel`` is rejected rather than ignored.
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try: | ||
| launched_runs("--pmc", "SQ_WAVES", "--kernel-replay-beta-enabled") | ||
| except SystemExit as exc: | ||
| assert exc.code != 0 |
There was a problem hiding this comment.
Addressed in eb63cdc1: added a test that calls the real run() path and asserts the complete stderr diagnostic.
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
| ``--kernel-replay-beta-enabled`` only acknowledges the beta; it does not select kernel replay. | ||
| Passing it without ``--replay-mode kernel`` is rejected rather than ignored, so a command line that | ||
| asks only for the acknowledgement cannot quietly fall back to application replay. |
There was a problem hiding this comment.
Addressed in eb63cdc1: the guide now documents that an input-file job selecting replay_mode: kernel is a valid alternative selector.
| .. note:: | ||
|
|
||
| - ``--replay-mode kernel`` requires ``--pmc`` and ``--kernel-replay-beta-enabled``. | ||
| - ``--replay-mode kernel`` requires ``--pmc`` and ``--kernel-replay-beta-enabled``. The reverse also holds: ``--kernel-replay-beta-enabled`` acknowledges the beta but does not select kernel replay, so passing it without ``--replay-mode kernel`` is rejected rather than ignored. |
There was a problem hiding this comment.
Addressed in eb63cdc1: the note now qualifies rejection to the case where no input-file job selects kernel replay.
Addressed in |
… integration Co-authored-by: Vivek Kale <vivek.kale@amd.com>
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
…n its own rocprofv3 already fails when --replay-mode kernel arrives without --kernel-replay-beta-enabled, but not the other way round. The flag's name reads as if it enabled replay, so a command line carrying only the acknowledgement ran application replay while the user believed they were running kernel replay, and the counter values application replay produces are indistinguishable from replayed ones. Fail with a diagnostic that names --replay-mode kernel instead. An input file job may be the thing asking for replay, and the acknowledgement can only be given on the command line, so the check only fires when nothing anywhere selected kernel replay. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>
Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>
ab93c89 to
8813cae
Compare
…to the kernel replay integration Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…cursor-fixes-help


Motivation
rocprofv3already fails when--replay-mode kernelarrives without--kernel-replay-beta-enabled. The reverse was accepted silently.That asymmetry matters because of the flag's name.
--kernel-replay-beta-enabledreads as thoughit enables kernel replay, and that is the natural thing to assume from it; in fact it only
acknowledges that the feature is beta, and
--replay-mode kernelis what selects it. A commandline carrying only the acknowledgement ran application replay — the whole application re-run
once per counter group — while the user believed they were running kernel replay.
Nothing makes that mistake self-correcting. The run succeeds, no diagnostic is printed, no output
field records which replay strategy was used, and the counter values application replay produces
are indistinguishable from replayed ones. The user gets plausible numbers collected by a strategy
they did not ask for, and any conclusion they draw about replay overhead, or about the
determinism the two strategies differ on, is drawn from the wrong run.
This is easy to hit in practice: the acknowledgement is the memorable half of the pair, and it is
the half that mentions kernel replay by name.
Technical Details
Both existing validation sites gain the symmetric check.
run()— the check is theelseof the existingif kernel_replay_mode:block.argsthereis the fully merged per-run view, so it sees the acknowledgement and the mode together regardless
of which source supplied either one. It fires well before
os.execvpe, so nothing is launched.main()— the check is guarded on nothing anywhere having selected kernel replay:The guard is load-bearing. The acknowledgement can only be given on the command line, but an input
file job may be the thing asking for replay, so
--kernel-replay-beta-enabledalongside a jobwith
"replay_mode": "kernel"has to stay valid. Without theinp_argsterm this check wouldreject a legitimate and already-supported combination.
The diagnostic names the flag the user actually needs, and both ways out rather than only one:
The flag's
--helptext now states that it does not select kernel replay on its own and thatpassing it alone is rejected rather than ignored, which is the root cause rather than the symptom.
The four documentation pages that describe the flag are updated to match:
quick-reference/rocprofv3-cli-options.rst,how-to/using-kernel-replay-rocprofv3.rst,how-to/using-rocprofv3.rst, andconceptual/kernel_replay/kernel_replay_callback_api.md— eachof which previously stated the requirement in one direction only.
This is a behaviour change for a command line that used to be accepted, so it is recorded under
Resolved issues in the changelog. It cannot break a working kernel replay invocation, since any
such invocation already passes
--replay-mode kernel; it can only fail a command line that wasnot doing what its author intended.
Issue Tracking
No GitHub issue or JIRA ID — found while validating the kernel replay CLI surface from
#7960. Happy to file one and link it if that is
preferred for release tracking.
Test Plan
Three tests added to
tests/rocprofv3/kernel-replay/test_kernel_replay_cli.py, which importsrocprofv3.pyand replacesrunso no application is executed and no environment is touched. Itneeds neither a GPU nor a built SDK.
test_beta_acknowledgement_without_replay_mode_is_rejected— the case this PR is about:--pmc SQ_WAVES --kernel-replay-beta-enabledmust exit nonzero.test_beta_acknowledgement_is_accepted_for_an_input_file_job_that_asks_for_replay— thecombination the
main()guard exists to protect: the acknowledgement on the command line with"replay_mode": "kernel"in an input file job must still run.test_application_replay_is_unaffected_without_the_acknowledgement— a plain--pmcrun,confirming the rejection is about the unaccompanied flag rather than about
--pmc.The second and third exist because a validation check is only as good as the things it declines to
reject; they pin the two neighbouring command lines that must keep working.
Test Result
All 30 tests in the file pass, comprising the 3 new ones and the 27 that were already there —
including
test_replay_without_beta_acknowledgement_is_rejected, which covers the originaldirection and confirms the two checks do not interfere:
The new rejection emits the intended diagnostic in the captured output rather than a bare exit
code, so the failure mode a user sees is what is being tested.
Submission Checklist