Skip to content

Fix/rocm therock version resolution - #2803

Open
Bekhouche wants to merge 13 commits into
lemonade-sdk:mainfrom
Bekhouche:fix/rocm-therock-version-resolution
Open

Fix/rocm therock version resolution#2803
Bekhouche wants to merge 13 commits into
lemonade-sdk:mainfrom
Bekhouche:fix/rocm-therock-version-resolution

Conversation

@Bekhouche

@Bekhouche Bekhouche commented Jul 24, 2026

Copy link
Copy Markdown

Summary

llamacpp.rocm_bin="latest" (or any explicit tag other than the pinned one) could resolve to a llama.cpp release built against a newer ROCm/TheRock version than the statically pinned therock.version, causing the download filename to embed the wrong ROCm version and 404. This bumps the immediate stale pin, then makes the version resolution self-verifying against the actual GitHub release instead of trusting a static value that can silently drift.

Fixes #2800

Changes

  • Quick fix: bump llamacpp.rocm-stable and therock.version to a verified-matching pair (b10108/7.14).
  • Dynamic discovery: when the resolved llama.cpp tag doesn't match the static pin (or even just to self-verify), look up the real ROCm version from the resolved release's own GitHub asset name instead of trusting the static config. Falls back gracefully offline; throws a clear error if verification is genuinely needed but fails online, instead of silently reconstructing the original 404.
  • TheRock runtime package: the discovered version now also determines which TheRock runtime gets installed and which one gets picked up at launch (previously only the download filename was fixed; the runtime library path could still resolve to a stale, mismatched version already on disk).
  • Persistence: the resolved version is written to a marker file alongside the install, so a later launch, in a different process, possibly offline, can recover it without redoing live discovery (which may not even be possible offline).
  • Incomplete-install hardening: get_therock_lib_path() now requires version.txt to exist and match before trusting a directory; failed TheRock downloads no longer leave a directory behind that could be mistaken for a real install.
  • Scoped to recipe=="llamacpp" only. sd-cpp has the same underlying bug, but its releases can ship multiple ROCm-versioned assets per tag, so naively picking the first match risks silently choosing the wrong one. Left as a documented follow-up requiring arch-aware disambiguation.

Test plan

  • Verified live against real GitHub release data (confirmed the reported 404 reproduces exactly, and that the fix resolves to a real, downloadable asset)
  • Built and tested on WSL/Linux and natively on Windows (VS 2026/MSVC)
  • Reproduced the original bug pattern (explicit tag diverging from the static pin) and confirmed self-correction
  • Verified rocm_bin="latest", default/builtin, offline-graceful-fallback, and online-hard-failure paths
  • Verified the persisted marker survives a simulated restart/offline scenario, on both platforms
  • No automated regression tests added yet (flagged as a known gap)

Bekhouche added 5 commits July 24, 2026 23:57
@Bekhouche
fix(rocm): bump rocm-stable/therock pins to a matching pair (b10108/7…
6c3b5a3 
….14)

The pinned llama.cpp build (b9752) expects ROCm 7.13, but the pinned
therock.version had drifted enough in practice for rocm_bin="latest"
users to hit a 404 building the download filename from a stale ROCm
version. Bump both pins together to the current verified-live pair.

Fixes lemonade-sdk#2800
@Bekhouche
fix(rocm): discover ROCm version from release assets when resolution …
6cea024 
…diverges from the pin

The rocm-stable filename embeds a ROCm/TheRock version that must match
what the resolved llama.cpp build was actually compiled against. That
held as long as installs used the statically pinned tag, but
rocm_bin="latest" (or an explicit custom tag) can resolve to a
different release than the pin describes — the static therock.version
then no longer applies, and the constructed URL 404s.

When resolution lands on a tag other than the pin, look up the real
ROCm version from that release's own GitHub asset names instead of
trusting the static pin, threaded through llamacpp's filename
construction via a per-thread override (mirrors the existing
rocm_arch_override pattern). Falls back to the static pin on any
lookup failure (offline, network error, no match), and the default
builtin path is unaffected — no new network call unless the resolved
tag actually diverges from the pin.

Also drops a dead duplicate rocm-stable branch in
LlamaCppServer::get_install_params (unreachable second `else if`).

Does not yet cover the TheRock runtime package version (still uses
the static pin) — AMD's TheRock tarball repo needs a full patch
version that a release asset filename never encodes. Left as a
follow-up.

