Skip to content

feat(plan): discover Windows AMD GPUs without planning against them - #931

Open
Kenneth-Javier wants to merge 6 commits into
JustVugg:devfrom
Kenneth-Javier:fix/amd-discovery-windows
Open

feat(plan): discover Windows AMD GPUs without planning against them#931
Kenneth-Javier wants to merge 6 commits into
JustVugg:devfrom
Kenneth-Javier:fix/amd-discovery-windows

Conversation

@Kenneth-Javier

Copy link
Copy Markdown
Contributor

Summary

Device discovery does not imply planning eligibility.

  • Windows AMD discovery currently depends on rocm-smi, which is not available through the Windows HIP environment used by this path. Windows AMD hosts were therefore planned as if no GPU had been discovered: nothing in coli plan, "no supported GPU detected", and --gpu N failing on working hardware.
  • This change uses hipInfo.exe for Windows AMD identity reporting: device name, architecture, integrated flag and reported total memory.
  • Integrated Windows AMD devices are reported with free_bytes=None. They remain visible to reporting while being excluded from automatic placement until a qualified free-memory budget exists.
  • The Windows launcher uses the same placement-eligibility predicate, and doctor checks the backend artifact the host was built to load.

Design background: Discussion #857. This PR does not close it.

Why

Discovery is a fact; a placement budget is a policy decision, and the planner had no way to separate them: free_bytes was mandatory and numeric, and every consumer read "a GPU is present" as "a GPU may be planned against". That reading stops holding when the GPU and the host draw on one physical pool.

free_bytes may now be None, meaning "not qualified as a budget on this platform". It is deliberately distinct from 0, which is a measurement ("the card is full"); plans_placement() tests is not None rather than truthiness so the two cannot collapse. Existing devices with numeric free_bytes retain their current planning semantics.

Implementation

1. Discovery_discover_amd_gpus() gains a Windows branch. hipInfo.exe is located from COLI_HIP_RUNTIME_DIR, then %HIP_PATH%\bin, then PATH, stopping at the first hit; the Colibri-specific runtime directory leads because it names the runtime the loader will bind, and a host may carry more than one HIP install. No install location is hardcoded, and no new dependency is introduced — hipInfo comes from the HIP environment already required to build the backend. A block missing a name or a memory total yields no device rather than one completed with zeros, and missing, failing or unparsable hipInfo yields no device at all. The Linux rocm-smi path is untouched.

2. Planning boundary — only devices whose free memory is qualified reach the VRAM budget, the shared-pool unified flag, _auto_tune, the bottleneck classification, the "VRAM already in use" warning, and the COLI_GPU/COLI_GPUS device list. Everything discovered still reaches reporting, marked (identity only) with a warning naming it. The launcher has a separate auto-enable path that writes COLI_CUDA/COLI_GPUS before environment_for_plan() is consulted, so it filters through the same predicate rather than a second local rule.

3. Doctor — the Windows probe required coli_cuda.dll regardless of what the host was built to load, so a working HIP install reported "GPU runtime library is missing", which is a fail and makes the whole report error. backend_loader.c compiles exactly one basename into the host (COLI_BACKEND_DLL), so the binary states which artifact it will load and doctor now checks for that one. A CUDA host still requires coli_cuda.dll; a HIP host requires coli_hip.dll; neither is satisfied by the other.

Validation

  • make -C c check — 508 tests, OK, 52 skipped, exit code 0
  • CUDA changes were tested with make -C c cuda-test (if applicable) — N/A, no CUDA or C code is changed
  • Performance claims include hardware, commands, and repeatable measurements — N/A, no performance claim is made

On the local MSYS2 GCC 16.1 toolchain the check also emits existing compiler warnings from unchanged C test translation units; this PR changes no C or header files.

Targeted owner run:

python -m unittest tests.test_env_defaults tests.test_resource_plan tests.test_doctor
  -> 98 tests, OK

Twenty-three tests were added to three existing owners; no new test file. The contract is reviewable without AMD hardware:

  • CPU-only and no-GPU behaviour unchanged
  • numeric-free_bytes devices retain existing planning behaviour, including free_bytes=0
  • an identity-only device produces an environment byte-identical to the CPU-only host's
  • Windows hipInfo parsing: integrated, discrete, multi-device, absent, non-zero exit, incomplete block, lookup precedence
  • Linux rocm-smi discovery preserved
  • doctor accepts each host's own backend and rejects the other's
  • launcher auto-enable regression, both directions

Physical validation on AMD Radeon 8060S / gfx1151, Windows. Four controlled observations, each in its own rebooted session, varying the firmware shared-memory setting:

shared-memory setting HIP total Windows visible
~6 GB 76.79 GiB 127.15 GiB
~32 GB 76.79 GiB 127.15 GiB
~64 GB 76.79 GiB 127.15 GiB
~123 GB 93.00 GiB 127.15 GiB

The static HIP values are not a qualified direct budget for the tested shared-memory setting.

Compatibility

  • The default CPU build remains dependency-free
  • No model files, generated binaries, or benchmark artifacts are included
  • Linux AMD, Windows NVIDIA and CPU-only behavior is covered by regression tests; no macOS-specific path is changed

Not in this PR

