Repository navigation
chore(cli): unify error-message capitalization across CLI commands - #1590
Merged
inureyes merged 1 commit intoSep 2, 2026
Merged
Conversation
Ten top-level `anyhow!` error strings in `src/commands/*.rs` started with a lowercase letter while the majority style across the CLI capitalizes the first word, so `mlxcel detect`, `chat`, `models`, `generate`, `generate-florence2` and `generate-falcon-ocr` printed differently styled errors than the rest of the commands.
Capitalize the first word at those ten sites (`detect.rs` x2, `chat.rs`, `models.rs`, `generate_florence2.rs`, `generate_falcon_ocr.rs`, `generate.rs` x4) and update the one exact-match assertion in `generate_tests.rs`. Pure string edits, no logic change.
The `not valid JSON: {error}` string in `generate_falcon_ocr.rs::parse_layout_detections` is intentionally left lowercase: its caller wraps it as `--layout-detections <path>: not valid JSON: ...`, so it is a nested cause fragment rather than the first word of the message the user sees, and the two `contains("not valid JSON")` assertions in `generate_falcon_ocr_tests.rs` keep passing unchanged.
Validated with `cargo fmt --all -- --check`, `cargo check --workspace --all-targets --features metal,accelerate` and `RUST_MIN_STACK=16777216 cargo test --features metal,accelerate -p mlxcel --bin mlxcel commands::` (169 passed), all on a CPU-only MLX build (`MLXCEL_BUILD_METAL=OFF`).
Closes lablup#1236
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
Capitalizes the first word of the ten top-level
anyhow!error messages insrc/commands/*.rsthat still started lowercase, so every CLI command prints errors in the same style as the majority (Tokenization failed: ...). One exact-match assertion ingenerate_tests.rsis updated to match.Related issues
Closes #1236
Type of change
chore— build, CI, dependencies, release infrastructureTest 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)RUST_MIN_STACK=16777216 cargo test --features metal,accelerate -p mlxcel --bin mlxcel commands::(169 passed; the stack bump is the dev-profile prefix CONTRIBUTING.md documents)grep -rnE 'anyhow!\("[a-z]' src/commands/*.rsnow matches only the one intentional site belowmain: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 #1241 against current
main. One deliberate deviation from the issue's site list:not valid JSON: {error}ingenerate_falcon_ocr.rs::parse_layout_detectionsstays lowercase because its caller prefixes it (--layout-detections <path>: not valid JSON: ...), so the user never sees it as the first word of a message. Capitalizing it would produce a mid-sentence capital and also break the twocontains("not valid JSON")assertions ingenerate_falcon_ocr_tests.rs. Happy to flip it if you would rather have the mechanical rule applied everywhere. The lowercasebail!messages inembed.rs,rerank.rs,tune.rsandgenerate_falcon_ocr.rsare outside the issue'sanyhow!scope and untouched.