ROCMAI-214: extract GPU/device-policy parsing and VRAM usage warnings into gpu_policy.rs by jussielo-amd · Pull Request #750 · ROCm/rocm-cli

jussielo-amd · 2026-10-27T08:53:48Z

Summary

Phase 6f-7 of the main.rs modularization (epic ROCMAI-27). Mechanical relocation, no behavior change: moves GPU/device-policy parsing and VRAM usage warning logic out of apps/rocm/src/main.rs into a new apps/rocm/src/gpu_policy.rs.

parse_device_policy/parse_gpu_selection/parse_gpu_indices_arg, select_gpu_indices_under_launch_lock, validate_pinned_gpu_index, effective_gpu_count, auto_select_gpu_indices/select_auto_gpu_index, gpu_vram_usage and its platform-gated gpu_vram_usage_sysfs variants (moved intact, cfg-gates unchanged — no platform-support change), read_drm_vram_usage/read_sysfs_u64/drm_device_is_amd, parse_gpu_vram_usage, gpu_low_memory_warning/vram_capacity_is_meaningful/serve_gpu_low_memory_warning, busy_gpu_indices, detect_gpu_count, plus struct GpuVramUsage and AUTO_FREE_VRAM_FRACTION, interleaved but part of the same cluster.
This cluster was documented as "staying in main.rs" by two earlier PRs in this same stack (ROCMAI-213: extract engine plugin binary resolution and IPC envelope into engine_ipc.rs #749's engine_ipc.rs, and serve_cmd.rs's pre-existing doc comment) — both corrected in this PR along with their imports, since those items are part of this ticket.
Real external callers beyond main.rs fixed: engine_ipc.rs, serve_cmd.rs.
An unrelated breakage from the move: advised_commands.rs's file-path allowlist for a CPU-fallback error string had to be updated to point at the new file.
dispatch()'s Command::EngineServeHttp arm calls parse_gpu_indices_arg directly — body confirmed byte-identical (independently re-verified by SHA-256), only the import changed.
33 tests partitioned into the new module's own test block, including two shared test fixtures (vram(), host_gpu()) duplicated into main.rs's test module where a test that stays behind also needs them — documented inline. Total workspace test count unchanged.
docs/architecture.md updated with this module's entry.

Test plan

Depends on #749 (ROCMAI-213) — fourth PR in the ROCMAI-86 (Phase 6f) stack.
Part of Jira ROCMAI-214 / epic ROCMAI-27.

🤖 Generated with Claude Code

Copilot

🟡 Changes recommended

Unconditional filesystem and path imports become unused on Windows non-test targets, causing warnings-as-errors checks to fail.

1 open issue

Gate fs and Path imports for Windows builds · New

What changed in this PR

Continues the main.rs modularization by moving GPU policy parsing, selection, and VRAM warnings into a dedicated module, with no intended behavior change.

Changes:

Relocates GPU helpers and 33 tests into gpu_policy.rs.
Updates caller imports and the command-message allowlist.
Documents the new module and corrects comments about helper ownership.

Verification used the supplied diffs; the repository snapshot was unavailable, so builds and tests were not run.

File	Description
`docs/architecture.md`	Documents GPU module responsibilities and callers.
`apps/rocm/src/serve_cmd.rs`	Updates GPU imports and ownership comments.
`apps/rocm/src/main.rs`	Removes relocated helpers and tests; imports the new module.
`apps/rocm/src/gpu_policy.rs`	Houses GPU parsing, selection, telemetry, warnings, and tests.
`apps/rocm/src/engine_ipc.rs`	Imports device-policy parsing from its new location.
`apps/rocm/src/advised_commands.rs`	Updates the error-message allowlist path.

_{Review effort: Balanced}

… into gpu_policy.rs Signed-off-by: Jussi Elo <jussi.elo@amd.com>

juhovainio

Thanks for the clean extraction. I diffed the moved code against what was removed from main.rs and the only differences are the pub(crate) visibility changes and the test-module imports. The 34 tests moved over unchanged, and the docs/architecture.md module map and the advised_commands.rs path update both look right.

One thing needs fixing before merge: the windows-build-and-test job fails at "Clippy (Windows targets)" under -D warnings, because the move left some imports unused on Windows.

std::fs and std::path::Path in gpu_policy.rs are only used by the Linux-gated sysfs fallback (see inline comments). Gate both with #[cfg(target_os = "linux")].
Stdio at apps/rocm/src/main.rs:147 is now unused on Windows too, since the code that used it moved out. That line is outside the diff so I can't comment on it inline, but it needs the same treatment (or removal if nothing on any platform still uses it).

Optional: the header comment in gpu_policy.rs is quite long and could be trimmed.

juhovainio · 2026-10-28T13:40:23Z

+//! and `#[cfg(not(target_os = "linux"))]` — both moved intact, cfg-gates
+//! unchanged, no platform behavior change.
+
+use std::fs;


std::fs is only used by the Linux-gated sysfs fallback, so this is an unused import on Windows (CI "Clippy (Windows targets)" fails under -D warnings). Gate it with #[cfg(target_os = "linux")].

Fixed in 7f350da — gated with #[cfg(any(target_os = "linux", test))] rather than plain #[cfg(target_os = "linux")], since this module's own test module also needs fs/Path on every platform (matches the cfg already on read_drm_vram_usage/read_sysfs_u64/drm_device_is_amd). Windows clippy is green on the updated commit.

juhovainio · 2026-10-28T13:40:23Z

+//! unchanged, no platform behavior change.
+
+use std::fs;
+use std::path::Path;


std::path::Path is only used by the Linux-gated sysfs fallback, so this is an unused import on Windows (CI "Clippy (Windows targets)" fails under -D warnings). Gate it with #[cfg(target_os = "linux")].

Same fix as the std::fs comment above — fixed in 7f350da with #[cfg(any(target_os = "linux", test))] on this import too, for the same reason: the test module needs Path on every platform, not just Linux.

gpu_policy.rs's std::fs and std::path::Path imports were only used by the Linux-gated sysfs fallback (read_drm_vram_usage and friends, already #[cfg(any(target_os = "linux", test))]), so Windows clippy under -D warnings failed on both as unused imports. Gate them the same way the functions already are, rather than only to target_os = "linux", since the test module also needs them on every platform. main.rs's Stdio import had the same problem from the other direction: its only non-test caller was already #[cfg(not(windows))], and its Stdio-using test is #[cfg(unix)], so once the gpu_policy.rs extraction moved out main.rs's other (unconditional) Stdio usage, the import was left unused on Windows. ProcessCommand stays unconditional since apply_app_path_env still uses it on every platform. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

juhovainio

No blocking issues from my pass over this one. I compared the lines removed from main.rs against the lines added in gpu_policy.rs, and apart from pub(crate) visibility, imports, module docs and the test scaffolding the function bodies are unchanged. The cfg-gates (including both gpu_vram_usage_sysfs variants) moved as-is, the CPU-mode bail in parse_device_policy is intact so there is still no silent CPU fallback, and the test set moved over with the same names. CI is green.

A few non-blocking notes:

The architecture entry says "owns private types", but GpuVramUsage is pub(crate) (see inline).
The module header in gpu_policy.rs is long and largely repeats the architecture.md entry; the two will drift, so consider trimming one.
The vram/host_gpu test fixtures are duplicated across two test modules. Fine as documented, just noting it.
The PR description ends with a generated-by footer; AGENTS.md section 11 asks to avoid those, so worth dropping.

Approving based on a diff read plus the green CI.

juhovainio · 2026-10-28T15:20:46Z

 - [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs) — mechanical relocation, owns private types (Phase 6a): `engines()`, plus env-root/runtime resolution — several helpers here are also called from [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs) and from other root-level commands, so they stay `pub(crate)`.
 - [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs) — mechanical relocation, dispatch-adjacent, owns a `pub(crate)` type (as of Phase 6e's follow-up, ROCMAI-726; see the Phase 6a variant note above for the owned-types pattern). Holds the freeform natural-language planner's plan-*execution*/consent half that [`freeform_planner.rs`](../apps/rocm/src/freeform_planner.rs) below deliberately left out: `validate_freeform_execution_action()`, `prepare_freeform_execution()`, `apply_freeform_execution_consent()`, `render_freeform_execution_header()`, and the `pub(crate)` `FreeformExecution` struct that `prepare_freeform_execution` returns and `render_freeform_execution_header` consumes. `main.rs`'s `execute_freeform_next_action()` and `run_freeform()` stay in `main.rs` — both are dispatch-level, not part of deciding whether a plan's next action is safe to execute — and `execute_freeform_next_action()` calls in via the unqualified re-import `use crate::freeform_execution::{prepare_freeform_execution, render_freeform_execution_header};` (`run_freeform()` only calls `execute_freeform_next_action()`, never this module directly), matching [`driver_install.rs`](../apps/rocm/src/driver_install.rs)'s dispatch-adjacent form rather than `freeform_planner.rs`'s own qualified-path form. `prepare_freeform_execution` takes the already-built `plan` by value rather than rebuilding it from `request`/`paths`/`config` (ROCMAI-725's single-build guarantee, inherited unchanged from `main.rs`); `prepare_freeform_execution`/`render_freeform_execution_header` are `pub(crate)` for that cross-module call; `FreeformExecution` was already `pub(crate)` (read directly by `main.rs` and this module's own tests); `validate_freeform_execution_action`/`apply_freeform_execution_consent` have no caller outside this module and stay module-private, unchanged from before the move. The cluster's six regression tests moved out of `main.rs`'s shared `mod tests` into this module's own `mod tests`, inheriting the qualified `freeform_planner::` path they already used to reach `freeform_planner.rs`'s items as `main.rs`'s former tests — this module reaches in from outside using the same qualified path `main.rs` and `providers.rs`'s tests use (unlike `chat.rs`/`comfyui.rs`, which import it unqualified).
 - [`freeform_planner.rs`](../apps/rocm/src/freeform_planner.rs) — full domain extraction (Phase 6e, ROCMAI-87). Owns the freeform natural-language planner cluster's types (`StructuredRequestPlan`, `PlannerIntent`, `PlannedToolCall`, `FreeformPlanAction`, `ProviderPlannerResponse`, `ProviderPlannerToolCall`, all module-private except `FreeformPlanAction` and the fields of `StructuredRequestPlan`/`PlannedToolCall` that `main.rs` reads directly), the deterministic plan builder (`build_freeform_plan*`), the provider-assisted path (`resolve_freeform_plan_with_provider`, `build_provider_planner_prompt`, `provider_planner_response_to_plan`, `validate_provider_planner_tool_call`), and plan rendering (`render_structured_request_plan`). [`main.rs`](../apps/rocm/src/main.rs) calls `build_freeform_plan_with_context`/`render_structured_request_plan` directly for the dispatch/review flow (`main.rs`'s only other call into this module, `render_launch_summary` → `host_default_engine`, is unrelated: bare `rocm`'s non-interactive launch summary), while [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs) above calls `plan_next_action`/`format_structured_tool_call` for the execution flow and reads `FreeformPlanAction`/`StructuredRequestPlan` by type for its execution-validation functions — those live there, not here, because they decide whether a plan's next action is safe to *execute*, a dispatch-adjacent concern distinct from *building* the plan; `main.rs`'s `run_freeform` builds a plan exactly once via `build_freeform_plan_with_context` and threads that same value through `execute_freeform_next_action` to `prepare_freeform_execution` rather than rebuilding it (ROCMAI-725: building a plan can call a configured cloud `planner_provider`, which is not guaranteed deterministic, so a second independent build could silently disagree with the first). [`chat.rs`](../apps/rocm/src/chat.rs)'s `natural_language_plan` MCP tool gets the same single-build guarantee from `plan_freeform_request_with_context`, which builds once and returns both the rendered text and the next action; `chat.rs` also calls `format_structured_tool_call` directly, like [`comfyui.rs`](../apps/rocm/src/comfyui.rs) does, and its `validate_chat_rocm_command_safety` reads `serve_args_request_cpu_device` directly as a no-CPU-fallback safety check on provider-proposed `serve` args. `host_default_engine`, `build_freeform_plan_with_context`, `render_structured_request_plan`, `plan_next_action`, `plan_freeform_request_with_context`, `provider_planner_response_to_plan`, `format_structured_tool_call`, and `serve_args_request_cpu_device` are `pub(crate)` for these reasons; `freeform_plan_next_action` and `render_freeform_plan` are test-only (`#[cfg(test)]`) and `FreeformPlanAction` was already `pub(crate)` before the split. `parse_freeform_invocation`/`should_treat_as_freeform` (deciding whether an invocation is freeform at all) stay in `main.rs`, a dispatch-level concern outside both this module and `freeform_execution.rs`. The cluster's regression tests live in `freeform_planner.rs`'s own `mod tests`; the handful of `main.rs` tests that exercise the invocation classifier/clap-suggestion-avoidance stay in `main.rs`'s shared `mod tests`, since neither is specific to this module or to `freeform_execution.rs` (and neither touches `freeform_planner::` at all), and the ROCMAI-725 regression tests proving the single-build guarantee live in `providers.rs`'s `mod tests` (reusing its mock local-provider server), reaching this module's items through the qualified `freeform_planner::` path like any other external caller.
+- [`gpu_policy.rs`](../apps/rocm/src/gpu_policy.rs) — mechanical relocation, owns private types (Phase 6f, ROCMAI-214): `--device`/`--gpu` parsing (`parse_device_policy`, `parse_gpu_selection`, `parse_gpu_indices_arg`), the managed-launch-lock GPU selection (`select_gpu_indices_under_launch_lock`, `validate_pinned_gpu_index`, `effective_gpu_count`, `auto_select_gpu_indices`, `select_auto_gpu_index`, `busy_gpu_indices`), per-GPU VRAM telemetry (the `GpuVramUsage` type, `gpu_vram_usage`, `simulated_low_vram_usage`, `gpu_vram_usage_amd_smi`, `gpu_vram_usage_sysfs`, `read_drm_vram_usage`, `read_sysfs_u64`, `drm_device_is_amd`, `parse_gpu_vram_usage`), and the serve-plan low-VRAM warning (`gpu_low_memory_warning`, `vram_capacity_is_meaningful`, `serve_gpu_low_memory_warning`), plus the best-effort `amd-smi`-based GPU count (`detect_gpu_count`). `const fn device_policy_name` sits immediately after this cluster's last function in the original file but stayed in `main.rs` rather than moving — it already had callers outside the cluster (`engine_ipc.rs`, `service_launch.rs`, `serve_cmd.rs`) plus its own test. `GpuVramUsage` (and its fields), `gpu_vram_usage`, `parse_device_policy`, `parse_gpu_indices_arg`, `serve_gpu_low_memory_warning`, and `vram_capacity_is_meaningful` are `pub(crate)` and read from [`main.rs`](../apps/rocm/src/main.rs) (`classify_accelerator_memory`, `host_accelerator_memory`, `restart_child_argv`, the `dispatch()` `Command::EngineServeHttp` arm, and `collect_serve_notes` respectively); `detect_gpu_count`, `gpu_vram_usage`, `parse_device_policy`, `parse_gpu_selection`, `select_gpu_indices_under_launch_lock`, `serve_gpu_low_memory_warning`, and `validate_pinned_gpu_index` are also `pub(crate)` and read from [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs); `parse_device_policy` is also read from [`engine_ipc.rs`](../apps/rocm/src/engine_ipc.rs)'s `run_builtin_engine_serve_http`. `effective_gpu_count`, `auto_select_gpu_indices`, `select_auto_gpu_index`, `simulated_low_vram_usage`, `gpu_vram_usage_amd_smi`, `gpu_vram_usage_sysfs` (both the Linux and non-Linux `cfg` variants), `read_drm_vram_usage`, `read_sysfs_u64`, `drm_device_is_amd`, `parse_gpu_vram_usage`, `gpu_low_memory_warning`, and `busy_gpu_indices` have no callers outside this module and stay private. The cluster's regression tests live in `gpu_policy.rs`'s own `mod tests`; two small test-only fixture builders (`vram`, `host_gpu`) are duplicated rather than shared, since `main.rs`'s own `serve_notes_pair_low_vram_with_the_vllm_utilization_hint` (testing `collect_serve_notes`, which stayed) still needs them too.


This says "owns private types", but GpuVramUsage and its fields are pub(crate) (as the same entry notes further down). Consider wording it as "owns a pub(crate) type".

tomastola

No blocking findings. The relocation is mechanical: comparing the lines removed from main.rs against those added to gpu_policy.rs, the only code differences are pub(crate) visibility on the items with outside callers (and on the GpuVramUsage fields). The advised_commands.rs allowlist path and the stale "stays in main.rs" doc comments in serve_cmd.rs and engine_ipc.rs are updated. The follow-up commit 7f350da (cfg-gating the fs/Path imports in gpu_policy.rs and Stdio in main.rs for Windows) matches the cfg-gates on the items that use them, and CI is green on it, including the Windows build.

Non-blocking

The description says 33 tests moved; gpu_policy.rs has 34 #[test]s and main.rs lost 34. One of them (forced_low_vram_hook_trips_the_serve_plan_warning) is gated on e2e-test-hooks, which probably explains the 33. Worth correcting the number or noting the gated test.
New in the follow-up commit: main.rs now has use std::process::Command as ProcessCommand; and use std::process::ExitCode; as separate adjacent lines. That is fine, but rustfmt's import grouping would normally merge them, so you may prefer a single use std::process::{Command as ProcessCommand, ExitCode};.

Decisions for the author

The vram and host_gpu test fixtures are duplicated into main.rs's test module. The doc comments say why, so this reads as deliberate. The cost is two copies to keep in sync; a shared #[cfg(test)] helper is the alternative if more fixtures need this.
The gpu_policy.rs module header and the docs/architecture.md entry list every call site by function name. That matches neighbouring entries, but these lists will go stale as later phases move callers.

jussielo-amd mentioned this pull request Oct 27, 2026

ROCMAI-210: extract update-surfaces rendering into update_cmd.rs and fold automation-proposal/daemon rendering into automations.rs #752

Merged

8 tasks done

jussielo-amd added this pull request to stack #761 October 27, 2026 13:18

jussielo-amd requested a balanced review from Copilot October 27, 2026 13:32

Copilot started reviewing on behalf of jussielo-amd October 27, 2026 13:33

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

Comment thread apps/rocm/src/gpu_policy.rs Resolved

jussielo-amd force-pushed the rocmai-214-gpu-policy branch from 6ba9e65 to f469b97 Compare October 28, 2026 08:28

Base automatically changed from rocmai-213-engine-ipc to main October 28, 2026 12:44

ROCMAI-214: extract GPU/device-policy parsing and VRAM usage warnings…

39868d7

… into gpu_policy.rs Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd force-pushed the rocmai-214-gpu-policy branch from f469b97 to 39868d7 Compare October 28, 2026 13:29

jussielo-amd marked this pull request as ready for review October 28, 2026 13:30

jussielo-amd requested a review from a team as a code owner October 28, 2026 13:30

jussielo-amd requested a review from volen-silo October 28, 2026 13:30

juhovainio requested changes Oct 28, 2026

View reviewed changes

jussielo-amd enabled stacked auto-merge October 28, 2026 14:02

jussielo-amd requested a review from juhovainio October 28, 2026 14:24

juhovainio approved these changes Oct 28, 2026

View reviewed changes

jussielo-amd disabled stacked auto-merge October 28, 2026 15:40

tomastola approved these changes Oct 28, 2026

View reviewed changes

jussielo-amd added this pull request to the merge queue Oct 29, 2026

Merged via the queue into main with commit bbc3a3f Oct 29, 2026
31 checks passed

jussielo-amd deleted the rocmai-214-gpu-policy branch October 29, 2026 06:35

Navigation Menu

Repository navigation

ROCMAI-214: extract GPU/device-policy parsing and VRAM usage warnings into gpu_policy.rs - #750

ROCMAI-214: extract GPU/device-policy parsing and VRAM usage warnings into gpu_policy.rs#750
jussielo-amd merged 2 commits into
mainfrom
rocmai-214-gpu-policy

jussielo-amd commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

juhovainio left a comment

Uh oh!

juhovainio Oct 28, 2026

Uh oh!

jussielo-amd Oct 28, 2026

Uh oh!

juhovainio Oct 28, 2026

Uh oh!

jussielo-amd Oct 28, 2026

Uh oh!

juhovainio left a comment

Uh oh!

juhovainio Oct 28, 2026

Uh oh!

tomastola left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jussielo-amd commented Oct 27, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

juhovainio Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

jussielo-amd Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

juhovainio Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

jussielo-amd Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

juhovainio Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

tomastola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants