Repository navigation
Bump HRX: llama.cpp a5c32fbf8b (fork tip: the converter registers --dspark, engine #961) - #992
Conversation
…spark, engine #961) The pin was d8bd0ff6052, whose convert_hf_to_gguf.py reads args.dspark at :261 and registers no --dspark argument, so every conversion died with AttributeError on an argparse Namespace that has no such attribute (engine #961). a5c32fbf8b is the fork's 1bit/hrx-vulkan-patched tip: #187 squash-merged on d8bd0ff6052, one commit ahead, which adds the registration at :156 and the V4/V4.1 arch guard. tools/check_pins.py: d8bd0ff60 -> a5c32fbf8 is 'ahead' (no other submodule moves).
PR Reviewer Guide 🔍(Review updated until commit d65a033)Here are some key observations to aid the review process:
|
The pin bump moves third_party/llama.cpp d8bd0ff60 -> a5c32fbf8 but not the registry's
record of it, and registry/architectures.json is what ctest `registry_pins` compares the
committed pins against - so the bump as opened fails that test.
Regenerated with the documented recipe (`scripts/registry-regenerate`: `scripts/registry-regen.sh`,
which is also what the bump workflows run) from this commit in a fresh clone: it inits the three
trees the registry reads at their pins, rewrites the file, and then checks it:
$ scripts/registry-regen.sh
wrote registry/architectures.json: hrx 294, npu 7, architectures 294, mapped 294
none of the 28 reviewed significant classes is mapped without a reason
registry/architectures.json records the committed pins
One line changes - the recorded pin - because a5c32fbf8b only adds the converter's --dspark
argparse block (#187), which moves no architecture. tools/copyright.py --check is clean.
|
Pushed
Done with the documented recipe, from a fresh clone ( One line changes — the recorded pin — because Also verified the fix actually arrives with the new pin, since that is the whole point of the bump (default
Nothing else touched: the diff on this branch is the gitlink plus this one-line registry update. |
|
Persistent review updated to latest commit 646c408 |
|
The conversion proof, from the new pin at That is byte-identical to #961's reference ( So the acceptance for #961 from this branch is: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY — the bump is one gitlink plus its registry line, and CI is green on both.
Reviewed as the author of the bump, at 646c4085 (two commits: 76eebc11c the gitlink, 646c4085f the registry line).
What I verified:
git diff origin/main...646c4085 --statis exactlyregistry/architectures.json 2 +-andthird_party/llama.cpp 2 +-,
i.e. no third file rides along with a pin bump.- The pin target resolves:
git fetch --depth 1 origin a5c32fbf8bfrom a clean clone gives one commit on top of
d8bd0ff60, and at that commitgrep -c '"--dspark"' convert_hf_to_gguf.pyis 1 (0 at the old pin). - The registry line is the generated half of the same change, not a hand edit:
registry/architectures.jsonrecords
llama.cpp (hrx)and ctestregistry_pinscompares the committed pins against it, so the gitlink alone fails that
test. The new value isa5c32fbf8b5ae1f060b69b1d01393512b7329134,countsunchanged at hrx 294 / npu 7 — right, since
#187 only adds the converter's argparse block. tools/check_pins.py origin/main->ok third_party/llama.cpp: d8bd0ff60 -> a5c32fbf8 is ahead;tests/check_pins_test.py
3 tests OK.- CI:
buildpass,CodeQLpass,Analyze (actions|c-cpp|javascript-typescript|python|rust)5x pass,
hrx-system gitlink agrees with the hold filepass,pinned loom-jit.cpp compiles against the pinned hrx-systempass.
pr_agentcancels or fails on every engine PR while coder-llm is off;hw_gateskips. Neither is a gate here.
Still true, and already in the body: the converter cannot be run on this box (import transformers fails on the
huggingface-hub version), so no converted GGUF is claimed; --dspark + DeepseekV4ForCausalLM still raises ImportError
because conversion/deepseek.py has no DeepseekV4DSparkModel on that branch — DeepSeek-V4.1 is the path that becomes
usable. A later fork tip needs its own bump PR; this one covers d8bd0ff60 -> a5c32fbf8b only.
|
Persistent review updated to latest commit 0a41718 |
|
Persistent review updated to latest commit 1bdccf5 |
|
Persistent review updated to latest commit 598c11c |
|
Persistent review updated to latest commit 232b152 |
|
Persistent review updated to latest commit d65a033 |
closes #961.
What moves
third_party/llama.cpp:d8bd0ff6052a→a5c32fbf8b5a, the fork's1bit/hrx-vulkan-patchedtip. One submodulegitlink, nothing else.
a5c32fbf8bisd8bd0ff6052+ fork #187 squash-merged, i.e. linearly one commit ahead —tools/check_pins.pyagrees:and the commit resolves from a clean clone of the fork (
git fetch --depth 1 origin a5c32fbf8b, one commit, parentd8bd0ff60).Why it fixes #961
At the old pin,
convert_hf_to_gguf.pyreadsargs.dsparkat:261and registers no--dsparkargument, soargparse.Namespacehas no such attribute and every conversion dies withAttributeErrorthere, whatever the model.The new pin adds the registration at
:156and the V4/V4.1 architecture guard at:265-267:What this pin does not bring, so it is not a surprise later
--dspark+DeepseekV4ForCausalLM(V4, not V4.1) still fails: the fork branch has noDeepseekV4DSparkModelinconversion/deepseek.py(nor aQwen3DSparkModelinconversion/qwen.py), althoughconversion/__init__.py:61listsboth and
main()imports the V4 class fromconversion.deepseek. The classes exist on other fork branches and comefrom upstream
3581ba0cf(#26452). DeepSeek-V4.1 is the path that becomes usable (DeepseekV41DSparkModelis inconversion/deepseek41.py:300). Noted in #961 and in the #187 review; not fixed by a pin bump.Not done here
No engine build was run for this bump on this box (no HRX deps here, and it is a gitlink-only change) — the PR's own
CI
buildjob is what compiles it, and the converter itself cannot be executed on this box at all yet:import transformersfails withhuggingface-hub>=1.5.0,<2.0 is required … but found huggingface-hub==2.4.0. That is anenvironment fact recorded in
~/briefs/TPRS-STATE.md, not something this pin can fix.