Refs lemonade-sdk#2800
@Bekhouche
fix(rocm): address review feedback on ROCm version discovery
3f6506e 
- Extend the discovered ROCm version to the TheRock runtime package
  itself, not just the llama-server download filename — TheRock has
  never shipped a non-".0" patch release (verified against
  repo.amd.com and TheRock's own major.minor-only GitHub tags), so
  the discovered major.minor fully determines it too. Threaded
  through as an explicit InstallParams field rather than the
  thread-local override, since install_therock_if_needed() doesn't
  go through llamacpp's install_params_fn callback.
- Fix BackendUtils::get_therock_lib_path(), used at model launch to
  set the ROCm library search path: it also assumed the static pin,
  so a successful divergent-tag install would fail to find its own
  runtime at launch. Falls back to scanning what's actually on disk
  for the arch when the statically-expected path is missing.
- Always run discovery for llamacpp rocm-stable, not only when the
  resolved tag diverges from the pin, so an accidental future pin
  drift (bumping the llama.cpp tag without therock.version) self-heals
  too instead of silently reproducing the original bug.
- Throw a clear, actionable error when the resolved tag diverges from
  the pin and discovery genuinely fails online, instead of silently
  falling back to a version that may not match and would 404 anyway.
  Offline/no_fetch_executables installs are still excused gracefully.
- Scope discovery to recipe=="llamacpp" only (was matching any
  rocm-stable recipe, wasting a lookup on acestep/thinksound/trellis/
  openmoss, which don't consume it). sd-cpp has the same underlying
  bug but is deliberately NOT included: its releases can ship more
  than one ROCm-versioned asset per tag (observed both -rocm-7.1.1-
  and -rocm-7.13.0- in the same release), so naively picking the
  first regex match could silently install the wrong ROCm target.
  Left as a follow-up requiring arch-aware disambiguation.

Refs lemonade-sdk#2800
@Bekhouche
fix(rocm): resolve TheRock runtime path by actual resolved version, n…
7f9202a 
…ot disk-scan guessing

The previous fix's get_therock_lib_path() still preferred the static
pin whenever that directory happened to exist on disk, even if a
different (newer, correctly discovered) version was the one actually
relevant to this launch — e.g. a stale 7.14 install left over from an
earlier default install would still be picked over a just-installed
7.15 from rocm_bin="latest". Its fallback for the divergent case
guessed via most-recently-modified directory, which is neither
version-aware nor safe against incomplete installs.

Replace the guessing with an explicit expected_version parameter.
LlamaCppServer::load() re-reads discovered_therock_version from
get_install_params() right after installing (cache hit, no extra
network call) and passes it through. get_therock_lib_path() now also
requires version.txt to exist and match before trusting a directory —
closing a related gap where any caller could pick up an incomplete
install directory.

Also fix install_therock() leaving an empty, version.txt-less
directory behind when the download itself fails (only extraction
failure was cleaned up before) — such a directory would otherwise be
indistinguishable from a real, if incomplete, install.

Refs lemonade-sdk#2800
@Bekhouche
fix(rocm): persist resolved TheRock version so it survives a restart
687bfbe 
get_install_params()'s ROCm version discovery lives in an in-memory,
per-process BackendManager cache. That's fine within one process, but
breaks across a lemond restart: install_backend() has an early-return
for "offline and already installed" that skips get_install_params()
entirely, and even when it doesn't, a fresh process has empty caches
and may not be able to redo the discovery live (e.g. offline). The
previous fix's claim that re-calling get_install_params() in load()
was "always a cache hit" was true only within the same process
lifetime as the original install — not after a restart.

Persist the resolved version as a marker file
(<install_dir>/therock_version.txt) written once after a successful
llamacpp rocm-stable install. LlamaCppServer::load() now reads this
marker first — a plain file read, no network or cache involved — and
only falls back to a live re-resolution for backends installed by an
older lemond build, before this marker existed.

Verified live on both WSL/Linux and native Windows (VS 2026/MSVC):
installing with rocm_bin pinned to a tag needing ROCm 7.13 (static
pin: 7.14) writes therock_version.txt containing "7.13.0", matching
the discovered value, on both platforms.

Refs lemonade-sdk#2800
@github-actions github-actions Bot added engine::llamacpp llama.cpp backend (LlamaCppServer); GPU/CPU LLM inference (Vulkan, ROCm, Metal) runtime::rocm AMD ROCm runtime bug Something isn't working labels Jul 24, 2026
@Bekhouche
Bekhouche marked this pull request as ready for review July 29, 2026 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working engine::llamacpp llama.cpp backend (LlamaCppServer); GPU/CPU LLM inference (Vulkan, ROCm, Metal) runtime::rocm AMD ROCm runtime

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot install ROCm Llama.cpp GPU backend / Pulling wrong version from github

1 participant

@Bekhouche