Skip to content

Fix Windows probes in the dasllama bench harness - #3582

Draft
artarasov wants to merge 1 commit into
GaijinEntertainment:masterfrom
artarasov:artem/dasllama-bench-windows-fixes
Draft

Fix Windows probes in the dasllama bench harness#3582
artarasov wants to merge 1 commit into
GaijinEntertainment:masterfrom
artarasov:artem/dasllama-bench-windows-fixes

Conversation

@artarasov

Copy link
Copy Markdown
Contributor

llama-bench --ref never spawned on Windows: _popen goes through cmd /c, which strips the first and last quote of the command line, so the quoted binary path breaks. das_version probed bin/daslang, which cmd can't resolve, and wmic is gone on Win11 24H2+ so the ram/gpu fields came back empty. All three fixed in profile_common.das. Found while benching SmolLM2 on a 14700HX box.

@borisbat borisbat 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.

Verified all three fixes on a Windows 11 Pro 26200 (25H2) box, through the real popen path rather than from a shell:

Claim Result
cmd can't resolve bin/daslang confirmed — 'bin' is not recognized; bin\daslang0.6.3
cmd /c strips the first and last quote confirmed — "…\where.exe" "cmd.exe"'C:\Windows\System32\where.exe" "cmd.exe' is not recognized
The sacrificial outer wrap fixes it confirmed
The wrap is "safe either way" confirmed on a plain command, on one with a 2>nul redirect, and on a single bare token

Lint and format are clean on the changed file. Diagnoses all look right. One thing I think needs another pass before this lands.

The virtual-adapter preference sits in the branch that doesn't run

The premise is that wmic is gone on 24H2+. It isn't gone on this box — 25H2, and wmic answers fine:

Name=Parsec Virtual Display Adapter

Name=NVIDIA GeForce RTX 5060 Ti

platform_gpu() returns the first Name= line and short-circuits (profile_common.das:483-488, unchanged by this PR), so this box records Parsec Virtual Display Adapter as its GPU — and the new prefer-physical logic below it never executes. As written the ordering fix only helps boxes where wmic is actually absent, which is not the same set as "24H2+" (the WMIC feature-on-demand is deprecated and off by default on clean installs, but survives an in-place upgrade).

Suggest hoisting the selection into a small helper and running it over both outputs, or dropping the wmic branch entirely and always going through CIM.

Smaller things

  • "Virtual" is a narrow filter. It catches Parsec and VirtualBox, but not Microsoft Remote Display Adapter, Microsoft Basic Display Adapter, Microsoft Hyper-V Video, VMware SVGA 3D, or Citrix Indirect Display Adapter. Case-sensitive as well.

  • The wmic miss is noisy. run_capture doesn't redirect stderr, so on a genuinely wmic-less box the harness prints 'wmic' is not recognized as an internal or external command, to the console twice in the middle of a bench run. 2>nul on both wmic commands handles it — and it survives the popen path (tested above).

  • The shell-free path already exists in-tree. daslib/fio.das:648 run_and_capture(args, output, timeout) goes through popen_argvCreateProcess directly, so neither the quote rule nor / vs \ applies. das_version() converts in one line and the new is_windows() branch disappears with it. run_capture can't move wholesale — it runs real shell pipelines like lspci | grep — and run_llama_bench would need its three callers to build argv, so the sacrificial wrap is a reasonable pragmatic fix there. Worth naming popen_argv in that comment so the next person doesn't re-derive the cmd /? rule.

  • run_capture has no f != null guard (profile_common.das:307) while run_llama_bench does. Pre-existing, but this PR is specifically about probes failing to spawn.

Thanks for chasing this down — the cmd quote rule in particular is a trap we keep re-hitting.

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.

2 participants