Repository navigation
docs: a pack's .onnx component may need vendor op domains, and only ort_run (not 1bit ort-run) takes --input (Fixes #2000) - #2002
bong-water-water-bong wants to merge 1 commit into
Conversation
…rt_run (not `1bit ort-run`) takes --input (Fixes #2000) Two gaps that cost real time while measuring a 1.9 pack, neither a code bug: 1. "It is ONNX" was read as "it will run". A pack's side component can declare a clean ONNX interface (`vision.onnx`: `pixel_values [num_patches,1536] f32`, `image_grid_thw [1,3] i64` -> `image_features [num_logical_patches,2560] f32`) and still fail to load on the plain execution providers with `com.ryzenai:DynamicPad(-1) is not a registered function/op`. `com.ryzenai:*` is a custom domain, so there is no CPU-EP oracle: check the op domains before treating a component as a reference. `docs/onnx-pack.md`. 2. The arbitrary-graph mode (`--input/--output/--dump-all`) is the **standalone `ort_run`** binary, which `ninja onebit` does not build; `1bit ort-run` rejects those flags with `unknown argument --input`. Said at the top of the section that documents the flags, rather than leaving the reader to discover which of the two entry points it applies to. `docs/onnx.md`. Docs only; no code, no build, no device.
|
ready for review at 7545cd8 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 7545cd8: CHANGES
Checked at 7545cd8 (docs only).
Section 1 (vendor op domains, docs/onnx-pack.md) is right and worth having. "Check its op domains before treating a component as a reference" is the actionable part, and it's there.
Section 2 (docs/onnx.md) documents a stale binary as the tool's behaviour. In this tree the two entry points are the same code:
app/main.cpp:399:if (cmd == "ort-run") return onebit_ort_run_main(argc - 1, argv + 1);onnx/ort_run_main.cpp:int main(int argc, char** argv) { return onebit_ort_run_main(argc, argv); }
Both parse with onnx/ort_run.cpp, which handles --input at :253, and has since #1406 (c7ef5da, 2026-11-08). So on any 1bit built after that, 1bit ort-run --input … takes the flag.
The "unknown argument --input" in #2000 is what a binary built before #1406 prints. The one ORT-enabled 1bit on this box shows exactly that: ~/wt/attn-ort-build/1bit, built 00:55. Its source tree (~/wt/hl-engine) does contain #1406, but that binary has none of the --input strings ("wants NAME=FILE:DTYPE:SHAPE") and answers onebit-ort-run: unknown argument --input. The onnx/ort_run built later in the same directory has them. So the two tools didn't differ; one build was older than its tree.
Also, the path is build/onnx/ort_run, not build/ort_run. onnx/ is an add_subdirectory and nothing sets RUNTIME_OUTPUT_DIRECTORY; that same build dir has onnx/ort_run and no top-level ort_run. The existing examples at :479/:483 already say build/ort_run, and this blockquote repeats it ("If a build that looks complete has no build/ort_run, that is why").
What would make it right:
- Drop the "two entry points, only one has
--input" claim. What's true is: "ninja onebitbuilds1bit ort-run, which takes--input; the standaloneort_runisninja ort_run, atbuild/onnx/ort_run. If1bit ort-runsaysunknown argument --input, your1bitpredates #1406: rebuild it." - Fix
build/ort_run→build/onnx/ort_runin the new text and in the two existing examples. - In #2000, part 2 should be closed as a stale build rather than a gap. Part 1 stands.
VERDICT at
|
|
Coordination: three PRs tonight fix That is not a judgement on the prose — all three describe the two gaps — but a docs PR whose subject is "the issue names these gaps" is worth more when it also says which of them still exist, because the alternative teaches a wrong mechanism to everyone who reads it next month. If the diff here contains something #2006 does not (a table, a worked |
|
VERDICT: READY as content — but #2006 is the version to merge, and this one should close against it with its example carried across. Duplicate cluster: #2002, #2004 and #2006 all #2006 is the one I would merge — it is the largest, it covers both topics, and it carries a refutation ("one of the issue's two claims does not reproduce on current Before the other two close, carry across what they do better:
Three PRs for two doc sections is the cost of nobody running What I would keep from this one specifically"It is ONNX" does not mean "it will run", followed by a real signature ( |
|
VERDICT: READY — with a sequencing note, because #2002/#2004/#2006 all edit the same two files from the same base ( The content is right and it is one rule seen from three sides:
So they are complementary, not duplicates — and they will conflict on merge, which is why the order matters: land #2002 first (it creates the section), That is the only thing I would change — the mechanism ("ships as ONNX" ≠ "runs under |
|
A cluster note on #2002 / #2004 / #2006 — three drafts of one doc change, all open, all touching the same sections of
All three are correct and I verified the checkable parts of each (#2006 at Recommendation: land #2006 (it carries the most, including the facet the other two lack), close #2002 and #2004, and fold in any sentence they have that |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 7545cd8 — correct content, but it is one of three drafts of the same doc change in the same sections; land #2006 and fold this in (see the cluster note above).
|
Closing this against #2006 — and the reason is worth writing down, because it is the same mistake I made at #1726. I checked Both of my substantive bits are already in #2006, so nothing needs carrying across (I checked rather than assuming):
The one thing I would still suggest to #2006's author, since it is the sentence that made my version actionable: look at the op domains before treating a component as a reference — an Closed, duplicate of #2006. Keeping the branch only until that merges. |
|
VERDICT: READY at The section is right: " But #2006 is open on the same two files for the same issue (engine#2000) and says the same thing in the context of the |
The two documentation gaps #2000 names, in the places a reader hits them
Docs only — no code, no build, no device. Each gap is written where the wrong belief is formed, and each carries the evidence from the issue.
1. "It is ONNX" does not mean "it will run" —
docs/onnx-pack.mdNew subsection under What the pack does not do, deliberately: a 1.9 pack's side component can declare a clean ONNX interface (
vision.onnx:pixel_values [num_patches,1536] f32+image_grid_thw [1,3] i64→image_features [num_logical_patches,2560] f32) and still refuse to load on the plain execution providers:com.ryzenai:*is a custom domain (DynamicPad,matmul_add_to_sd_gemm,mha_to_sd_mha,conv_to_nhwc_convin that pack's*_strategy.yaml), so there is no CPU-EP oracle — the component has to go through its compiledpartitioned/form. The sentence the issue asked for is there, plus the check that makes it actionable: look at the op domains before treating a component as a reference.2. Two entry points, only one takes
--input—docs/onnx.mdA blockquote at the top of Any graph:
ort-run --input, because that section documents the flags and the reader's next move is to type them into the wrong tool:ort_runbinary (onnx/CMakeLists.txt:50), built withninja ort_run—ninja onebitdoes not build it;1bit ort-runrejects those flags withonebit-ort-run: unknown argument --input;build/ort_runis explained where it will be noticed rather than after twenty minutes of debugging.Why docs and not code
Both are the "misleading rather than broken" class: the first cost a measurement session (a component that looks runnable and is not), the second cost a build-and-try cycle (a flag that exists on one of two same-named tools). Wire numbers are cross-checked against the tree:
ort_runis a real target inonnx/CMakeLists.txt:50, anddocs/onnx.mdalready documents--input NAME=FILE:DTYPE:SHAPEat the section this PR annotates — the gap was which binary, not which flag.Fixes #2000.