Skip to content

Add Sphinx documentation site - #311

Merged
pmoutsias-amd merged 25 commits into
mainfrom
docs/add-sphinx-documentation-site
Aug 27, 2026
Merged

Add Sphinx documentation site#311
pmoutsias-amd merged 25 commits into
mainfrom
docs/add-sphinx-documentation-site

Conversation

@pmoutsias-amd

Copy link
Copy Markdown
Contributor

Summary

  • Adds a Sphinx documentation site under docs/rocm-docs/, single-sourced from README.md and CONTRIBUTING.md via MyST {include} directives so the docs and repo content stay in sync automatically
  • Covers installation, getting started, command reference, demos, and contributing/license, using the rocm-docs-core theme
  • Updates some links in README.md and CONTRIBUTING.md to be absolute so they render correctly when reused via {include}

Test plan

  • Build the docs locally (pip install -r docs/rocm-docs/sphinx/requirements.txt && sphinx-build docs/rocm-docs docs/rocm-docs/_build) and spot-check each page renders correctly
  • Confirm README.md and CONTRIBUTING.md still render correctly on GitHub with the updated absolute links

@pmoutsias-amd
pmoutsias-amd requested a review from a team as a code owner August 25, 2026 17:27

@tomastola tomastola left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach — single-sourcing from README.md/CONTRIBUTING.md via MyST {include} avoids the usual drift between the repo and the docs site. I built the site locally (rocm-docs-core resolved to 1.40.0) and it does build; the pages render and the section anchors land where you intended. A few things to resolve before this is mergeable.

Blocking — CI is red

1. License header check (hawkeye) — 7 new files are missing the MIT header:

docs/rocm-docs/conf.py
docs/rocm-docs/commands.md
docs/rocm-docs/demos.md
docs/rocm-docs/getting-started.md
docs/rocm-docs/about/contributing.md
docs/rocm-docs/about/license.md
docs/rocm-docs/install/installation.md

For the .md files that's the HTML-comment block already used across docs/*.md:

<!--
Copyright © Advanced Micro Devices, Inc., or its affiliates.

SPDX-License-Identifier: MIT
-->

and the #-comment equivalent for conf.py. index.rst, _toc.yml.in, and requirements.txt aren't in licenserc.toml's includes, so they're fine as-is. hawkeye check reproduces this locally.

2. Commit signatures + sign-off — both commits are unsigned and carry no Signed-off-by trailer:

065f231 N | signoff:
1f5d607 N | signoff:

Needs a re-sign plus git rebase --signoff (or recommit with -s -S) and a force-push.

Real defect

3. The platform-support table has a dead link on the install page. docs/wsl.md is the one relative link in an included region that wasn't converted, and it's the only one that actually breaks:

README.md:1056: WARNING: 'myst' cross-reference target not found: 'docs/wsl.md' [myst.xref_missing]

