Skip to content

[rocprofiler-sdk] fix rejection of maximum valid stochastic sampling interval - #8431

Merged
samcrater-amd merged 4 commits into
developfrom
users/scrater/aiprofsdk913
Jul 24, 2026
Merged

[rocprofiler-sdk] fix rejection of maximum valid stochastic sampling interval#8431
samcrater-amd merged 4 commits into
developfrom
users/scrater/aiprofsdk913

Conversation

@samcrater-amd

@samcrater-amd samcrater-amd commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

Some PCS interval values were not being accepted/rejected properly.

Technical Details

Ensured we were only reporting valid interval ranges, and added log telling user how to find valid ranges on a config failure.

JIRA ID

JIRA ID : AIPROFSDK-913.

Test Plan

Ran profiler with intervals that were just below, at, and above the maximum supported interval value.

Test Result

Profiler accepted maximum interval and rejected others.

Submission Checklist

@samcrater-amd
samcrater-amd requested review from a team as code owners July 10, 2026 20:02
@therock-pr-bot

therock-pr-bot Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
🌿 Branch Name ✅ Pass —
📝 PR Title/Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ❌ Fail Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-tool/tool.cpp, projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/ioctl/ioctl_adapter.cpp; no test file found
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ Unit Test

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

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

@samcrater-amd samcrater-amd changed the title fix int overflow error, improve failure diagnostic message [rocprofiler-sdk] fix rejection of maximum valid stochastic sampling interval Jul 10, 2026
@therock-pr-bot

therock-pr-bot Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ Unit Test

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@github-actions

github-actions Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Code Coverage Report

Code Coverage Report

Tests Only

code coverage tests.png

Samples Only

code coverage samples.png

Tests + Samples

code coverage all.png

@vlaindic

vlaindic commented Jul 13, 2026

Copy link
Copy Markdown
Member

Thanks for the PR: @samcrater-amd . Please check the comment I added into the Jira.

@samcrater-amd
samcrater-amd force-pushed the users/scrater/aiprofsdk913 branch from 1782c70 to 84a0d66 Compare July 13, 2026 15:33
@samcrater-amd
samcrater-amd force-pushed the users/scrater/aiprofsdk913 branch 2 times, most recently from f30d46e to 0eb5f79 Compare July 20, 2026 17:36
vlaindic
vlaindic requested changes Jul 20, 2026
View reviewed changes

@vlaindic vlaindic 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'm seeing this in the testing plan: Ran profiler with intervals that were just below, at, and above the maximum supported interval value. but I'm not seeing any automation tests. Should we add them? We could do it as a separate PR (tracked in another JIRA/issue)

Comment thread projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-tool/tool.cpp Resolved
@samcrater-amd
samcrater-amd force-pushed the users/scrater/aiprofsdk913 branch from 0eb5f79 to 627cbc7 Compare July 21, 2026 15:49
vlaindic
vlaindic approved these changes Jul 23, 2026
View reviewed changes

@vlaindic vlaindic 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.

LGTM, thanks!

@samcrater-amd
samcrater-amd merged commit d7b1753 into develop Jul 24, 2026
46 of 49 checks passed
@samcrater-amd
samcrater-amd deleted the users/scrater/aiprofsdk913 branch July 24, 2026 12:12
@samcrater-amd samcrater-amd mentioned this pull request Aug 17, 2026
Merged
1 task done
amd-justchen added a commit that referenced this pull request Sep 3, 2026
@amd-justchen
fix(rocprofiler-sdk) fix pc sampling microsecond interval being clamp…
61331e3 
…ed (#10247)

## Motivation

[PR8431](#8431) had the
unintended effect of also limiting the maximum number of microseconds
users could specify when using PC sampling.

## Technical Details

Gated clamp behind a check on interval type.

## JIRA ID
JIRA ID : AIPROFSDK-913

## Test Plan

ran `rocprofv3-avail info --pc-sampling` to check valid intervals

## Test Result

time-based interval was unbounded after fix

## Submission Checklist

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

Reviewers

@ApoKalipse-V ApoKalipse-V ApoKalipse-V left review comments
@vlaindic vlaindic vlaindic approved these changes

Assignees

No one assigned

Labels

Not ready to Review organization: ROCm project: rocprofiler-sdk

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samcrater-amd @vlaindic @ApoKalipse-V