Repository navigation
docs: replace opaque (A4)/(B11) task codes in code comments - #1586
Merged
Merged
Conversation
Doc comments and inline comments across the server, CLI, loading and surgery crates carried internal task codes, `(A4)` for the `--surgery` CLI wiring and `(B11)` for the llama-server `--cache-type-k/--cache-type-v` split flags, that mean nothing to a reader of the rendered rustdoc. The codebase convention elsewhere is `(issue #NNN)`, but the commits that introduced these codes had their issue references scrubbed, so there is no public number to substitute. Drop the `(A4):` / `(B11):` / `(— A4)` prefixes at every site so each comment reads as a plain sentence, capitalizing the first word where the prefix used to carry it. Three rustdoc section headers (`## Active-pipeline fallback (— A4)`) and the `Used by:` line in `mlxcel-surgery/src/config.rs`, which still said "future CLI wiring (A4)" although the wiring has landed, now name the `--surgery` flag directly. Comments only, no code changes. Validated with `cargo fmt --all -- --check` and `cargo check --workspace --all-targets --features metal,accelerate` (CPU-only MLX build, `MLXCEL_BUILD_METAL=OFF`). `grep -rnE '\((— )?(A4|B11)\)|\((A4|B11):' src/` returns nothing. Closes lablup#1234
This was referenced Sep 2, 2026
inureyes
added
good first issue
3 tasks
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.
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
Removes the internal
(A4)and(B11)task codes from doc comments and inline comments so the rendered rustdoc reads as plain sentences.A4was the--surgeryCLI wiring andB11the llama-server--cache-type-k/--cache-type-vsplit 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
docs— documentation onlyTest plan
cargo fmt --all -- --checkcargo check --workspace --all-targets --features metal,accelerate(initially on a CPU-only MLX build viaMLXCEL_BUILD_METAL=OFF; superseded by the Metal gate below)grep -rnE '\((— )?(A4|B11)\)|\((A4|B11):' src/returns nothingmain: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 #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--surgeryflag lands" in themlxcel-surgeryintegration-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.