Fix: make placement accounting backend neutral - #903
Merged
JustVugg merged 1 commit intoAug 10, 2026
Conversation
Kenneth-Javier
added a commit
to Kenneth-Javier/colibri
that referenced
this pull request
Aug 12, 2026
A discovered device and a placement budget are not the same fact, and the
planner had no way to say so: `free_bytes` was mandatory and numeric, and
every consumer treated "a GPU is present" as "a GPU may be planned against".
That is fine while free VRAM *is* the budget, which is true of a discrete
card. It stops being true on a part where the GPU and the host share one
physical pool and the runtime's free figure describes pages the RAM tier is
already counting.
So `free_bytes` may now be None, meaning "not qualified as a budget on this
platform" — deliberately NOT the same claim as 0, which is a measurement
("the card is full") and keeps every behaviour it has. plans_placement()
tests `is not None` rather than truthiness so the two cannot collapse.
Only qualified devices reach the decisions: the VRAM budget, the shared-pool
`unified` flag, `_auto_tune`, the bottleneck classification, the
"VRAM already in use" warning (which divides free by total, and would
otherwise tell the user to stop an engine that is not running), and the
COLI_GPU/COLI_GPUS device list — that one is an instruction, not an
inventory. Everything discovered is still reported, marked `(identity only)`
in `coli plan` with a warning naming it, so the hardware does not silently
vanish from the output.
Also makes the no-device line backend-neutral. "no NVIDIA device detected" on
an AMD host was the same vendor assumption JustVugg#903 removed from doctor; this is
the line it missed.
No numeric-`free_bytes` behaviour changes: a plan built from a discrete card
produces the same tier, the same tuning and the same environment as before.
Kenneth-Javier
added a commit
to Kenneth-Javier/colibri
that referenced
this pull request
Aug 12, 2026
`rocm-smi` is a Linux tool. Neither the Windows HIP SDK installer nor a source build ships it, so `_discover_amd_gpus` found nothing on Windows and every AMD host there was planned as if it had no GPU: no device in `coli plan`, "no supported GPU detected", and `--gpu N` failing on hardware that works. `hipInfo.exe` is what both shipped SDKs do provide, and it sits in the same directory as `amdhip64_7.dll`. It comes from the HIP environment already required to build the backend, so this adds no dependency. Lookup stops at the first hit: COLI_HIP_RUNTIME_DIR, then %HIP_PATH%\bin, then PATH. COLI_HIP_RUNTIME_DIR leads because it names the runtime the loader will actually bind, and hipInfo lives beside it — on a host with two HIP installs, which is common, a stale machine-wide HIP_PATH must not describe the hardware through a runtime the engine will not load. Nothing is hardcoded. A block missing a name or a memory total produces no device rather than one completed with zeros; zeros would read as measurements. Missing, failing or unparsable hipInfo yields no device at all. Devices are recorded identity-only: `isIntegrated: 1` maps onto the `unified_memory` flag JustVugg#903 introduced, `gcnArchName` is kept (Windows needs an explicit HIP_ARCH, so the arch is worth showing), and `free_bytes` is None. That last part is the deliberate one. hipInfo DOES report free memory. On the validated gfx1151 host it reported 89.24 GB "100% free" while Windows had 59.3 GiB physically available — the same pages counted twice, ~30 GB apart, on a part where the GPU and host share one pool. Spending that as a VRAM budget would authorise an expert tier the machine cannot back, while the RAM tier is sized from the same memory. The value is parsed and discarded rather than stored, and docs/windows.md says so and shows the numbers. This changes only what Colibri turns on by itself. Every environment variable keeps working as documented. Qualifying a safe automatic budget on shared memory needs measurement on real hardware, and is left to a later change instead of guessed at here. The Linux rocm-smi path is untouched, and a test pins that.
Kenneth-Javier
added a commit
to Kenneth-Javier/colibri
that referenced
this pull request
Aug 12, 2026
JustVugg#903 made the accelerator check backend-neutral in wording. The probe under it was still CUDA-specific: on Windows it required coli_cuda.dll beside the executable, whatever the host was actually built to load. A HIP_DLL host loads coli_hip.dll. So doctor reported "GPU runtime library is missing" on a working HIP install -- and that branch is a fail, which makes the whole report status `error`, not a warning. Verified against the engine built for the JustVugg#788 validation: it carries coli_hip.dll beside it, and the old rule called it missing. Accepting either name would trade one wrong answer for another: a HIP host with a stray coli_cuda.dll left over from a CUDA build would pass while the backend it actually needs is absent. The binary already says which one it wants. backend_loader.c compiles exactly one basename in -- COLI_BACKEND_DLL is "coli_hip.dll" under COLI_HIP_DLL and "coli_cuda.dll" otherwise -- so read it out of the image and require that artifact. A binary carrying neither is not a DLL-split GPU build and keeps its existing CPU-only answer. This validates the host/artifact contract and nothing more. It does not claim the HIP runtime loaded, that runtime identity was verified, or that any GPU computed anything; those are separate contracts with their own checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is the placement/accounting half of PR #893, split as requested in review. It is testable locally and does not touch model kernels, routing, Vulkan, Metal, or runtime cache policy.
Included
doctoruses backend-neutralaccelerator.gpureporting rather than labeling every accelerator CUDA.Related issues
This addresses the planner/diagnostic portion of the family discussed in:
The backend-specific fixes from #887/#892, #813/#585, and Kimi #848 are deliberately not included. They need hardware reporters and separate review.
Verification
112 passed.407 passed, 57 skipped.git diff --check: passed.The plan remains an estimation layer; this PR does not claim that it replaces runtime allocation or provides a runtime speedup. It makes the estimator's pool and width assumptions explicit and testable before a later CNRE runtime policy is introduced.