[hipDNN] Logging clean-up and feedback follow-up from #4312#4423
Conversation
… installed without spdlog
CMiservaAMD
left a comment
CMiservaAMD
left a comment
There was a problem hiding this comment.
LGTM.
Just holding-off on setting the PR to approved until I have a chance to see how difficult it might be to add changes in https://github.com/ROCm/rocm-libraries/compare/users/cmiserva/almiopen-1048/single-backend-spdlog?expand=1 to this PR.
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (76.83%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #4423 +/- ##
===========================================
- Coverage 65.93% 65.35% -0.58%
===========================================
Files 1600 1581 -19
Lines 254848 242154 -12694
Branches 35899 33925 -1974
===========================================
- Hits 168021 158250 -9771
+ Misses 71603 69886 -1717
+ Partials 15224 14018 -1206
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
The code files flagged are either covered by plugins, or are error handling from HIP errors that aren't covered by tests. I think coverage looks good. |
PR Review SummaryThis is a well-executed refactoring PR that enhances the logging infrastructure across hipDNN components. All changes are consistent, properly tested, and improve code quality. Changes Summary1. Namespace Cleanup
2. Environment Variable Handling Improvements
3. Code Consolidation
4. Performance Optimization
5. Test Coverage
Recommendation✅ Approve - The implementation is clean, consistent, and well-tested. No issues requiring action were identified. Generated by Claude Code |
These empty calls served no purpose and were leftover from previous refactoring. Removing them cleans up the CMake files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…OCm/rocm-libraries into users/bharriso/logging-changes-cleanup
|
Auto-merge enabled. Please disable if blocking comments are added! |
…ng (#4440) ## Motivation Remove spdlog/fmt dependency from hipblaslt-provider. This change is part of removing spdlog/fmt from plugin_sdk. ## Technical Details Changes: - Convert logging to stream based style - Remove hipdnn_enable_spdlog() calls from CMakeLists.txt files - Remove spdlog includes and shutdown calls from test main.cpp files ## Test Plan Build and tests are working locally (CI not enabled for hipblaslt-provider yet). Logs are properly output (unchanged). ## Test Result Build and tests are continuing to pass. Logs are printing properly. --- ### Note Off the logging clean-up branch (PR #4423) that is about to be merged. This ensures we have the latest namespaces, and other clean-up changes for this work. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
) ## Motivation Remove spdlog/fmt dependency from miopen-provider. This change is part of removing spdlog/fmt from plugin_sdk. ## Technical Details Changes: - Convert logging to stream based style - Remove hipdnn_enable_spdlog() calls from CMakeLists.txt files - Remove spdlog includes and shutdown calls from test main.cpp files ## Test Plan Build and tests are working locally and in CI. Sanity check logging to ensure it's functioning same as before for samples, and tests. ## Test Result Build and tests are continuing to pass. Logs are printing properly. Waiting on CI signal. --- ### Note Off the logging clean-up branch (PR #4423) that is about to be merged. This ensures we have the latest namespaces, and other clean-up changes for this work. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ging-changes-cleanup
|
Issuing gardener override, the windows CI issue is known and waiting for another full CI run could take another day leading to more postponed work. |
## Motivation Address non-blocking feedback, and clean-up from #4312. ## Technical Details Summary of changes: - Fix improper logging namespaces in several places - Improve env handling for logging to be more forgiving (remove whitespace, and ignore capitalization) - Remove duplicated files / functions for logging across components - Swap to enum instead of string compares everywhere - Improve locking for shared state ## Test Plan Build & tests are working for hipDNN + provider components ## Test Result Build & tests are passing locally, and waiting on CI --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: CMiservaAMD <cmiserva@amd.com>
## Motivation Address non-blocking feedback, and clean-up from #4312. ## Technical Details Summary of changes: - Fix improper logging namespaces in several places - Improve env handling for logging to be more forgiving (remove whitespace, and ignore capitalization) - Remove duplicated files / functions for logging across components - Swap to enum instead of string compares everywhere - Improve locking for shared state ## Test Plan Build & tests are working for hipDNN + provider components ## Test Result Build & tests are passing locally, and waiting on CI --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: CMiservaAMD <cmiserva@amd.com>
7 participants
Motivation
Address non-blocking feedback, and clean-up from #4312.
Technical Details
Summary of changes:
Test Plan
Build & tests are working for hipDNN + provider components
Test Result
Build & tests are passing locally, and waiting on CI