Move user-facing output from libraries to CLI callers#111
Merged
sgopinath1 merged 2 commits intoROCm:mainfrom Apr 8, 2026
Merged
Move user-facing output from libraries to CLI callers#111sgopinath1 merged 2 commits intoROCm:mainfrom
sgopinath1 merged 2 commits intoROCm:mainfrom
Conversation
Add IsEnabled() (bool, error) to the gpu-tracker Interface as a read-only query that checks tracker state without side effects. CLI callers use it to print context-appropriate messages (e.g. "already enabled", "disabled") before calling the mutating methods. This moves all user-facing fmt.Printf messages out of the library into the CLI layer, where they belong. The library now only returns errors and data — it does not print to stdout.
There was a problem hiding this comment.
Pull request overview
This PR refactors the GPU Tracker and CDI components so that internal libraries return structured data (and errors) while CLI commands handle all user-facing output/formatting.
Changes:
- Added
IsEnabled()to the GPU Tracker interface and updated CLI commands to print context-appropriate status messages. - Refactored GPU Tracker APIs to return structured results (
[]GPUStatusEntry,*AccessibilityResult) instead of printing tables/messages. - Updated CDI
PrintSpecto return a formatted JSON string and moved generation/validation messaging into CLI commands.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/gpu-tracker/gpu-tracker.go | Adds structured types/results, IsEnabled(), and changes status/mode methods to return data instead of printing. |
| internal/gpu-tracker/gpu-tracker_test.go | Updates tests to accommodate new return values from refactored GPU Tracker APIs. |
| internal/cdi/cdi.go | Changes PrintSpec signature and removes some direct printing from CDI operations. |
| cmd/amd-ctk/gpu-tracker/status/status.go | Moves GPU status table formatting/output into the CLI. |
| cmd/amd-ctk/gpu-tracker/reset/reset.go | Prints reset output in CLI, using IsEnabled() to add conditional guidance. |
| cmd/amd-ctk/gpu-tracker/gpu-tracker.go | Moves exclusive/shared operation result output into the CLI based on structured return values. |
| cmd/amd-ctk/gpu-tracker/enable/enable.go | Adds pre-check via IsEnabled() and moves enable messaging into CLI. |
| cmd/amd-ctk/gpu-tracker/disable/disable.go | Adds pre-check via IsEnabled() and moves disable messaging into CLI. |
| cmd/amd-ctk/cdi/validate/validate.go | Moves validation messaging into CLI and prints valid/invalid results. |
| cmd/amd-ctk/cdi/generate/generate.go | Prints “Generated CDI spec …” from CLI after writing spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shiv-tyagi
force-pushed
the
remove-printf-from-library
branch
from
beaaebf to
e7f5727
Compare
Member
Author
|
Addressed review comments from copilot. This one is good to go in. The next PRs will enhance error wrapping and logging fixes. |
Library methods now return structured data instead of printing directly to stdout, letting callers control display formatting. gpu-tracker: - ShowStatus returns []GPUStatusEntry instead of printing a table - MakeGPUsExclusive/MakeGPUsShared return *AccessibilityResult with Changed, NotChanged, InvalidGPUs, InvalidRanges fields - Export Accessibility type as string with SHARED_ACCESS/EXCLUSIVE_ACCESS constants; keep internal int representation for backward-compatible JSON serialization of the tracker file cdi: - WriteSpec no longer prints success message; caller prints it - PrintSpec returns (string, error) instead of printing to stdout - ValidateSpec no longer prints status/result messages; caller handles display based on the (bool, error) return
sgopinath1
reviewed
Apr 8, 2026
internal/cdi/cdi.go
Outdated
| // PrintSpec prints the generated CDI spec on the console | ||
| PrintSpec() error | ||
| // PrintSpec returns the generated CDI spec as a formatted JSON string | ||
| PrintSpec() (string, error) |
Collaborator
There was a problem hiding this comment.
Shouild we rename this function, since it is not printing anything now?
Member
Author
There was a problem hiding this comment.
I have renamed to FormatSpec reflecting that it formats the CDI spec instead of printing.
shiv-tyagi
force-pushed
the
remove-printf-from-library
branch
2 times, most recently
from
9bf84b1 to
e8ac0f7
Compare
sgopinath1
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does the following fixes:-
Test plan
I have tested for all the above cases and the behaviour looks as expected.