Automatic Windows AMD shared-memory budgeting; dynamic allocation qualification; performance tuning; zero-copy claims; routed-expert placement; KV work; the Windows memory_available() fix; HIP-aware cuda_binary().

No performance improvement or automatic Windows AMD placement is claimed. The change enables truthful discovery while deliberately withholding automatic placement until a qualified free-memory budget exists.

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.
`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.
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.
The Windows bare-`coli chat` auto-enable reads discover_gpus() and writes
COLI_CUDA and COLI_GPUS into the engine environment itself, before
environment_for_plan() is ever consulted. The planner's own placement gate
therefore cannot cover this path, and a device discovered without a qualified
free-memory budget was enough to switch the GPU on.

With a Windows AMD device (free_bytes None) that produced a full auto-enable
and a sized expert tier from a budget nothing had measured:

    [GPU] auto-enabled CUDA · AMD Radeon(TM) 8060S Graphics · 8.6 GB expert tier
    COLI_CUDA=1  COLI_GPUS=0  CUDA_EXPERT_GB=8.000

It does not fire today only because cuda_binary() still looks for
coli_cuda.dll on Windows and so reports false on a HIP host. That is a
separate defect and is deliberately left alone here -- but it means the leak
is one unrelated fix away from becoming live, and whoever makes that fix
would have no reason to know this gate exists.

Filter at the source with the same predicate the planner uses, rather than a
second local rule that could drift from it. Auto-enable is an automatic
placement decision; only a device whose memory is qualified as a budget may
drive one.

Explicit user intent is untouched: --gpu and --vram never reach this branch
(it requires `a.gpu is None and not a.vram`) and keep working exactly as
before, as does every Windows NVIDIA host, whose devices carry numeric
free_bytes and remain qualified.
The discovery commit justified free_bytes=None with a single reading: hipInfo
reporting 89.24 GB free against 59.3 GiB of Windows-available memory, called
the same pages counted twice. The measurement was real, but it came from one
session under a firmware configuration that was not recorded, and the
explanation went further than the evidence.

Four controlled observations since, each in its own rebooted session with
everything else held constant, varying the Armoury Crate shared-memory limit
across its full range:

    ~6 GB    total 76.79 GiB   free 76.63 GiB   Windows visible 127.15 GiB
    ~32 GB   total 76.79 GiB   free 76.63 GiB   Windows visible 127.15 GiB
    ~64 GB   total 76.79 GiB   free 76.63 GiB   Windows visible 127.15 GiB
    ~123 GB  total 93.00 GiB   free 92.84 GiB   Windows visible 127.15 GiB

Three settings spanning a twentyfold range gave the same reading; only the
maximum differed. At the minimum the reported total was ~12.8x the configured
limit. Windows-visible memory and the 0.50 GiB dedicated frame buffer did not
move at all, including in the run where the HIP figure did -- so the increase
was not taken from the host.

That is a better argument for free_bytes=None than the original one, and a
more careful one. It says what was measured and stops there: no claim that
hipInfo over-reports, that the figure is a fixed hardware property, that the
setting maps onto it, or that HIP free and host-available memory are the same
pages. Whether they are is exactly what a bounded allocation experiment would
have to establish, and nothing here has allocated anything.

Wording only. The AST of resource_plan.py is unchanged with docstrings
excluded; the parser, the planning boundary and free_bytes=None all behave
exactly as before, and the owner suites stay green.
The new Windows backend-artifact tests passed on Windows and failed on Linux
and macOS, six of them, all with the same shape:

    {'linked': False, 'missing': False} != {'linked': True, 'missing': False}
    'warn' != 'pass'

The helper faked both sys.platform and os.name to reach the Windows branch.
Faking os.name is the problem: pathlib picks its flavour from it at
construction time, so the Path() that cuda_linkage() builds from the fixture
path became a WindowsPath. On a POSIX host /tmp/xxx/colibri.exe turns into
\tmp\xxx\colibri.exe, which does not resolve, so the is_file() guard returned
early and every assertion compared against a false negative -- the
backend-marker logic was never reached at all.

Measured on Ubuntu 24.04 / CPython 3.12:

    PosixPath   /tmp/xxx/colibri.exe    is_file() True
    WindowsPath \tmp\xxx\colibri.exe    is_file() False   (os.name = 'nt')

So the helper now fakes sys.platform only, which leaves pathlib alone. That
needs the dispatch to key on sys.platform as well, because os.name is still
"posix" under the test and would otherwise send it down the ldd branch.

On every real host the two agree -- Windows is nt/win32, Linux and macOS are
posix/linux and posix/darwin -- so all three select exactly the branch they
selected before. Only the synthetic posix/win32 pair behaves differently, and
that pair cannot occur outside a test.

The alternative was to skip these tests off Windows. They are worth more than
that: the Windows backend contract stays reviewable by anyone, on any runner,
without the hardware.

No change to the backend-marker scan, the expected-DLL extraction, the
artifact check, the linked/missing semantics, or run_doctor's accelerator
status.
@Kenneth-Javier
Kenneth-Javier force-pushed the fix/amd-discovery-windows branch from 33c92ab to 36378ea Compare August 12, 2026 14:25
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.

1 participant