🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/158771

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• ghstack-mergeability-check and Check labels failing with 'Resource not accessible by integration'

✅ No Failures

As of commit 07b5dbe with merge base 7a08755 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorchbot label "topic: not user facing"

Isn't this header file inclusion necessary for the source file to compile successful? If not, then just remove the #include statement entirely. The rocm_smi.h header comes from the rocm-smi-lib apt package.

Isn't this header file inclusion necessary for the source file to compile successful? If not, then just remove the #include statement entirely. The rocm_smi.h header comes from the rocm-smi-lib apt package.

Hi @jeffdaily I saw the conversation between you and @trixirt. Based on that discussion intra_node__comm.cpp doesn't compile with USE_ROCM=ON unless both rocm_smi.h and librocm_smi64 are available. Since rocm-smi-lib isn't always isnstalled by default I kept the include but wrapped it in a check so it's only added when the header is present.

Also in my latest commit I updated the block that uses rsmi_init() and RSMI_STATUS_SUCCESS so that it's guarded with the same HAS_ROCM_SMI macro. The original version was causing a test failure when the header wasn't included. This way the code's only compiled if the header is available.

Shouldn't we instead change the cmake procedure to hard fail if rocm-smi isn't installed? The compile might succeed with this PR when rocm-smi header is missing, but you're also unsafely turning a build issue into a runtime issue if someone tries to use the feature that needed rsmi_init() to work properly.

Shouldn't we instead change the cmake procedure to hard fail if rocm-smi isn't installed? The compile might succeed with this PR when rocm-smi header is missing, but you're also unsafely turning a build issue into a runtime issue if someone tries to use the feature that needed rsmi_init() to work properly.

Hi @jeffdaily , thanks for the earlier feedback. I went ahead and updated the PR to follow your recommendation.

Instead of conditionally compiling the #include <rocm_smi/rocm_smi.h> based on a CMake check, I've now moved that check up into the top-level CMakeLists.txt and changed it to fail hard during configuration if the header isn't present. This avoids the issue you pointed out.

I also switched from checking the USE_ROCM CMake variable to checking ENV{USE_ROCM} since the CMake variable wasn't defined early enough in the top-level CMakeLists.txt. Using the environment variables ensures the check runs reliably when I build with USE_ROCM=1.

Please let me know if this looks good or if you'd prefer the check be somewhere else. Thank you for your recommendations.