It renders as <a href="#docs/wsl.md">docs/wsl.md</a> — an in-page anchor to nothing, so clicking it does nothing on the install page. (docs/vllm.md at README:41 and LICENSE.TXT at README:10 have the same shape but fall outside every include range, so they're currently harmless — worth deciding whether to normalize them too or leave them.)

Gaps worth settling before merge

4. Nothing builds or publishes this site. There's no .readthedocs.yaml at the repo root, so as it stands this adds docs sources that never become a site. Is the RTD project config landing in a follow-up, or should it be part of this PR?

5. No CI job builds the docs, and this design specifically needs one. The includes are anchored on prose strings — :end-before: "See [CONTRIBUTING.md]", :end-before: "<!--", and heading text. Rewording that one sentence in README.md, or adding an HTML comment anywhere inside the Demos section, silently truncates or empties a page with no signal anywhere. That's the cost of single-sourcing, and a sphinx-build -W job is what makes it safe. Related: the build currently emits 16 Non-consecutive header level increase; H1 to H3 warnings (the included chunks start at ### because start-after drops the ##). I checked the rendered output — docutils normalizes them, so pages get correct <h1>/<h2> and the in-page TOCs are right; they're cosmetic. But they'd block -W, so they need suppressing or restructuring as part of adding the job.

Minor

6. rocm-docs-core>=1.7.1 is unpinned — it resolved to 1.40.0 for me. ROCm docs repos generally pin == so a docs build can't break from an upstream release landing.

7. Two different strategies for the same link problem. README.md anchors were rewritten to absolute https://github.com/ROCm/rocm-cli#... URLs, while installation.md solves the same problem the opposite way — cutting at :end-before: "See [CONTRIBUTING.md]" and re-authoring a docs-relative link. The absolute-URL route works, but it has costs on the primary surface: on GitHub, #model-serving and #interactive-interfaces used to be in-page scrolls and are now full page loads that always resolve against main, and they're wrong on a fork or a tag. On the docs site they send the reader off-site entirely. Since installation.md shows the cut-and-re-author pattern works, is it worth using that consistently and leaving the README anchors relative?

Adds a Sphinx documentation site under docs/rocm-docs/, single-sourced
from README.md and CONTRIBUTING.md via MyST {include} directives so
the docs and repo content stay in sync automatically. Covers
installation, getting started, command reference, demos, and
contributing/license, and is set up to publish with the rocm-docs-core
theme. Updates some links in README.md and CONTRIBUTING.md to be
absolute so they render correctly when reused via {include}.

Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Anchor the table in README.md and pull it into
docs/rocm-docs/install/installation.md via {include} instead of
duplicating it by hand.

Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
@pmoutsias-amd
pmoutsias-amd force-pushed the docs/add-sphinx-documentation-site branch from 065f231 to 911da53 Compare August 26, 2026 13:45
@pmoutsias-amd

Copy link
Copy Markdown
Contributor Author

@tomastola Thanks for the thorough review — believe I've addressed all 7 items:

  1. License headers — added the MIT header block to all 7 flagged files (conf.py, commands.md, demos.md, getting-started.md, about/contributing.md, about/license.md, install/installation.md).
  2. Signed/signed-off commits — all commits are now signed (SSH, verified) and carry Signed-off-by.
  3. docs/wsl.md dead link — fixed, now points to the absolute GitHub URL. Left vllm.md/LICENSE.TXT as relative links since they fall outside the include ranges and aren't affected the same way — happy to normalize those too if you'd rather.
  4. .readthedocs.yaml — added, pointing at docs/rocm-docs/conf.py and the pinned requirements file.
  5. CI docs build with -W — added a docs-build job gated on doc-path changes; it's passing clean (also cleaned up the non-consecutive header warnings you flagged).
  6. rocm-docs-core pinning — pinned to ==1.40.0 (what it resolved to locally).
  7. Link-fix strategy — unified on absolute GitHub URLs throughout.

One heads-up: there's a small follow-on PR (#312) with an editorial pass from Anisha still open against this branch — not touching anything above, but once it merges in, branch protection may dismiss your approval on the new commits. I'll ping you again then if that happens, but it should be a quick recheck.

@pmoutsias-amd

Copy link
Copy Markdown
Contributor Author

Added editorial updates from @anisha-amd.

- Add the MIT license header to the 7 new files hawkeye flagged as
  missing it.
- Fix the WSL2 platform-support-table link to docs/wsl.md: it's now an
  absolute GitHub URL so it resolves when the table is pulled into
  install/installation.md via MyST include, matching the pattern
  already used for the other README anchors.
- Pin rocm-docs-core to the exact version that resolved locally
  (1.40.0) instead of an open->= range.

Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
- Add .readthedocs.yaml so RTD builds against docs/rocm-docs/conf.py
  with the same pinned rocm-docs-core toolchain CI uses.
- Add a docs-build CI job that runs sphinx-build -W (warnings as
  errors) against the same source RTD points at, gated on the new
  `docs` path-filter category so it only runs when doc sources,
  README.md/CONTRIBUTING.md (single-sourced via MyST includes), or
  the build config change.
- Suppress the myst.header warning from :start-after:/:end-before:
  README includes stripping the anchor heading, which is cosmetic:
  docutils re-normalizes the resulting heading depth in the rendered
  output.
- Scope external_projects to [] instead of the implicit "all" default,
  which was fetching intersphinx inventories for every project in
  rocm_docs' bundled catalog (~90 projects) - several of which 404 or
  have moved upstream. No page here uses a cross-project intersphinx
  role, so this is lossless and makes -W builds deterministic instead
  of flaky against infrastructure this repo doesn't control.
- Filter the "current project not found in projects" warning: rocm-cli
  isn't registered in rocm-docs-core's shared projects catalog yet, so
  external_projects_current_project can never resolve until that's
  added upstream. rocm_docs.projects handles the unresolved project as
  None everywhere it's used, so this is informational, not a defect.

Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
README.md's own #model-serving and #interactive-interfaces anchors were
rewritten to absolute GitHub URLs so they'd still work once pulled into
the docs site, but that breaks in-page anchor scrolling when the README
is viewed on GitHub. Revert both to relative anchors and instead apply
the cut-and-reauthor pattern already used for the CONTRIBUTING.md link
in installation.md: narrow the surrounding {include} in getting-started.md
and commands.md around each sentence and hand-author a docs-relative
replacement (commands.md#model-serving, getting-started.md#interactive-interfaces).

Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Avoids the sidebar rendering as duplicated Demos > Demos for the
single-entry section by overriding the nav link text to match the
homepage tile's wording.

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
Joins the "single prebuilt binary" fragments into one complete
sentence in both README.md and the standalone index.rst homepage
copy, and adds a lead-in sentence before the bare `rocm` code fence
in the Getting Started "First run" section.

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
@pmoutsias-amd
pmoutsias-amd force-pushed the docs/add-sphinx-documentation-site branch from e844c87 to 42abffa Compare August 26, 2026 18:31
Sentence-case headings, expand DCO acronym, replace banned/informal
wording (may, e.g.), normalize slash usage, lowercase version
placeholder.

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
@pmoutsias-amd
pmoutsias-amd force-pushed the docs/add-sphinx-documentation-site branch from 42abffa to 77ea773 Compare August 26, 2026 20:55

@michaelroy-amd michaelroy-amd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 77ea773c240263b1a1fa609ae9b70959c3fb5644 after the follow-up commits. The prior blockers are addressed: required MIT headers are present, commits/sign-offs validate, .readthedocs.yaml and a pinned rocm-docs-core==1.40.0 requirement exist, Sphinx runs with -W, the included docs/wsl.md reference no longer produces a dead anchor, and the include-boundary/link strategy is internally consistent. All 17 required checks pass, including the Sphinx docs build, DCO, licensing, and link-sensitive build. One non-linked docs/vllm.md code reference is a minor usability follow-up, not a broken link. No blocking finding. Approving.

@pmoutsias-amd

Copy link
Copy Markdown
Contributor Author

Pushed 3 README updates on top of the approved changes:

  1. Install channel reframing — Linux/WSL and Windows install snippets now default to the release channel, with nightly as the opt-in flag/env var, reflecting that a stable release is now published.
  2. New rocm examine reference section — documents --json and --framework auto|pytorch|llama-cpp|skip.
  3. Bonus: Configuration section description — added a short lead-in explaining what rocm config covers (default engine/runtime, telemetry opt-in, chat provider setup).

Flagging since this PR was already approved — the push didn't reset that, so no re-review required, just visibility on the new content.

volen-silo and others added 11 commits August 27, 2026 13:33
* fix(lemonade): retry interrupted backend setup

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

* ci(e2e): build prebuilt lanes with the test-hooks feature

The suite's deterministic failure seams are compiled out without
`rocm/e2e-test-hooks`. `cargo xtask e2e` passes the feature only when it
builds the binaries itself, so every lane that pre-builds and exports
ROCM_CLI_BINARY silently tested a binary without those seams. The
scripted-Lemonade-failure scenario then never reached its premise and
failed as a regression on the one lane that selects it.

Pass the feature on all prebuilt e2e lanes, and add a workflow-contract
test so a lane cannot drift back. Release and packaging builds keep the
feature off — a shipped binary must not carry a failure-injection seam.

Also name the cause in the retry assertion, which otherwise reports a
baffling missing announcement rather than the real misconfiguration.

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

---------

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
The hook pins in .pre-commit-config.yaml were bumped by hand and drifted
between releases. Dependabot's pre-commit ecosystem resolves each rev
against the hook repository's tags, and skips the builtin and local
blocks, which have no upstream release to track.

Weekly and grouped, with the same 7-day cooldown as the other two
ecosystems: a hook is executable code that runs on every contributor's
machine at commit time and in the prek CI job, so a compromised release
would run before anyone reads the bump.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* fix: raise the supported Linux minimum to Ubuntu 24.04

Every published Lemonade embeddable, v10.2.0 through v11.5.2, is linked
against GLIBC_2.38 and GLIBCXX_3.4.32. Ubuntu 22.04 ships glibc 2.35, so
the Lemonade engine cannot start there at all, yet README.md advertised
Linux x86_64 as "full support ... both inference engines" and docs/wsl.md
named 22.04 as a supported WSL base.

Document a single minimum of Ubuntu 24.04 for Linux and WSL2, keeping the
glibc number as the reason so the requirement stays meaningful on other
distributions, and drop 22.04 from the WSL preflight's supported set so the
automated check matches the prerequisite it enforces.

Fixes #258

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>

* fix(wsl): accept Ubuntu releases after 24.04

Signed-off-by: Michael Roy <michael.roy@amd.com>

---------

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
Co-authored-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
A scenario reaches the shared pre-warm tree through a symlink at its own
`data/runtimes`. An `install sdk` run that way writes the link's path — a
per-scenario temp dir — into the manifest that lands in the SHARED
registry, and into the venv's console-script shebangs. The temp dir goes
away with the scenario and the shared runtime is left naming a folder
that no longer exists, so later, unrelated runs fail.

Drop such a runtime before deciding whether the tree is fresh, so the
pre-warm reinstalls instead of serving a dead one. `rocm update` cannot
see this: it compares versions against the index, not the runtime
against the disk.

Removing the folder is the repair, not a precaution. A poisoned venv
keeps a working `bin/python` — a symlink to the base interpreter, still
present — so the install reuses it and audits already-satisfied packages
rather than reinstalling them, leaving every shebang pointing at the
folder that went away. Measured on uv 0.9.30: reinstalling over a
poisoned venv reports success and repairs nothing.

The signal is an install root outside the tree, deliberately not
`status=unusable`. Unusable has many causes — a missing rocm_sdk probe
block alone reports it — and this deletes what it selects, so a healthy
multi-GiB runtime must not hang on a validation detail. Read-only
runtimes are exempt: `runtimes adopt` records an external folder on
purpose.

Refs: #315

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* fix(install): record the folder the files land in, not the link

`install sdk` built the install root by joining onto the data dir and
recorded whatever that produced. Reaching `data/runtimes` through a
symlink is enough to make that name a route rather than a place: correct
while the link exists, and wrong the moment it goes.

The path outlives the command. It goes into the registry manifest, the
sidecar beside the runtime, and — through uv — the `#!` line of every
console script in the environment. So a runtime installed that way keeps
reporting itself installed at a folder that is not there, while the files
sit untouched next door and every entry point fails to start.

Resolve the root before anything is written to it. Only the root:
`python_executable` is derived from it and must keep the venv's own
`bin/python`, which is itself a symlink to the base interpreter, so
resolving that would record the system Python instead. The adopt path
already draws the line in the same place, canonicalizing the install root
next to `absolute_existing_file_path_preserving_symlink` for the
interpreter; this gives the install path the same treatment, `--prefix`
included.

The E2E harness creates exactly this shape when a scenario opts into the
shared pre-warmed runtime, so the shared tree on a runner was what got
poisoned — but nothing about the fault is test-only. Any data dir reached
through a link has it.

Refs: #315
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

* fix(vllm): name the missing interpreter behind a failed launch

Two messages described a stale runtime as an absence.

A spawn that fails on a console script whose `#!` interpreter is gone
reports ENOENT against the script, so the error named a file that is
plainly there. Say which interpreter is missing when that is what
happened, and stay quiet when the ordinary reading is right — a genuinely
absent file, a binary, or a live interpreter.

Resolution drops a registered runtime whose recorded interpreter is not
there. That is correct — a runtime that cannot run is not a candidate —
but it left the failure describing an empty registry while `rocm runtimes
list` prints the entry. Name the manifests that were passed over and the
interpreters they record.

Both are additive: the note only ever decorates an error already being
returned, and a registry it cannot read yields no note rather than
replacing the original failure.

First tests for this area, which had none.

Refs: #315
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

* test(install): compare resolved paths, not verbatim ones, on Windows

The Windows lane failed on the prefix rather than the behaviour.
`canonicalize` hands back `\\?\C:\…` there, so a test that built its
expectation that way compared a verbatim path against the plain one the
resolver deliberately returns, and three assertions turned on the
difference.

Build the expectations with the resolver instead, which strips the prefix
for the same reason the product does: a stored path is later compared
against ordinary ones, and `\\?\C:\…` never `starts_with`-matches `C:\…`.
That the prefix really is stripped stays asserted on its own, so this does
not become the function agreeing with itself.

This also removes a second Windows-only trap: `temp_dir()` there can
return an 8.3 short path, which `canonicalize` expands, so the two sides
disagreed on the folder name as well as the prefix.

Same fault in the scenario's step definitions, where the comparison would
have failed only on the Strix Windows lane. That crate cannot reach
`rocm-core`, and a dependency for six lines of string handling is the
worse trade, so it strips the prefix locally with the reason recorded.

Refs: #315
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

* fix(install): give up on a parent component consistently across hosts

The guard against resolving a `..` past a missing directory never fired
on Windows, because the two platforms disagree about what such a path
means. Windows collapses `..` lexically, so `<root>/missing/..` "exists"
and canonicalizes to `<root>`; Unix walks the path through the filesystem
and finds nothing. The walk therefore stopped a level early on Windows,
resolved to the wrong folder, and re-attached the tail — recording a
different folder depending on the host, which is precisely what this
function exists to prevent.

Check for the parent component before asking whether the candidate
exists, so both hosts give up in the same place.

The give-up stays narrow: a `..` whose parent is really there is
unambiguous everywhere and still resolves, which is now pinned by its own
test so a future tightening cannot quietly leave ordinary paths
unresolved.

Found by the Windows lane; it does not reproduce on Linux.

Refs: #315
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

---------

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
#307)

* fix(install): resolve driver repo version and package release in plans

The driver install plan stored the repo version and package release as
raw shell parameter-expansion templates (${ROCM_CLI_AMDGPU_VERSION:-7.2.4}
and ${ROCM_CLI_AMDGPU_PACKAGE_RELEASE:-70204}). Two problems followed.
The human-readable plan summary printed the version template verbatim on
its "repo_version:" line, leaking an unexpanded shell placeholder into
user-facing dry-run output. And the Debian/Ubuntu apt source line wrapped
the template in single quotes, so the shell never expanded it and the
literal placeholder would have been written into
/etc/apt/sources.list.d/amdgpu.list.

Resolve both templates once, at plan-build time, to their effective
values (the env var when set and non-empty, matching shell :- default
semantics, otherwise the documented default) and bake the concrete values
into every rendered URL, repo path, and command. The resolver is
generalized to resolve_shell_default_template and hardened to pass bare
${VAR} and nested-default shapes through unchanged rather than emitting a
partially rewritten value.

Add unit tests for the resolver (default, env override, empty-as-unset,
non-template pass-through, bare-var and nested-default pass-through) and
update every distro plan test to assert the concrete values, plus a
render-level regression test and an end-to-end scenario asserting the
dry-run summary shows the resolved version rather than the raw
placeholder. Tests that read these process env vars serialize on a shared
lock and save/restore prior values so they stay deterministic under
edition-2024 env semantics.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(install): guard the two remaining env-reading driver-plan tests

build_driver_install_plan now resolves the repo-version template via
std::env::var at the top of the function, so every test calling it reads
process env. The sweep that added ScopedTestEnv::with_amd_overrides_cleared()
to the other call sites missed windows_install_driver_is_validate_only and
wsl_install_driver_uses_rocdxg_guidance_without_dkms, leaving two unguarded
readers racing the guarded mutators in the same test binary. Add the guard to
both. No production change.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

---------

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
…306)

* fix(cli): accept negative float flag values in space form

The space form of negative-number float flags (e.g. `serve --temperature
-1`) was rejected by clap with a confusing "unexpected argument '-1'"
error, because clap parsed the leading-dash token as a flag rather than a
value. Only the equals form (`--temperature=-1`) reached the range
validator that reports the valid range.

Enable `allow_negative_numbers` on the affected float flags
(`--temperature` and `--top-p` on both `chat` and `serve`) so the space
form reaches the value parser and both forms validate identically,
surfacing a clear range-validation message instead of an unexpected
argument error.

Add regression tests covering both the space and equals forms for
`--temperature` and `--top-p` on `chat` and `serve`.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(cli): extend negative-value fix to --max-tokens and add serve scenario

The negative-number-as-flag gotcha is a tokenizer-level ambiguity that
fires before any value parser runs, so it is not float-specific:
`--max-tokens -1` (space form) on both chat and serve was still rejected
as an unexpected argument while `--max-tokens=-1` reached parse_positive_u32.
Add allow_negative_numbers to --max-tokens on both commands so the space
form reaches the value parser and reports a clear error, matching the
--temperature/--top-p behavior fixed here.

Per AGENTS.md §3, the user-observable stderr change is now covered by an
ungated Gherkin scenario, @id:serve-negative-temperature-rejected, in
model_serving.feature. The value parser runs inside argument parsing
before engine selection or GPU pre-flight, so the scenario needs no GPU
and no engine and gates every PR (mirrors the ungated
@id:fix-position-argument-rejected precedent).

Also: reword the shared rationale comment to cover all three numeric value
flags (and note clap only treats a token as negative when the whole
remainder parses as one, so missing/malformed values still error at parse
time), give the chat regression test a symmetric comment, and extend both
space-form regression tests with the --max-tokens cases.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(e2e): number the negative-temperature serve scenario

The new scenario was the only unnumbered title in model_serving.feature;
every other scenario uses 'Scenario: <N> - <Title>'. Assign the next free
number (15) to keep the file consistent. No behavior change.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

---------

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* fix(serve): validate --device via clap possible values (exit 2)

Model `rocm serve --device` as a clap `ValueEnum` (`DevicePolicyArg`) so an
invalid value is rejected by clap's usage validation (exit code 2) with the
valid choices listed, instead of parsing as a free-form string and failing
later in application logic with a generic "unsupported device policy" error
(exit code 1).

This makes `--device` consistent with every other enum-style argument in the
CLI (`--engine`, config set-*, engines install, etc.), all of which use clap
possible values. The historical aliases (`auto`/`gpu` for gpu_required, `cpu`
for cpu_only) stay accepted for backward compatibility but are hidden from the
advertised list, and the intentional cpu_only app-level rejection (exit 1 with
a GPU-required message) is preserved.

Adds regression tests asserting invalid `--device` is a clap InvalidValue usage
error and that every value and alias still parses, and reworks the device
possible-values sync test to read clap's structural possible values rather than
a hand-written doc string.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(serve): correct --device help text and hide cpu_only choice

The per-variant ValueEnum doc comments render in `serve --help`, shell
completions, and the invalid-value suggestion list. `gpu_preferred`
previously read "Prefer a ROCm GPU when one is available", which is
misleading: parse_device_policy maps gpu_preferred to GpuRequired and
rocm serve has no CPU fallback path. Reword it to state it behaves
identically to gpu_required.

cpu_only is always rejected (rocm serve requires GPU execution), so hide
it from the advertised choices with #[value(hide = true)] while keeping it
parseable so the deliberate rejection message is preserved.

Add device_policy_arg_maps_through_parse_device_policy to guard the
DevicePolicyArg::as_policy_str -> parse_device_policy mapping (gpu_required
and gpu_preferred resolve to GpuRequired; cpu_only is rejected), closing
the previously untested conversion path.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(serve): stop advertising hidden cpu_only device value

Hiding `cpu_only` from `--device` help/completion left three surfaces still
presenting it as a real choice. Align them with what the CLI now advertises:

- README serve synopsis dropped `cpu_only` from the --device value list.
- Removed the '--gpu is ignored with --device cpu_only (the model runs on CPU)'
  sentence: cpu_only is rejected (exit 1), never runs on CPU, so the line
  advertised a fallback path that does not exist (AGENTS.md §6).
- Corrected the drifted test comment to note cpu_only is #[value(hide = true)]
  and that serve_device_help_lists_match_device_policy_names compares against
  the full DevicePolicy set (hidden entries included).

Doc/comment only; no behavior change.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

---------

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
…ff critical path (eai-8190) (#295)

* fix(dash): seed launcher front door with live serving instances

The launcher front door built an empty AppState, so it always rendered
the idle variant even when a model was actively serving. Read the
managed-service registry (the same authority `rocm services` reads)
once per hub-loop pass and seed the AppState's instances from it.

Also treat `Ready` as serving everywhere the dashboard counts running
instances (`is_serving()`), matching the `Running`+`Ready` treatment
already used elsewhere (e.g. home.rs) -- a served model reports
`Ready`, not `Running`, so the count previously undercounted actual
serving models.

Adds apps/rocm's direct dependency on rocm-dash-core (previously only a
transitive dep) so the launcher can build `Instance`s from the
registry's `DiscoveredService` records.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(dash-daemon): run amd-smi detection off the run loop's critical path

Detecting amd-smi (`amd-smi version` plus the first `system_info()`)
can take up to ~15s on real hardware. Running it inline before the run
loop's first tick blocked managed-service discovery and the first
snapshot broadcast behind it, so an already-running model did not
surface in the dashboard until GPU detection finished -- a visible
~15-20s "0 models running" lag while `rocm services` already reported
it live.

Spawn detection in the background and adopt the result via a oneshot
channel the moment it lands, without ever blocking the loop while it
is in flight. The loop now starts ticking immediately, so serving
instances appear within one discovery tick; GPU metrics fill in once
detection completes.

Adds a regression test asserting on ordering (the instance must
surface in a snapshot whose gpu_system_info is still None) rather than
wall-clock timing, since a pure "arrived within Ns" check would be
flaky under subscriber starvation.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(dash): exercise off-critical-path detection and launcher front door

Address review feedback on the amd-smi-off-critical-path change.

- Gate the "amd-smi unavailable" warning behind gpu_init_done so a healthy
  host never flashes it during the detection window; settle as unavailable
  (and say so once) if the detection task ever ends without a result.
- Add an amd_smi_skip_kfd_preflight test seam so the daemon regression test
  drives a fake amd-smi through the real detection path instead of
  short-circuiting on a GPU-less CI host (the /dev/kfd guard stays mandatory
  in production). The test now genuinely fails if detection moves back onto
  the critical path, and asserts the surfaced snapshot carries no premature
  "amd-smi unavailable" warning.
- Add direct unit tests for launcher_serving_instances (ready record maps to
  a live Instance; unbound :0 record is dropped) and a behavioural launcher
  scenario driving bare rocm through a PTY to prove the front door shows
  "Serving <model>" rather than "Idle" for a live registry service.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

---------

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
check_4_render_group built the render-group remediation command from the
/dev/kfd owner group obtained via `stat -c %G`. When the device GID has no
matching group name, stat prints the literal "UNKNOWN", and the existing
guard only rejected empty strings. That let UNKNOWN leak into the suggested
fix, producing `sudo usermod -a -G UNKNOWN,video "$USER"` in the plan,
summary, and --json output -- a command that fails because no such group
exists.

Reject "UNKNOWN" (case-insensitive) alongside empty values so the group
falls back to "render". This makes diagnose's fallback coincide with the
"render,video" that fix.rs hardcodes in the UNKNOWN/empty case; it does not
unify the two group sources, which are still computed independently and can
diverge when /dev/kfd has a real group name that is not "render".

Add a regression test covering both the "UNKNOWN" and "unknown" sentinel
casings falling back to the render group.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
…entation-site

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
…ment rocm examine, describe Configuration section

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
Not one of the commands being hardened for this release; team decision
was to drop it from the reference for now rather than document it.

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
@pmoutsias-amd
pmoutsias-amd force-pushed the docs/add-sphinx-documentation-site branch from 35d4580 to b7300c3 Compare August 27, 2026 17:35
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
…entation-site

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
use_repository_button and use_download_button rely on rocm-docs-core
resolving a repository_url from the current git branch. GitHub
Actions' pull_request checkout leaves a detached HEAD on the
synthetic merge ref, so get_branch() returns an empty URL, and
sphinx_book_theme crashes trying to unpack it, failing the -W build.

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
- Broaden the known-warning suppression filter to also cover the
  "rocm-ai" flavor, which isn't in the published rocm-docs-core
  release yet and would otherwise fail the -W build even though
  rocm_docs.theme already falls back to "rocm" gracefully.
- Re-enable use_repository_button/use_download_button, pinning
  repository_url/repository_branch explicitly instead of letting
  rocm-docs-core infer them from the local git branch. CI's
  pull_request checkout leaves a detached HEAD on the synthetic merge
  ref, which resolves to an empty repository_url and crashes
  sphinx_book_theme's repository button; use_download_button was
  never actually affected by this, it only reads local source files.

Verified by building against the pinned rocm-docs-core==1.40.0 in a
clean venv (matching CI) -- build succeeds with 0 warnings.

Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
@pmoutsias-amd
pmoutsias-amd added this pull request to the merge queue Aug 27, 2026
Merged via the queue into main with commit 0fbd395 Aug 27, 2026
20 checks passed
@pmoutsias-amd
pmoutsias-amd deleted the docs/add-sphinx-documentation-site branch August 27, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants