Feat/vte rdna3 backend#2675
Conversation
VTE (https://github.com/kyuubyN/VTE) is a from-scratch AMD-native inference engine that dispatches HIP kernels directly (no llama.cpp/ROCm-PyTorch dependency), built and measured on Windows RDNA3 hardware. This wires it in as a new recipe following the existing WrappedServer/BackendDescriptor pattern, spawning vte-server (an OpenAI-compatible HTTP subprocess) exactly like every other backend. What's included: - BackendDescriptor + WrappedServer (VTE.h, VTE_server.h/.cpp): recipe="vte", experimental=true, selectable_backend=false (single "rocm" flavor), support row for gfx110X on Windows. - Three registered models, one per architecture VTE supports (Qwen2.5, Granite 4.1, Qwen3.5), each downloaded and loaded end-to-end on real hardware before being added here, not just wired up on paper. - A fix to identify_rocm_arch_from_name() (system_info.cpp): the RX 7600 -- the exact card VTE is validated on -- was missing from the RDNA3 name-matching list, so every ROCm backend (including the pre-existing llamacpp) showed as "unsupported" on this hardware before this change. - A CMakeLists.txt /MANIFESTINPUT quoting fix: unquoted, it broke the linker (LNK1181) on any build path containing a space. - capabilities.py entry mirroring ryzenai's shape, and one CI matrix row. - Docs regenerated via docs/tools/gen_backend_boilerplate.py (no hand edits to the GENERATED regions). Known limitation, not addressed here: RuntimeConfig::recipe_options() only translates config keys a descriptor declares in its own `options` array; `ctx_size` is a shared option opted into via `uses_ctx_size` instead, and falls back to reading only the global top-level config key, never a per-recipe one. A recipe-level ctx_size default (e.g. via config_extra) silently has no effect because of this -- confirmed by building and loading for real, not by reading the code. Worked around here at the model level (each entry's own `recipe_options.ctx_size`), but the recipe_options() gap itself is real and applies to any future backend that tries the same recipe-level default. temp-tests/ (19 tests) validates the config/registration surface described above; not a replacement for test/server_llm.py --wrapped-server vte --backend rocm against a real build, which was run manually but isn't CI-automatable from this environment.
There was a problem hiding this comment.
Thanks for the substantial work on this integration. The overall backend structure fits Lemonade’s registry and WrappedServer model well.
I want to note that this rather bigger change - bringing in a new backend - is a substantial changed that should be backed by and issue or a discord discussion with maintainers. Have a look at our contribution guidelines.
@ramkrishna2910 @Geramy what is your take on adding this VTE backend?
Now here is my code focused review, as I think the PR is not merge-ready yet.
The main blockers I found are:
-
Clean-install runtime setup is incomplete
The adapter downloads and starts vte-server, but it does not ensure that amdhip64.dll or a compatible HIP runtime is available. TheRock is currently installed only for ROCm-channel backends resolving to rocm-stable, while VTE declares a single rocm flavor without ROCm channels.
A local test on a machine with an existing HIP SDK can therefore pass while a normal Lemonade installation fails on startup.
Please either bundle the required runtime, integrate VTE with Lemonade’s TheRock installation and PATH setup, or expose a clear external HIP SDK setup requirement. This should be tested on a clean Windows machine without an existing HIP_PATH.
-
Non-streaming responses are missing OpenAI usage fields
VTE’s chat and completion responses do not contain usage.prompt_tokens, usage.completion_tokens, or usage.total_tokens, and the Lemonade adapter forwards the response unchanged.
The standard test/server_llm.py suite explicitly checks these fields for both chat completions and text completions, so the current implementation should fail those tests.
-
Request parameters are not normalized
Lemonade and modern OpenAI clients commonly send
max_completion_tokens, but VTE only readsmax_tokens. Lemonade already has JsonUtils::with_legacy_max_tokens_alias() for exactly this purpose.Similarly, Lemonade tests use
repeat_penalty, while VTE readsrepetition_penalty.The adapter should normalize both fields before forwarding the request.
-
The real integration test has not been run
The PR itself states that:
test/server_llm.py --wrapped-server vte --backend rocm
has not been executed. The temporary tests only inspect files, strings, and configuration and explicitly state that they do not replace a build or an end-to-end test.
The new CI row also uses the generic [Windows, rocm, lemon-prod] runner labels even though the descriptor only supports gfx110X. Please target a known RDNA3 runner and require the complete LLM test suite to pass.
Additional issues:
- multi_model is marked as supported, but VTE refuses to start when less than 80% of total VRAM is free. This likely prevents loading VTE after another GPU model. Either disable the capability for now or integrate the preflight behavior with Lemonade’s VRAM and eviction policy. Additionally this project would typically like to have gfx1151 and similar APU support as well and this contradicts proper APU behavior.
- The title and PR description claim RDNA2 support, but the Lemonade descriptor enables only gfx110X. Upstream VTE also describes RDNA2 as theoretical and not tested on real hardware. I suggest limiting this PR to experimental Windows RDNA3 support.
- The support description should include the RX 7600, since that is the only card validated on real hardware.
- The downloaded third-party executable should be pinned with a SHA-256 hash in backend_versions.json, rather than trusting a mutable release asset only by tag.
- VTE_server.cpp still says that the file has not been compiled, which contradicts the PR description.
- The
temp-tests/directory should be removed or converted into maintained tests before merge. - The unrelated
/MANIFESTINPUTfix is reasonable, but would be cleaner as a separate focused PR. - Overall too many obvious comments in the code see our agents guidelines
The architecture registration, architecture validation, process cleanup, and parent watchdog are good foundations. Once the runtime installation path and OpenAI compatibility issues are fixed and the real Lemonade test suite passes on gfx110X, this should be in much better shape for review.
|
Thank you for the detailed review! I'm actively working on addressing the blockers you raised. In the meantime, I will open a formal issue to discuss the motivation and design details of this integration, as suggested. |
Responds to the code review on this PR:
- TheRock integration: rocm_channels={"stable"} on the descriptor makes
install_backend() install TheRock (the HIP runtime) alongside vte-server,
and load() now puts TheRock's bin/ (amdhip64.dll) on the child's PATH and
HIP_PATH, mirroring llamacpp_server.cpp's Windows block. Without this, VTE
only started on a machine with a pre-installed HIP SDK.
- Request normalization: chat_completion()/completion() now normalize
max_completion_tokens->max_tokens (via the existing
JsonUtils::with_legacy_max_tokens_alias) and repeat_penalty->
repetition_penalty before forwarding, matching llamacpp/vllm's pattern.
- multi_model flipped to false in capabilities.py: vte-server's 80%-free-VRAM
preflight makes loading alongside another model unreliable; claiming
multi_model support contradicted that.
- Support row now names the RX 7600 specifically as the validated card,
rather than implying the whole RX 7700/7800/7900 range was tested.
- Removed the stale "not compiled" header comment (it now has been, see
below) and trimmed comment density across VTE.h/VTE_server.h/
VTE_server.cpp to match this repo's default-to-no-comments convention.
- Regenerated docs (gen_backend_boilerplate.py) for the support-row wording
change; --check is clean.
Verified by building lemond and LemonadeServer locally against this branch
(clean compile, zero warnings from these files) -- not by reading the code.
temp-tests/ (20/20) still green.
Not addressed here: SHA-256 pinning for the release asset (needs a cut
release to hash first) and the CI runner label (this repo's runner
taxonomy is by capability tag -- rocm/vulkan/xdna2/stx-halo -- not by exact
GPU model, so there's no more-specific "RDNA3-only" label to swap in
without maintainer input on what runner, if any, they'd dedicate to this).
get_therock_lib_path() returns the bin/ directory directly, but vte-server's
own DLL search (vte/bridge/dll_discovery.py) treats HIP_PATH as the SDK root
and appends "bin" itself -- passing the bin/ dir as-is made it look in a
nonexistent bin/bin/. It was silently working anyway via the PATH-based
fallback (which does search directories as-is), not through the intended
HIP_PATH lookup. Use the parent of the returned bin/ dir instead, matching
the real ROCm install layout (root/{bin,lib,include,...}) confirmed on this
machine's actual ROCm 7.1 install.
Two bugs found by actually running the load on a machine with the HIP SDK uninstalled (ROCm folder deleted, only the graphics driver's bundled amdhip64_7.dll in System32 remained) -- the clean-machine scenario the PR review specifically asked to see tested: - backend_versions.json pinned "vte" under the raw "rocm" key. normalize_backend_name() resolves the recipe's channel to "rocm-stable" before looking up the pinned version (same as llamacpp's own "rocm-stable"/"rocm-nightly" keys), so the lookup failed with "backend_versions.json is missing version for: vte:rocm-stable". Renamed the key to match. - VTEServer::get_install_params() checked backend != "rocm", but the framework calls this with the already-normalized name (BackendManager:: get_install_params resolves it before invoking the per-recipe fn) -- always "rocm-stable" now that rocm_channels is non-empty, never bare "rocm". install_backend()/get_backend_binary_path() don't need the same fix: both do their own internal normalization on the raw "rocm" this file still passes them. Verified end to end after both fixes: install_backend() downloaded and extracted TheRock's gfx110X-all package for real (~13GB, includes amdhip64_7.dll under bin/), vte-server started using it, and a real chat completion through Lemonade's HTTP API returned a correct answer -- with no HIP SDK installed anywhere on the system.
…claim VTEOps::populate_metadata() reads GGUF context_length/architecture into ModelInfo the same way LlamaCppOps does, via single_ops<VTEOps>() -- default_backend_ops() never parsed the file at all, so max_context_window stayed 0 (omitted from /v1/models) and the architecture allowlist check in load() was silently inert (model_info.gguf.architecture was never populated for it to compare against). Bumped to vte-server 0.3.0, which fixes a critical bug (AOT kernels silently skipped whenever hipcc was absent from PATH, producing all-zero logits) plus the usage-field/param-alias/security fixes already expected here. Pinned its SHA-256. Corrected capabilities.py: vte was the only backend in the whole catalog claiming generation_parameters (identical params -> identical output), which needs a fixed seed no backend here actually has -- every other entry already declares this False. Full test/server_llm.py --wrapped-server vte --backend rocm now passes end-to-end (was blocked on test_000 before this).
vte-server 0.3.1 adds the llama GGUF architecture (plus a rendered-prompt tokenization fix). Registers Llama-3.1-8B-Instruct-VTE, adds "llama" to the architecture allowlist, and pins the 0.3.1 asset by SHA-256. Regenerated the backend-models doc region for the new entry.
# Conflicts: # docs/guide/configuration/custom-models.md
|
Opened #2679 for the design and motivation discussion, that thread is the right On the code-level blockers, I've pushed fixes for the ones that didn't depend on 1. Clean-install runtime (TheRock). VTE now declares 2. Usage fields. vte-server now returns 3. Param normalization. The adapter normalizes 4. Real integration test. multi_model: set to RDNA2 claim: removed. The descriptor enables only gfx110X, the support row RX 7600 in support description: done. SHA-256 pin: the downloaded asset is now pinned by sha256 in Stale "not compiled" comment: removed. /MANIFESTINPUT fix: split into its own focused PR (#), Comment density: pruned toward AGENTS.md across the C++ files. Happy to take temp-tests/: I'll drop these in the final pre-merge pass, kept for now as a Still open on my side, and better scoped separately: APU (gfx1151) support, which I'll hold further iteration pending direction on #2679. |
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the substantial follow-up. Several original blockers are now addressed, especially the TheRock setup, request aliases, usage fields, and the reduced capability claims. However, the PR is still not merge-ready:
-
backend_versions.jsonnow contains two top-levelchecksumsobjects. The second one can overwrite the existing checksum registry. Please merge the VTE checksum into the existingchecksums.github.<repo>.<tag>.<asset>structure. -
Setting
multi_model: falseintest/utils/capabilities.pyonly skips the test; it does not change runtime behavior. VTE still usesSlotPolicy::Standard, so Lemonade may keep other GPU models loaded, hit VTE’s 80%-free-VRAM preflight, then trigger the router’s global eviction-and-retry path. This needs a real runtime policy or the VTE preflight must be disabled when hosted by Lemonade. -
vte-serverstill enables its own 60-second idle auto-unload. Activity is touched only when generation starts, so a long request can race with model unload. Lemonade should own model lifetime; please disable VTE’s internal auto-unload in this integration. -
The temporary tests are already stale: they still expect VTE 0.3.0 while the current pin is 0.3.1. They also explicitly do not replace the real build/E2E suite. Please remove them or convert the useful checks into maintained tests.
-
There is still no green CI run for the current head, and the generic Windows ROCm runner does not guarantee gfx110X hardware. Please provide a successful full Windows build and
test/server_llm.py --wrapped-server vte --backend rocmrun on a known compatible GPU. -
The PR still contains the unrelated
/MANIFESTINPUT, RX 7600 detection, and frontend DownloadManager changes despite stating they were split out. Please keep this backend PR focused.
There is also documentation drift: the generated docs say “3 models” but list four, while the PR description still mentions RDNA2 and an unexecuted integration test.
So I am keeping Request changes. The integration is moving in the right direction, but the checksum regression, runtime lifecycle, and VRAM/eviction behavior must be fixed before merge.
- Merge the VTE checksum into the existing checksums.github.<repo>.<tag> structure instead of a second top-level "checksums" object that overwrote the first. - Bump the pin to vte-server 0.3.2 and have the adapter spawn it with --idle-timeout 0 (disables its internal idle auto-unload, which could race a long request) and --vram-limit-pct 0 (disables its 80%-free-VRAM preflight, which collided with the router's eviction-and-retry). Lemonade now owns model lifetime and VRAM policy; vte-server's per-allocation OOM guard still protects a real OOM load. - Remove temp-tests/ (stale and never a substitute for the real suite). - Drop the unrelated /MANIFESTINPUT, RX 7600 detection, and frontend DownloadManager changes from this PR; they belong in their own PR. - Fix the generated backend-models doc count (3 -> 4 models). Verified end to end on a real RX 7600: Lemonade installs 0.3.2 (asset SHA-256 verified), spawns vte-server with the two flags, loads a VTE model, and returns a correct chat completion with usage fields.
|
Thanks, all six plus the doc drift are addressed. Pushed the fixes and cut 1. Duplicate checksums object. Merged the VTE checksum into the existing 2. multi_model runtime behavior. The adapter now spawns vte-server with 3. Internal auto-unload. vte-server 0.3.2 treats 4. temp-tests. Removed. 5. CI runner / gfx110X. I went through the workflows to find a gfx110X label. The only arch-specific self-hosted runner in the repo is 6. Unrelated changes. Dropped the Doc drift (the "3 models" count) is fixed, and the PR description no longer mentions RDNA2 or an unexecuted test. End to end on a real RX 7600: Lemonade installs 0.3.2 with the asset SHA-256 verified, spawns vte-server with both flags, loads a VTE model, and returns a chat completion with correct usage fields. |
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the additional cleanup. The checksum registration is now correctly integrated into the existing registry, and disabling VTE’s own idle unload and VRAM preflight when hosted by Lemonade resolves two important lifecycle conflicts.
However, I still need to keep Request changes because the current head has the following blockers:
-
recipeNames.ts is deleted, but it is still widely imported
This PR deletes
src/app/src/renderer/utils/recipeNames.tsentirely. Unchanged files such asApp.tsx,ModelManager.tsx,BackendManager.tsx,systemData.ts, and several collection utilities still import exports from it.This should break the frontend TypeScript/Webpack build with unresolved-module errors. Please restore the file. Its deletion also appears unrelated to the VTE integration.
-
The system ROCm runtime path is still not forwarded to VTE
Lemonade may skip installing TheRock when
resolve_rocm_root()finds a compatible system runtime throughROCM_PATHorrocm-sdk. The VTE adapter then only callsget_therock_lib_path()and does not use that resolved system ROCm root.Therefore, a valid runtime selected only through
ROCM_PATHcan be accepted by Lemonade but remain invisible tovte-server, because VTE searchesHIP_PATHandPATH, notROCM_PATH.Please resolve the effective ROCm root once and inject its
bindirectory into the childPATHand its root intoHIP_PATH, regardless of whether the runtime came from TheRock or a system installation.
3 Capability documentation is inconsistent with the implementation
test/utils/capabilities.py still explains multi_model: false by saying that VTE refuses to load below 80% free VRAM. The adapter now explicitly passes --vram-limit-pct 0, so that explanation is no longer true.
Keeping multi_model disabled conservatively is fine, but the comment should describe the actual remaining limitation and not behavior that Lemonade now disables.
… capabilities doc
|
Thanks for the detailed review! I've addressed all the feedback and pushed the updates: Restored recipeNames.ts: The file has been restored to its original state, resolving the import errors and fixing the TypeScript/Webpack frontend compilation. |
fl0rianr
left a comment
There was a problem hiding this comment.
I reviewed the current PR head and the now-executed CI run.
The overall integration has improved substantially: the backend artifact is version- and checksum-pinned, TheRock installation is wired through the ROCm channel mechanism, request aliases and usage fields are supported, VTE’s internal idle unload and initial VRAM gate are disabled under Lemonade, and the unrelated RX 7600/manifest fixes have now been merged separately in #2678.
I am still requesting changes before merge for the following reasons.
1. The current VTE CI failure does not appear to be a missing Hugging Face model
The Windows VTE job currently fails before attempting any model download or starting vte-server.
The server log shows:
- the model cache is built;
- the test requests
GET /api/v1/models/Qwen2.5-1.5B-Instruct-VTE; - Lemonade immediately returns
404.
This is the first static_max_context_window test. It queries the registered model before calling /pull, so the current failure means the VTE model was filtered out of Lemonade’s runtime model catalogue. It is not yet evidence that the checkpoint filename or repository cannot be found.
The failing workflow was created from a base that predates the merge of #2678, and this PR branch is still behind main. Without the RX 7600 detection change, the gfx110X VTE backend is considered unavailable and all models using that recipe are removed by filter_models_by_backend().
Please rebase or merge the current main into this branch and run the workflow again. If the model still returns 404 afterwards, please include the runner’s /api/v1/system-info output and GET /api/v1/models?show_all=true result so the remaining backend-gating issue is visible.
All other workflows are currently green; only the Windows VTE inference job is failing.
2. The effective ROCm runtime selection can still choose an incompatible system installation
BackendManager correctly checks whether a detected system ROCm runtime matches the pinned TheRock version. When it does not match, Lemonade installs the compatible TheRock runtime.
However, VTEServer::load() independently calls resolve_rocm_root() again and always prefers any detected system runtime. It falls back to TheRock only when no system root exists.
That produces this path:
- An incompatible system ROCm installation is detected.
- Lemonade correctly installs the pinned TheRock runtime.
- VTE nevertheless puts the incompatible system runtime into
HIP_PATHandPATH. - The freshly installed compatible TheRock runtime is unused.
Please make the runtime selected for the child process consistent with the decision made during installation. A mismatching system runtime must not override the pinned TheRock runtime.
3. multi_model: false is still test metadata rather than an enforced runtime restriction
The capability catalogue now states that multiple concurrent VTE models may cause allocator conflicts or OOM. However, this only skips Lemonade’s multi-model test.
The VTE descriptor still uses SlotPolicy::Standard, which explicitly provides no device exclusivity. Lemonade can therefore load VTE alongside another GPU model, or load multiple VTE models when max_loaded_models permits it. VTE’s own free-VRAM preflight has also been disabled.
If this coexistence is considered unsafe, it needs to be enforced by the actual router/backend lifecycle rather than only documented in test/utils/capabilities.py. Otherwise, the capability should be enabled and coexistence must be validated.
4. GPU selection is unsafe on mixed AMD iGPU/dGPU systems
Upstream VTE currently hardcodes hipSetDevice(0) and does not expose a device-selection argument.
Lemonade can detect a supported RX 7000 dGPU and expose the VTE backend as available, while vte-server may initialize a different AMD device if the system also contains an iGPU. Runtime installation selection through HIP_PATH does not select the physical GPU.
Please either:
- provide and forward explicit GPU selection to VTE; or
- restrict and clearly document this initial integration as supporting only systems with one visible AMD GPU.
Relying on unspecified HIP enumeration order can result in the wrong device or incompatible kernels being selected.
5. The successful CI run still needs to reach the actual backend
After rebasing onto the merged RX 7600 fix, the Windows VTE job should demonstrate at least:
- the VTE model remains present in the runtime catalogue;
- the registered GGUF downloads successfully;
- the VTE backend and matching ROCm runtime install successfully;
- the model loads;
- non-streaming chat and completion requests pass;
- streaming chat passes;
- usage fields and parameter aliases pass;
- unload and process cleanup complete successfully.
The present run stops at the model-catalogue check, so it does not yet validate the backend installation, TheRock environment, model loading, or inference paths.
Please include the runner GPU and detected ROCm family in the successful run summary. The generic [Windows, rocm, lemon-prod] label alone does not document which hardware executed the test.
6. The PR and RFC descriptions need to be updated
The PR description still contains several obsolete statements:
- it claims RDNA2 support;
- it says three models are registered although there are four;
- it says the full integration test has not been run;
- it refers to temporary tests that are no longer in the diff;
- it describes the RX 7600 and
/MANIFESTINPUTchanges as part of this PR, although they were moved to and merged through #2678.
Please update the description to match the current implementation and CI status.
RFC #2679 also still contains placeholder references such as #[número] and @[mantenedor]. More importantly, it has not yet received maintainer direction on whether this narrow, single-maintainer backend should become part of Lemonade. That project-level decision should be resolved independently of the remaining code fixes.
Smaller compatibility issues
These are not the primary merge blockers, but should be fixed or recorded as explicit follow-ups:
- responses always report
finish_reason: "stop", including token-limit termination where OpenAI-compatible behavior should report"length"; - streaming emits a usage-only chunk even when
stream_options.include_usagewas not requested; - the descriptor enables the full
gfx110Xfamily although onlygfx1102has been executed on real hardware. The documentation should continue to distinguish validated support from cross-compiled, untested targets.
The PR is moving in the right direction, and the successful build, registry, routing, docs, and packaging checks are good progress. The next useful step is to rebase onto current main and rerun the VTE job. Approval still depends on that test reaching real inference and on resolving the ROCm-runtime selection and runtime coexistence issues above.
The earlier restore of this file (needed because it's a shared upstream utility several core files import, not VTE-specific) diverged from upstream's version. No functional difference, just drop the extra comments so this PR carries zero frontend diff.
VTEServer::load() was checking resolve_rocm_root() (an existence-only probe, no version comparison) before get_therock_lib_path(), so any present-but-incompatible system ROCm install would win over a correctly-installed TheRock runtime, silently ignoring the compatibility decision install_backend()/BackendManager already made. Swapped the order: get_therock_lib_path() first (non-empty only when BackendManager actually decided TheRock was needed and installed it), falling back to the system root only once that's confirmed empty -- which is exactly when trusting the system install is safe. Mirrors how llamacpp_server.cpp already handles this (never calls resolve_rocm_root() at all).
multi_model: false was documented in test/utils/capabilities.py but never enforced: VTE used SlotPolicy::Standard (no device exclusivity), so the router could load it alongside another GPU model or a second VTE instance once its own free-VRAM preflight was disabled under Lemonade. New SlotPolicy::ExclusiveGpu mirrors ExclusiveNpu (evict every other GPU server before loading), but also needs the reverse direction NPU never did: every NPU backend is already ExclusiveNpu/CoexistByType, so a Standard NPU load that could violate the invariant doesn't exist. Most GPU backends (llamacpp, vllm, ...) ARE Standard, so a later Standard GPU load now explicitly evicts a live ExclusiveGpu holder first (find_exclusive_gpu_server(), checked before the slot-policy switch). VTE's descriptor moves from Standard to ExclusiveGpu, closing the gap between documented and enforced behavior the second review round flagged.
Real finish_reason and stream_options.include_usage gating. Verified end to end: Lemonade installs 0.3.3 with the asset SHA-256 verified, loads a VTE model, and a max-tokens-truncated response now reports finish_reason: "length" through the full Lemonade->vte-server path.
A generic runner label like [Windows, rocm, lemon-prod] doesn't say which physical card actually executed a CI run. Every server_llm.py- based test now logs the detected amd_gpu/nvidia_gpu/amd_npu name and family right after confirming the server is reachable, querying the same /api/v1/system-info the router itself uses for backend gating. Shared base class, so this applies to every backend's test run, not just VTE. Best-effort (wrapped in try/except): a logging failure never fails the actual test run. Verified live: "Detected amd_gpu: AMD Radeon RX 7600 (family: gfx110X)" now appears in server_llm.py --wrapped-server vte --backend rocm output, suite still OK (skipped=20).
The review used gfx1102/gfx110X terminology directly; match it instead of relying on the reader to know RX 7600 maps to gfx1102.
Replaces the "requires a single visible AMD GPU" limitation note: as of vte-server 0.3.4, VTE enumerates every visible HIP device and picks the first one matching a discrete architecture it actually supports, so an integrated GPU is never selected on a mixed iGPU+dGPU system. Notes the narrower residual gap honestly: two supported discrete GPUs on the same system still resolve to "the first one found," with no Lemonade-side selection wired through yet.
|
Thanks for the re-review. All six points plus the smaller issues are fixed. Two notes before the list: CI shows action_required on my latest pushes (fork gate), so it hasn't re-run yet. I validated everything locally on real RX 7600 hardware in the meantime (details in point 5).
Also added GPU/family detection logging to the shared test harness, so any backend's CI log now self-documents its hardware, e.g. Detected amd_gpu: AMD Radeon RX 7600 (family: gfx110X).
Smaller issues Let me know if you want the CI run kicked off, or if anything needs another pass first. |
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the substantial follow-up. Good work you are doing!
Several earlier blockers are now genuinely resolved:
- #2678 is merged and this branch is fully rebased onto the current
main; - the PR description accurately reflects the four models and the RDNA3-only scope;
- the ROCm/TheRock priority inversion has been corrected;
- VTE is pinned to 0.3.5 with the matching SHA-256;
- upstream now reports accurate
finish_reasonvalues and gates the usage-only streaming chunk throughstream_options.include_usage; - mixed AMD iGPU/dGPU selection is substantially improved upstream;
- model coexistence is now intended to be enforced at runtime rather than only skipped in the test catalogue.
However, I am keeping Request changes because the new GPU-exclusivity implementation is not correct yet.
1. ExclusiveGpu uses the descriptor’s default device, not the selected backend’s effective device
The router currently determines the incoming device with:
DeviceType device_type = model_info.device;and stores that same value in the loaded server metadata. Both directions of the new exclusivity logic then depend on this value through device_type or server->get_device_type().
model_info.device, however, is populated from the recipe descriptor’s static default_device; it does not represent the backend variant selected in effective_options.
This produces concrete incorrect behavior:
-
llamacppdeclaresDEVICE_GPUas its default even when the selected backend iscpuorsystem. Loading a CPU-only llama.cpp model will therefore evict VTE unnecessarily. -
whispercppdeclaresDEVICE_CPUas its default even when the selected backend isrocmorvulkan. A Whisper ROCm model can therefore be loaded while VTE remains resident:- loading Whisper ROCm after VTE does not enter the reverse GPU-eviction check;
- loading VTE after Whisper ROCm does not see Whisper in
evict_all_gpu_servers().
The latter defeats the safety property ExclusiveGpu was added to provide.
The codebase already exposes WrappedServer::effective_device(options) and effective_slot_policy(options) specifically for recipes whose device or policy depends on the selected backend. Please resolve the effective device and policy before the exclusivity and LRU decisions, store the effective device in the server metadata, and use the effective values when inspecting loaded servers.
This will also require ensuring that variant-dependent backends actually implement the relevant effective-device mapping. The current descriptor defaults alone are insufficient.
2. The policy is global to every GPU, not the conflicting AMD device
DeviceType distinguishes only CPU, GPU, and NPU. Consequently, evict_all_gpu_servers() evicts every loaded GPU backend regardless of vendor or physical device.
For example, loading VTE on an AMD RX 7600 will evict an unrelated CUDA model running on a separate NVIDIA GPU, even though those models do not share VRAM or a runtime allocator. Loading that CUDA model afterwards will evict VTE in return.
That is broader than the stated reason for this policy and regresses legitimate multi-GPU coexistence. Please either:
- make exclusivity aware of the accelerator provider or physical device; or
- obtain explicit maintainer agreement that
ExclusiveGpudeliberately means global exclusivity across all GPUs and clearly document that limitation.
At minimum, this should not silently contradict Lemonade’s current multi-model documentation, which states that CPU/GPU models have no inherent coexistence restriction beyond available memory.
3. The new core router policy has no test coverage
SlotPolicy::ExclusiveGpu modifies generic router behavior, but the PR does not add router tests for it. Setting VTE’s multi_model capability to false means the existing multi-model integration test is skipped, so it does not validate this implementation.
Please add focused coverage for at least:
- loading VTE after a real GPU-backed model evicts the existing GPU model;
- loading a real GPU-backed model after VTE evicts VTE;
- loading a CPU variant does not evict VTE;
- a GPU variant whose descriptor default is CPU is still detected correctly;
- the chosen behavior for a model on a separate GPU/vendor.
These tests should exercise the resolved backend option, not only descriptor constants. Otherwise, the specific bug described above can easily pass the test suite.
4. VTE’s device selection can still violate Lemonade’s RDNA3-only contract
Upstream 0.3.5 now avoids accidentally selecting an integrated GPU by choosing the first device whose commercial name appears in GPU_ARCH_MAP. That is a meaningful improvement.
However, that map contains both RDNA3 and RDNA2 cards. Lemonade exposes VTE only when a gfx110X device is present, but on a machine containing both an RX 6000 and RX 7000 card, VTE may select the RX 6000 if HIP enumerates it first.
That silently runs the Lemonade integration on hardware this PR deliberately excludes as unvalidated.
Please either:
- pass
VTE_DEVICE_INDEXfor the specific RDNA3 device selected by Lemonade; - change VTE’s automatic selection to prefer gfx110X for this integration; or
- reject ambiguous mixed-RDNA-generation configurations with a clear error.
6. Documentation and project-level approval remain incomplete
Because this PR adds a new core SlotPolicy, the backend-development documentation should list ExclusiveGpu, and the multi-model documentation should explain its behavior. It currently still says that CPU/GPU models have no inherent coexistence limits.
RFC #2679 also still has no core-maintainer (@ramkrishna2910 ?) response to the central question of whether this narrow, single-maintainer backend should become a supported Lemonade integration. Its text additionally contains stale statements about mixed iGPU/dGPU support and says the engineering work is essentially complete, although the effective-device issue above remains unresolved.
The RFC should be updated, but more importantly, the project-level decision should be made before merge.
The VTE adapter itself is now close, and most of the original integration concerns have been handled well. The remaining blocker is primarily the correctness and scope of the new generic GPU-exclusivity mechanism.
|
Thanks for the constructive feedback! I am currently working on resolving the technical issues you highlighted, specifically: Refining the GPU exclusivity logic to scope it exclusively to AMD/ROCm devices, ensuring other GPU vendors (like CUDA/Nvidia) are unaffected. Repository Migration to lemonade-sdk: Move the VTE repository into the official Lemonade GitHub organization. This institutionalizes VTE as part of the broader Lemonade ecosystem, mitigating risks associated with single-author personal repositories, while securing shared governance and boosting community collaboration. |
…riptor defaults Wires up the previously-dead WrappedServer::effective_device()/ effective_slot_policy() hooks into the eviction path, resolved after the backend variant is picked and before eviction runs, instead of the recipe's static default_device. Fixes two concrete cases: a CPU-only llamacpp load no longer evicts VTE (its descriptor defaults to GPU), and a Whisper ROCm load is now correctly evicted by/evicts VTE (its descriptor defaults to CPU). Adds a new effective_is_amd_gpu() hook so ExclusiveGpu eviction is scoped to AMD/ROCm devices specifically, not every GPU regardless of vendor -- VTE no longer evicts or gets evicted by an unrelated CUDA model on a separate GPU. The eviction decision itself is extracted into a pure, dependency-free gpu_exclusivity_policy module with direct unit test coverage (no existing router/eviction test infrastructure existed to extend), plus a second test target exercising the real llamacpp/whisper backend-variant resolution the decision logic depends on. Also adds VTE_TARGET_ARCH_FAMILY, passed to vte-server unconditionally so its own device auto-selection can't pick an RDNA2 card on a system that also has an RDNA3 one -- this integration only validates gfx110X.
|
Changes are in, please take a look and let me know if anything still needs work.
Router::load_model() now constructs the backend early and calls effective_device()/effective_slot_policy() on it (previously declared but never actually called anywhere). LlamaCppServer/WhisperServer now implement effective_device() for real, which also fixed two latent bugs: Whisper's "rocm" backend was mapping to DEVICE_CPU, and llamacpp's "system" backend was mapping to DEVICE_GPU. Verified live: llamacpp_backend=cpu no longer evicts VTE; whispercpp_backend=rocm now correctly evicts VTE and is evicted by it, both directions.
New WrappedServer::effective_is_amd_gpu(options) hook, implemented per-backend (true only for the resolved ROCm variant). Eviction now only fires between two AMD-GPU-backed servers. Conservative call: vulkan doesn't count as AMD even on AMD hardware, since neither backend detects the underlying device vendor today. Open to scoping it in if you'd rather.
No router/eviction test infra existed, so the eviction decision is now a pure function (gpu_exclusivity_policy.h/.cpp), tested directly (test_gpu_exclusivity_policy, 7 cases covering your five scenarios, fed already-resolved values so descriptor constants can't fake a pass). A second target (test_backend_effective_device, 15 cases) links the real LlamaCppServer/WhisperServer and asserts their resolution end-to-end, so the actual bug you described can't slip past the decision tests alone.
Lemonade now passes VTE_TARGET_ARCH_FAMILY=gfx110X unconditionally when spawning vte-server, read from VTE's own descriptor row (single source of truth). vte-server honoring it is a separate change I'm making in that repo next.
ExclusiveGpu documented in docs/dev/adding-a-backend.md, multi-model.md no longer claims unlimited CPU/GPU coexistence. Also caught and fixed a stale VTE row in README.md's generated support matrix. RFC #2679 updated to match current reality, plus a concrete governance proposal (repo migration to lemonade-sdk, experimental/community-maintained status, CI smoke tests). Full server_llm.py --wrapped-server vte --backend rocm suite re-run after all of this: OK (skipped=20), no regressions. |
There was a problem hiding this comment.
Thanks for the update and hard work, one more round please, since zhere are two remaining blockers.
1. ExclusiveGpu does not cover sd-cpp:rocm
The router resolves GPU exclusivity before calling load(). SDServer, however, still relies on changing device_type_ inside load() and does not override either:
effective_device()effective_is_amd_gpu()
As a result, the following sequences do not enforce VTE’s claimed AMD-GPU exclusivity:
- load
sd-cpp:rocm, then load VTE; - load VTE, then load
sd-cpp:rocm.
In both cases, the router can classify the sd-cpp load as CPU/non-AMD during the exclusivity decision, allowing both processes to occupy the same AMD GPU.
Please either implement the effective-device/provider hooks for every AMD-capable backend or derive this centrally from the resolved backend. Add a router-level test using sd-cpp:rocm in both load orders.
2. VTE_TARGET_ARCH_FAMILY is not implemented by VTE 0.3.5
The adapter exports:
VTE_TARGET_ARCH_FAMILY=gfx110X
but the pinned VTE 0.3.5 release does not read this variable. Its device selector only checks VTE_DEVICE_INDEX and otherwise chooses the first recognized discrete RDNA2 or RDNA3 GPU.
Therefore, on a mixed RDNA2/RDNA3 system, Lemonade may expose VTE because an RDNA3 GPU exists while VTE starts on the RDNA2 GPU instead.
Please pin a VTE release that actually validates VTE_TARGET_ARCH_FAMILY, or perform the device selection/validation in Lemonade itself.
The branch is also currently one commit behind main, and the workflows for this head are still awaiting approval. After fixing the two points above, please rebase and I'll run the complete CI suite.
…with no override
sd-cpp was missing from ExclusiveGpu's exclusivity check: WrappedServer's base
effective_device()/effective_is_amd_gpu() default to the static descriptor,
and only LlamaCppServer/WhisperServer had per-backend overrides. Same latent
gap exists for acestep, openmoss, thinksound, trellis, and vllm, all of which
declare an amd_gpu-only ROCm support row.
Instead of writing a fifth (and future sixth, seventh...) hand-rolled
override, the base class now derives both from the descriptor's own support
rows when the resolved backend matches a single-device-family row exactly
(ROCm/CUDA/NPU/CPU) -- ambiguous cross-vendor rows like Vulkan (which lists
cpu+amd_gpu+nvidia_gpu together, since it can target any of them) fall
through to the static default rather than guessing. This covers all 6
currently-missing backends with no per-backend code; llamacpp/whisper keep
their explicit overrides since their resolved backend needs its own channel
mapping before it lines up with a support-row key.
Verified live: loading VTE then sd-cpp:rocm correctly evicts VTE via the
router log ("sd-cpp conflicts with AMD-GPU-exclusive peer ... evicting").
|
Both fixed, please take a look. 1. sd-cpp:rocm not covered Not just sd-cpp: acestep, openmoss, thinksound, trellis, and vllm had the same gap (each declares an amd_gpu-only ROCm row with no override). Went with your "derive centrally" option: WrappedServer's base class now derives effective_device()/effective_is_amd_gpu() from the descriptor's own support rows when the resolved backend matches a single-device-family row exactly. Cross-vendor rows like Vulkan (cpu+amd_gpu+nvidia_gpu together) can't be resolved to one answer from the descriptor alone, so they fall through to the static default instead of guessing. Covers all 6 missing backends with zero per-backend code; llamacpp/whisper keep their overrides since their resolved backend needs channel mapping first. New test_backend_effective_device cases cover sd-cpp specifically. On the router test in both load orders: confirmed one direction live (sd-cpp:rocm loaded, then VTE evicts it, per the router log). Couldn't get the reverse direction running, not a code issue: the pinned sd-server ROCm build fails to start on my machine (STATUS_DLL_NOT_FOUND, confirmed via native process creation and a clean reinstall; the CPU variant of the same binary starts fine), looks like a real gap in that sd.cpp release, unrelated to this PR. No router-level test infra exists in this repo for either direction, but the decision logic is backend-agnostic and already covered symmetrically by test_gpu_exclusivity_policy, exercised live through llamacpp/whisper both ways and sd-cpp one way. 2. VTE_TARGET_ARCH_FAMILY not implemented problem Implemented in vte-server 0.3.6 (published): filters candidates to the requested family, raises a clear error if nothing matches instead of falling back silently. Lemonade's pin updated to 0.3.6, verified with a clean reinstall. Branch rebased onto current main (0 commits behind). Please take a close look and let me know if you see anything else that needs work. |
… PowerShell polling)
Adds VTE, a from-scratch AMD-native inference backend that dispatches HIP kernels directly instead of going through llama.cpp or ROCm-PyTorch, as a new recipe for RDNA3 GPUs on Windows.
(https://github.com/kyuubyN/VTE)
Key Changes
vteas a new recipe (BackendDescriptor/WrappedServerinVTE.h,VTE_server.h,VTE_server.cpp), spawningvte-server, an OpenAI-compatible HTTP subprocess, the same way every other backend runs.experimental=true, singlerocmflavor (selectable_backend=false), support row forgfx110Xon Windows, validated on real hardware forgfx1102(RX 7600) specifically.server_models.json. Each checkpoint was downloaded and loaded end to end on real RX 7600 hardware before being registered here.rocm_channels = {"stable"}on the descriptor triggers Lemonade's TheRock install path, and the adapter injects TheRock'sbin/into the child processPATH/HIP_PATH, mirroringllamacpp-rocm. Verified on a machine with no pre-installed HIP SDK.usagefields (prompt_tokens/completion_tokens/total_tokens) on both chat and text completions, and normalization ofmax_completion_tokens/repeat_penaltyaliases before forwarding tovte-server(which also accepts both natively).vte-serverwith--idle-timeout 0and--vram-limit-pct 0, so Lemonade is the sole owner of model lifetime and VRAM policy rather than vte-server making its own unload/preflight decisions.vte-serverrelease asset is pinned by SHA-256 inbackend_versions.json, verified after download, instead of trusting a mutable tag.docs/tools/gen_backend_boilerplate.pysoREADME.md,docs/dev/backends-reference.md,docs/guide/cli.md, and the configuration docs reflect the new recipe. No hand edits to theGENERATEDregions.Known limitations
RuntimeConfig::recipe_options()only translates config keys a descriptor declares in its ownoptionsarray.ctx_sizeis a shared option opted into viauses_ctx_sizeinstead, and its handling only reads the global top-level config key, never a per-recipe one. A recipe-levelctx_sizedefault (viaconfig_extraon the descriptor) silently has no effect because of this. Confirmed by building and loading for real, not by reading the code. Worked around at the model level instead (each entry's ownrecipe_options.ctx_size, present on all four models above). Therecipe_options()gap itself is real and would hit any future backend trying the same recipe-level default.gfx1100/gfx1101(RDNA3 siblings), cross-compiled offline and confirmed to build cleanly but not run on that hardware. See VTE's owndocs/LIMITATIONS.mdfor the full breakdown.Testing
test/server_llm.py --wrapped-server vte --backend rocmsuite run manually against real RX 7600 hardware: chat/completions (streaming and non-streaming), usage fields, and parameter aliases all pass.lemond/LemonadeServerlocally with the new recipe and exercised the whole path end to end: TheRock install, model load, non-streaming and streaming/v1/chat/completions,/v1/models.main(which now includes the RX 7600 detection fix from fix: quote /MANIFESTINPUT path and detect RX 7600 as gfx110X #2678).