Repository navigation
docs: copyedit README, CONTRIBUTING, and vLLM; add uninstall and verify guidance - #616
pmoutsias-amd wants to merge 16 commits into
Conversation
State the default ROCm version and the supported versions in the ROCm installation intro and Getting started, and move the "ROCm 10 and newer" section ahead of driver installation. Explain that ROCm 10 or newer needs a `--version` pin and the exact GPU arch as a raw gfx code via `--family`. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Editorial pass for style and consistency: remove em dashes and spaced double hyphens, use "instead of" and "through", replace and/or and word slashes, spell out TUI on first use, use American spellings, split long paragraphs, and use absolute GitHub URLs for doc links in CONTRIBUTING.md and README.md so they resolve on the docs site. No changes to technical content or to the anchor strings the docs-site include directives depend on. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
969ee1e to
9bf2e37
Compare
Keep the rocm diagnose flag list tight by moving the long --report and --send detail below it, add colon lead-ins to the flag lists that had none, and clarify the --family sentence. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Add expected output and a PATH fallback to the verify section. Name the four uninstall categories, list managed ROCm SDK installs under --keep-data, and split the uninstall warnings into a list that also names the shared caches left in place. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Point the ROCm installation section and the site install page at Logs and cleanup, so readers who finish installing can find how to remove ROCm CLI. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
…omation modes Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Distinguish the managed uv cache from the shared one, list downloaded models under --keep-data, and include the APU case in the undetermined verdict conditions. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
rominf
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · cf49378
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
No ticket named. The description lists twelve commits and covers what the diff does. One of its claims is wrong: it says the uninstall text was "checked against build_uninstall_plan()", and the first blocking finding below shows it was not.
This is the first automated round on this PR, so every finding below is new.
Blocking
-
(new) On a default install, the new
rocm uninstalltext promises that each--keep-*flag keeps its category. It does not. —README.md:924-934; same claim atdocs/engine-plugins.md:98; root causeapps/rocm/src/main.rs:21012(build_uninstall_plan) withcrates/rocm-core/src/runtime.rs:215-227- By default the config dir and the data dir are both
~/.rocm, and the cache dir is~/.rocm/cache. The plan deletes each category's directory unless that category's flag is passed. - I ran
rocm uninstall --dry-run --keep-binarieswith a throwawayHOME:--keep-datastill plansconfig: ~/.rocm, which wipes the models, managed SDK installs anduv-cachethe README says it keeps.--keep-configstill plansdata: ~/.rocm, which contains the config.--keep-cachestill plansconfig: ~/.rocm, which containscache/.
- "Each
--keep-*flag leaves one category in place" is therefore false on the default layout. A user who trusts--keep-dataloses every managed runtime and downloaded model. - The code decides what actually happens, so the text is the side that is wrong today. The bug underneath is in the code.
- Confidence 100 · logic · Fix: either make the plan honour a kept category when directories overlap (skip deleting a directory that contains or equals a kept one), or document that on the default layout these flags only work in combination. The code fix is the better one.
- By default the config dir and the data dir are both
-
(new) The new ROCm version claims state a moving value as fixed and give a list of installable versions that is wrong today. —
README.md:228,README.md:395(README.md:228is also included intodocs/rocm-docs/getting-started.md)- The source has no allow-list of versions. Without
--version,therock.rsinstalls whatever the release index currently lists as latest. - I ran dry-runs against the live public index:
install sdk --family gfx120X-all --version 7.13.0 --dry-runresolves 7.13.0, which "It can install ROCm 7.14, 10.0, or 10.1" excludes.--version 10.0.0 --family gfx1200 --dry-runfails ("selected torchaudio 2.11.0.3+rocm10.1.0, which does not share ROCm build 10.0.0").- The default today picks 7.14.1.
- "By default … installs ROCm 7.14" becomes wrong at the next release, and
install sdk --helpsays nothing about a default version. - Confidence 85 (run-confirmed, but it depends on what the index serves today) · logic · Fix: say "the latest stable release (currently 7.14)". If 7.14, 10.0 and 10.1 are a support statement, call them "supported" rather than "can install", and recheck that 10.0 actually works before listing it.
- The source has no allow-list of versions. Without
Non-blocking
-
(new) The README now contradicts itself about downloaded models. —
README.md:557-562againstREADME.md:931-932- The new uninstall text says
--keep-datakeeps downloaded models, which is correct: they sit under<data>/models, and uninstall deletesdata_dir. - The reworded Disk space paragraph, and
rocm storage's "Shared with other tools (never removed by ROCm CLI)" heading (apps/rocm/src/storage.rs:508-514), still say they are never removed. - The code decides this, so the Disk space sentence and the storage label are the ones to fix. That claim was there before this PR.
- Confidence 88 · mechanical · Fix: limit "never removed" to the
uvand Hugging Face caches, or torocm storage's commands.
- The new uninstall text says
-
(new) Two
--helptexts now contradict the README this PR corrected (AGENTS.md §5: check the same claim on every surface). —apps/rocm/src/main.rs:299-302,apps/rocm/src/main.rs:185-195rocm chat --helpstill says it "Reads the prompt from --prompt or, if omitted, from standard input". The README now correctly says an interactive run without--promptopens the dashboard chat (main.rs:2226).rocm diagnose --helpdescribes--sendas a "prefilled issue form" that "reaches the tracker". The README correctly describes a prefilled mail (report_delivery.rs).- Both were stale before this PR.
- Confidence 85 · mechanical · Fix: update both doc comments to match the README.
-
(new) The pointer from
diagnose --model'sundeterminedlist torocm modelis incomplete. —README.md:594-595- The new Curated models section (
README.md:607-610) says the default list hides some recipes, and only--verboseshows them. assess_model_readinessmatches against every recipe, hidden ones included.- Confidence 75 · mechanical · Fix: point at
rocm model --verbose.
- The new Curated models section (
-
(new) Synopsis errors that predate this PR survive a commit titled "fix command synopses". All confirmed against the built binary's
--helpor by a run.README.md:533,rocm storage [report] [--json]:rocm storage --jsonfails with "unexpected argument '--json'". It should readrocm storage [report [--json]].README.md:298: the edited[--distro [NAME]] [--report [--send]]implies the two can be combined, but the CLI rejects--reportwith--distro.README.md:617-626: therocm servesynopsis omits--gpu-memory-utilization,--tool-call-parser,--api-keyand--require-api-key.README.md:568: therocm engines installsynopsis omits--yes.- Confidence 100 · mechanical · Fix: correct the synopses, or narrow what the commit claims to fix.
-
(new) The hand-copied sentence on the docs site was not updated with its README original. —
docs/rocm-docs/getting-started.md:35-37- The README now reads "GGUF versus safetensors rule, because which…", but the copy still says "GGUF-vs-safetensors rule, since which…".
- The include anchor is unaffected, so this is wording drift only.
- Confidence 85 · mechanical · Fix: copy the new wording over.
Decisions for the author
- Absolute
blob/mainlinks instead of relative ones — tradeoff- In favour:
CONTRIBUTING.mdand the README "More docs" list are pulled into the Sphinx site, where relativedocs/…links would break. The files already use this pattern. - Against: those links are no longer checked by the offline
docs-links(lychee) job. On a versioned docs site they always point atmain, not the release being read.
- In favour:
Positive signals
- Every
{include}slice indocs/rocm-docs/still resolves to a unique, intended span. The only changes against the base are the intended content. A localsphinx-build -Wpassed, and CI's Sphinx and lychee jobs are green. - The new
undeterminedbullets, including the APU case, matchUndeterminedReason::UnifiedMemoryUnreadableinmodel_readiness.rsexactly. - The
rocm chattext now describes the code's real split between the interactive dashboard chat and the one-prompt form (main.rs:2226). - The
docs/vllm.mdrestructure keeps every qualifier and condition. Each technical claim in it was confirmed againstengines/vllm/src/install.rs, and the(0, 1]range check was confirmed by a run.
Deployment notes
None
What this covered
- Read: every changed file in full, against
2c37f5bc…cf49378a(base merge-base7f72556d→ head), using the PR's three-dot diff. That isREADME.md,CONTRIBUTING.md,docs/vllm.md,docs/rocm-docs/getting-started.mdanddocs/rocm-docs/install/installation.md. - Also read: every
docs/rocm-docsinclude wrapper, and the CLI source behind each new claim (main.rs,therock.rs,storage.rs,model_readiness.rs,runtime.rs,apps/rocmd/src/watchers.rs,engines/vllm/src/install.rs). - Ran: the CLI built from this head, for
--helpand dry-runs, with throwawayHOMEdirs. CI status was read for head cf49378: all checks green, GPU lanes skipped as docs-only. The--keep-datadry-run in the first blocking finding was re-run independently before filing. - Fan-out: five independent workers. Three covered disjoint README ranges plus include integrity, one covered
CONTRIBUTING.mdanddocs/vllm.md, and one ran agent-instruction adherence, history, code comments, prior-PR review comments (#473, #396, #311, #579) and commit quality. - Did not run: the version findings depend on the live package index on 2026-10-09.
- Not reconciled against any discussion on this PR.
Note that --keep-* flags don't protect their files by themselves on the default layout, where config and data share ~/.rocm. State the supported ROCm versions and describe the default as the latest stable release. Correct the storage and diagnose synopses, point the undetermined list at rocm model --verbose, narrow the "never removed" claim to rocm storage, and resync the copied getting-started sentence. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
ROCm 10.1 is the latest stable release, so describe the unpinned `rocm install sdk` result as currently ROCm 7.14 and point to the `--version` and `--family` steps for 10.0 and 10.1. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
| To remove ROCm CLI and what it manages, see | ||
| [Logs and cleanup](../commands.md#logs-and-cleanup). |
There was a problem hiding this comment.
Maybe this could simply have a rocm uninstall command example, and then point to Logs and Cleanup?
| @@ -19,22 +19,22 @@ SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
This starts with installing a rocm sdk, but shouldn't it start by checking if the needed sdk is available using rocm examine? Then if you need a different version of configuration you would run rocm install sdk?
| reused as the `:start-after:` anchor for the next include, which also | ||
| matches the original sentence in README.md; edit both together. --> | ||
| Running the command when a managed runtime is already the active default asks | ||
| first, because the new install takes over as the active default; see |
There was a problem hiding this comment.
| first, because the new install takes over as the active default; see | |
| Running _rocm install sdk_ when a managed runtime is already active prompts for confirmation first, because the new install takes over as the active default; see |
There was a problem hiding this comment.
| moved to the SDK's *build* of the torch *release* that the engine pins: the release |
|
|
||
| ## ROCm 10.x wheel discovery | ||
|
|
||
| For most ROCm SDK versions, `rocm engines install vllm` pins a fixed vLLM wheel |
| host from 10.0's production index. Both rows take their torch stack from the | ||
| same `whl-next` index, which is where `rocm install sdk` resolves either SDK's | ||
| own `+rocmX.Y` torch from, but vLLM's discovery is independent of that: it | ||
| resolves its own torch/torchvision/torchaudio pins fresh from the row's own | ||
| static version prefixes, rather than reusing whatever the SDK install | ||
| resolved, which may be a different torch version than this row pins. Patch | ||
| and any dev/pre-release suffix are still ignored within a row, since AMD | ||
| resolves its own torch, torchvision, and torchaudio pins fresh from the row's | ||
| own static version prefixes, instead of reusing whatever the SDK install | ||
| resolved, which might be a different torch version than this row pins. Patch |
There was a problem hiding this comment.
Maybe break this sentence into two or three sentences?
…and vLLM - Start the "Configure ROCm" section with `rocm examine` and keyed on `active_runtime_status: ready`; `rocm install sdk` is for a different version or configuration. - Name the command in the "install over an active default" sentence, and update the hand-copied sentence and include anchor in getting-started.md to match. - Add a `rocm uninstall` example and a `--dry-run` note to the Uninstall section of installation.md. - vllm.md: say "including 7.14" for ROCm SDK 7.x pinning, "torch release", and split the long discovery sentence in three. Review comment #1 (the WSL page) is not in this commit. I removed the --keep-* caveat and the inlined qwen copy on purpose, so the message doesn't mention them. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Summary
This PR has fourteen commits: four that add content (1, 5, 6, and 9), two that add or place cross-reference links (7 and 8), three copyedit commits (2 to 4), two that correct commit 9 (10 and 11), one that corrects commits 5 and 6 and the diagnose text (12), one that addresses the automated review (13), and one that rewords the default-version text (14). Commits 2 to 4 add no new facts; commits 1, 5, 6, 9, 10, 11, 12, 13, and 14 do, and are described separately below so they can be reviewed apart from the wording changes.
1. Call out supported ROCm versions (7.14, 10.0, 10.1)
rocm install sdkcurrently installs ROCm 7.14 when--versionis omitted. The statement appears in the ROCm installation intro and in Getting started (commits 13 and 14 reword it from "default 7.14").--versionpin and the exact GPU arch as a rawgfxcode through--family, with an example (--version 10.1.0 --family gfx1200).install sdksubsections (--devel, approval prompt, install location) and ahead of the unrelated "Driver installation" and "Updates" sections. This is the only block move in the PR, and it belongs with the new pointer sentence: if the pointer goes, the move should go with it.getting-started.mdinclude anchor to match the reworded README sentence.2. Copyedit README, CONTRIBUTING, and vLLM guide
README.md,CONTRIBUTING.md, anddocs/vllm.md, which are single-sourced into the docs site.README.mdandCONTRIBUTING.mdnow use absolute GitHub URLs so they resolve on the docs site.3. Tighten README flag lists and add lead-ins
rocm diagnoseflag list tight (one paragraph per bullet) by moving the long--reportand--senddetail below the list.diagnose,fix,dash, andbench loadlists.4. Rejoin prune lock sentence and complete acceptance-script list
services pruneparagraph, so its reference to the wait is not left dangling.5. Add install verification and
rocm uninstallguidance (new content)rocm version, which prints the CLI version (release tag or branch and commit hash), the ROCm SDK this machine would use, and the GPU driver version, ornot detectedfor the last two when none is found.rocm uninstallunder "Logs and cleanup": what--dry-run,--yes, and each--keep-*flag do (commit 13 adds the default-layout caveat for the--keep-*flags), and a new--force-dev-binariesentry in the usage block.uninstallwarns about before it removes anything: shared caches it leaves alone, managed service records (their processes are not stopped), and remote sessions (runrocm remote stop <session>first).apps/rocm/src/main.rs(version(),build_uninstall_plan(), and theUninstallflag definitions), not copied from the earlier draft; commit 13 corrects one flag claim that this check missed, which saidrocm versionshowed only the CLI version and that uninstall warned about services "still running".6. Clarify verify and uninstall guidance (new content)
rocm versionrun looks like, thatnot detectedfor the SDK and driver is expected before ROCm or a driver is installed, and what to check ifrocmis not found.--keep-data, since they live in the data directory.uvpackage cache and the Hugging Face model cache; commit 12 refines this). Checked againstbuild_uninstall_plan()andshared_cache_candidates().7. Link install pages to uninstall guidance
docs/rocm-docs/install/installation.md) pointing to "Logs and cleanup", so readers can find how to remove ROCm CLI. The site-page sentence is first placed directly under Build from source; commit 8 moves it.8. Give the install page uninstall pointer its own heading
## Uninstall ROCm CLIheading so it doesn't read as part of Build from source, and keeps the existing Contributing sentence in its original place. This and commit 7 are the only changes in this PR to a site wrapper; neither adds include anchors or alters existing ones.9. Fix command synopses and document
rocm model,rocm chat --provider, and automation modes (new content)--send(requires--report) torocm diagnoseand the positional[QUERY ...]torocm logs.rocm model [--verbose](aliasrocm models), which the "Will a model run here?" text already referred to, and splits that text'sundeterminedconditions into a list.anthropic|openai|...placeholder withlocal|openai|anthropic, and documents--providersetup (rocm config enable-providerandset-provider-key) and--tools.observe,propose, andcontained. Incontainedmode onlyserver-recoveracts on its own (restarts a failed managed service); the others stay review-gated or record only, percrates/rocm-coreandapps/rocmd/src/watchers.rs. Per-watcher default modes are left out to avoid a table that can drift.apps/rocm/src/main.rs.10. Clarify that
rocm chat --providerapplies only to one-prompt userocm chatwithout--promptopens the dashboard chat, where you switch providers with/provider <name>. With--promptor piped input, it sends one prompt and prints the reply.--provideris ignored by the dashboard chat, and thatlocalis the default.Command::Chathandler inapps/rocm/src/main.rs.11. Correct contained-mode, chat flag, and model verbose descriptions
--modeland--tools, which the dashboard chat also ignores, and states it once for all three flags.containeddescription, which overstated that every watcher applies changes on its own.proposenow says it applies to watchers that can make a change.rocm model --verbose: describes what it prints (every recipe, including hidden ones, with aliases, data type, minimum GPU memory, and engines) instead of repeating the flag's help text.Command::Chathandler inapps/rocm/src/main.rs, and against the watcher code incrates/rocm-coreandapps/rocmd/src/watchers.rs.12. Clarify uninstall cache and data scope, add APU case to diagnose
uvcache (under the data directory, controlled by--keep-data) from the shared one (UV_CACHE_DIRor~/.cache/uv, left in place and reported). Lists downloaded models under--keep-data, since they live in the data directory and uninstall removes them.undeterminedconditions now include the APU case, where the CLI can't yet read the memory pool the engine uses. Checked againstUndeterminedReasoninmodel_readiness.rs.13. Address automated review: uninstall default layout, ROCm versions, synopses
~/.rocmand the cache is~/.rocm/cache, so a single--keep-*flag doesn't protect its files. The intro now says each flag "skips the removal of one category", and a new paragraph says to pass--keep-configand--keep-datatogether and to run--dry-runfirst.docs/engine-plugins.mdgets the same caveat. The earlier "checked againstbuild_uninstall_plan()" claim had verified each flag in isolation, not the overlapping default layout. The behavior itself is tracked in uninstall: --keep-* flags are ignored when config, data, and cache directories overlap (default layout) #626.rocm storage, becauserocm uninstalldoes remove downloaded models unless you pass--keep-data.rocm diagnose [--symptom TEXT] [--top N] [--json] [--distro [NAME] | --report [--send]]androcm storage [report [--json]]. Theundeterminedlist now points torocm model --verbose.getting-started.md: resyncs the hand-copied sentence with the README.14. Avoid calling 7.14 the latest stable release
rocm install sdk"currently installs ROCm 7.14" without--version, and that 10.0 or 10.1 need--versionand--family.Test plan
{include}directives indocs/rocm-docs/still find their:start-after:and:end-before:strings, in order, against the edited files.Signed-off-bytrailer.rocm install sdk --version 10.1.0 --family gfx1200 --dry-runresolves on WSL2 (run 2026-10-09): ROCm 10.1.0, torch 2.14.0, torchvision 0.28.0, and torchaudio 2.11.0.3, all+rocm10.1.0, with no error. A review dry-run the same day showed an unpinned install picking 7.14.1. The dry-run on WSL2 did not use a gfx1200 GPU. A review dry-run of--version 10.0.0 --family gfx1200failed on a torchaudio mismatch (it selected the 10.1.0 build).rocm uninstallwarns about remote sessions,containedmode acts on its own only forserver-recover, and the dashboard chat ignores--modeland--tools.docs-buildpasses in CI.