Repository navigation
docs(bench): clean scrub artifacts in benchmark_results - #1584
Merged
inureyes merged 1 commit intoSep 2, 2026
Merged
Conversation
`docs/benchmark_results/model_tests_m5max.md` and `model_tests_m1ultra.md` carried leftovers from the issue-reference scrub: a relative link to `turbo-kv-cache.md` that resolved into `benchmark_results/` where the file does not exist, a sentence that lost its subject ("found that fixture collapses"), four bold spans written as `** text**` that CommonMark renders as literal asterisks, and dangling "epic-" and "(post-)" fragments where an issue number used to be.
Point the link at `../turbo-kv-cache.md` the way the sibling `model_tests.md` does, restore the missing sentence subjects, close the bold markers so the Phase-1b/2/3 and follow-up headings render, and name the referents the scrub removed: the "epic-" matrix is the TurboQuant KV speed gate matrix and the "(post-)" rows are the post-Phase 3 steel envelope readings.
Docs-only change. Verified that `docs/turbo-kv-cache.md` exists at the new link target and that no `** `, `epic-`, `(post-)`, or "exactly as." fragments remain in either file.
Closes lablup#1233
This was referenced Sep 2, 2026
inureyes
added
good first issue
inureyes
pushed a commit
that referenced
this pull request
Sep 2, 2026
## Summary
Replaces the fourteen copy-pasted `image::open(path).map_err(...)` closures in `generate_vlm.rs` and `generate_diffusion.rs` with a single `open_image` helper. The error text is byte-identical to the previous majority form; the drifted variants (inline `{error}` capture, bare `anyhow!` import, mixed `|e|`/`|error|` binders) disappear.
## Related issues
Closes #1245
## Type of change
- [x] `refactor`: internal restructuring without behavior change
## Test plan
- [x] `cargo fmt --all -- --check`
- [x] `cargo check --workspace --all-targets --features metal,accelerate` (initially on a CPU-only MLX build via `MLXCEL_BUILD_METAL=OFF`; superseded by the Metal gate below)
- [x] `cargo clippy -p mlxcel --bin mlxcel --features metal,accelerate -- -D warnings` (clean; in particular no `redundant_closure` on the `.map(|path| open_image(path))` sites, since the closure performs the `&PathBuf` to `&Path` coercion)
- [x] `RUST_MIN_STACK=16777216 cargo test --features metal,accelerate -p mlxcel --bin mlxcel commands::` (169 passed)
- [x] `grep -rn "Failed to load image" src/commands/` matches exactly one line, the helper body
- [ ] Real-checkpoint validation not run: the change does not touch inference, only the error path when `image::open` fails, and the success path returns the same `DynamicImage` as before
- [x] Full local gate from CONTRIBUTING.md on a Metal MLX build, run on a local branch that merges all eight re-implementation PRs (#1583, #1584, #1585, #1586, #1587, #1589, #1590, #1591) onto `main`: `cargo clippy --workspace --all-targets --features metal,accelerate -- -D warnings` (clean) and `cargo test --workspace --profile test-fast --features metal,accelerate --no-fail-fast -- --test-threads=1` (114 test binaries, 10385 passed, 0 failed, 326 ignored)
## Notes for reviewers
This re-implements the closed PR #1252 against current `main`. The issue counted eleven sites; `main` has since grown to thirteen in `generate_vlm.rs` plus the one in `generate_diffusion.rs`, all converted here. The helper lives in `generate_vlm.rs` next to its heaviest users and is `pub(crate)` so `generate_diffusion.rs` can reach it as `super::generate_vlm::open_image`; a separate `commands/util.rs` felt heavy for a one-function module, but it is a trivial move if you would rather have it there.
inureyes
pushed a commit
that referenced
this pull request
Sep 2, 2026
## Summary Removes the internal `(A4)` and `(B11)` task codes from doc comments and inline comments so the rendered rustdoc reads as plain sentences. `A4` was the `--surgery` CLI wiring and `B11` the llama-server `--cache-type-k/--cache-type-v` split flags; the commits that introduced them had their issue references scrubbed, so there is no public `(issue #NNN)` to substitute and the prefixes are simply dropped. ## Related issues Closes #1234 ## Type of change - [x] `docs`: documentation only ## Test plan - [x] `cargo fmt --all -- --check` - [x] `cargo check --workspace --all-targets --features metal,accelerate` (initially on a CPU-only MLX build via `MLXCEL_BUILD_METAL=OFF`; superseded by the Metal gate below) - [x] `grep -rnE '\((: )?(A4|B11)\)|\((A4|B11):' src/` returns nothing - [x] Full local gate from CONTRIBUTING.md on a Metal MLX build, run on a local branch that merges all eight re-implementation PRs (#1583, #1584, #1585, #1586, #1587, #1589, #1590, #1591) onto `main`: `cargo clippy --workspace --all-targets --features metal,accelerate -- -D warnings` (clean) and `cargo test --workspace --profile test-fast --features metal,accelerate --no-fail-fast -- --test-threads=1` (114 test binaries, 10385 passed, 0 failed, 326 ignored) ## Notes for reviewers This re-implements the closed PR #1240 against current `main`. The issue listed six sites; a full grep found 23 parenthesized occurrences across 17 files (`src/server`, `src/bin/mlx_server.rs`, `src/commands/serve.rs`, `src/cli/turbo_args.rs`, `src/loading`, `src/models`, `mlxcel-surgery`), all handled here. Bare narrative mentions such as "once A4's `--surgery` flag lands" in the `mlxcel-surgery` integration-test module docs are left alone: they are stale in a different way (the flag has landed) and deserve their own rewrite rather than a mechanical prefix drop.
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
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
Cleans the reference-scrubbing leftovers in
docs/benchmark_results/model_tests_m5max.mdandmodel_tests_m1ultra.md: the one broken relative link in the repo, a sentence missing its subject, four bold spans that rendered as literal asterisks, and the dangling "epic-" / "(post-)" fragments where issue numbers were stripped.Related issues
Closes #1233
Type of change
docs— documentation onlyTest plan
docs/turbo-kv-cache.mdexists at the new../turbo-kv-cache.mdlink target (matches the siblingmodel_tests.md)grep -nE '^\*\* |epic-|\(post-\)| as\.$'returns nothing in either file**...**spans mentally against CommonMark: no leading space after the opening delimiter, so they now render as boldmain:cargo clippy --workspace --all-targets --features metal,accelerate -- -D warnings(clean) andcargo test --workspace --profile test-fast --features metal,accelerate --no-fail-fast -- --test-threads=1(114 test binaries, 10385 passed, 0 failed, 326 ignored)Notes for reviewers
This re-implements the closed PR #1244 against current
main. Where the scrub removed a referent I named it from context rather than guessing the issue number: the "epic-" matrix is the TurboQuant KV speed gate matrix that the surrounding headings describe, the "(post-)" table rows are the post-Phase 3 steel-envelope readings, and the "exactly as." sentence refers to the Phase 3 kernel described a few paragraphs earlier. The "This PR" phrasing in the follow-up paragraph was also rewritten, since the document is a benchmark log rather than a PR body.