Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rocm: add reason for disabled component #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dbarry9
Copy link
Contributor

@dbarry9 dbarry9 commented Aug 21, 2024

Pull Request Description

Previously, in the absence of a ROCm device, the rocm component did not set the string containing the reason that the component was disabled. This corresponds to Issue #220.

These changes have been tested with ROCm 6.3.1 on Frontier.

Author Checklist

  • Description
    Why this PR exists. Reference all relevant information, including background, issues, test failures, etc
  • Commits
    Commits are self contained and only do one thing
    Commits have a header of the form: module: short description
    Commits have a body (whenever relevant) containing a detailed description of the addressed problem and its solution
  • Tests
    The PR needs to pass all the tests

@dbarry9 dbarry9 added the status-work-in-progress PR is still being worked on label Aug 21, 2024
Copy link
Contributor

@Treece-Burgess Treece-Burgess left a comment

Choose a reason for hiding this comment

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

I personally would add a format specifier to snprintf, but I don't believe it is actually necessary in this case or beneficial since we are providing the error message.

The rest of the file seems to follow suit with no format specifier when we directly provide the error message as well.

@Treece-Burgess Treece-Burgess linked an issue Sep 28, 2024 that may be closed by this pull request
@Treece-Burgess Treece-Burgess added the component-rocm PRs and Issues related to the rocm component label Dec 19, 2024
@dbarry9 dbarry9 force-pushed the 2024.08.21_rocm-disabled-msg branch 2 times, most recently from 6dd235b to 6dc29ca Compare February 25, 2025 22:04
@dbarry9 dbarry9 removed the status-work-in-progress PR is still being worked on label Feb 25, 2025
@@ -225,7 +225,6 @@ rocm_init_private(void)
fn_exit:
_rocm_vector.cmp_info.initialized = 1;
_rocm_vector.cmp_info.disabled = papi_errno;
strcpy(_rocm_vector.cmp_info.disabled_reason, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was overwriting the error message with an empty string; I am not sure why it was here.

Previously, in the absence of a ROCm device, the rocm component did not
set the string containing the reason that the component was disabled.

These changes have been tested with ROCm 6.3.1 on Frontier.
@dbarry9 dbarry9 force-pushed the 2024.08.21_rocm-disabled-msg branch from 6dc29ca to 6e1a74f Compare February 26, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-rocm PRs and Issues related to the rocm component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rocm: component disabled reason missing
2 participants