Repository navigation
refactor(cli): deduplicate eleven identical image-loading closures into open_image - #1252
Closed
DebayanSen96 wants to merge 1 commit into
Closed
DebayanSen96 wants to merge 1 commit into
DebayanSen96 wants to merge 1 commit into
Conversation
…to open_image The same image::open error closure was copy-pasted eleven times across generate_vlm.rs (10x) and generate_diffusion.rs (1x), and the copies had drifted in formatting (inline capture style, bare anyhow! import). Add one shared open_image(&Path) helper in commands/mod.rs and call it from all eleven sites. User-facing error text is byte-identical, so nothing observable changes. Fixes lablup#1245
Contributor
Author
|
Closing — contribution needs local build/test verification before review. |
7 of 8 tasks
4 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.
inureyes
added
type:refactor
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.
Fixes #1245.
The same
image::openerror closure was copy-pasted eleven times acrossgenerate_vlm.rs(10x) andgenerate_diffusion.rs(1x), and the copies had started drifting in formatting (one inline-capture variant atgenerate_vlm.rs:777, a bareanyhow!import ingenerate_diffusion.rs).Changes:
pub(crate) fn open_image(path: &Path) -> anyhow::Result<image::DynamicImage>insrc/commands/mod.rswith the canonical message.open_image(path); the closure bodies are gone.No Rust toolchain on this machine, so I couldn't run
cargo fmt/cargo testlocally — I formatted the call sites to what rustfmt would produce (single-line.map(|path| open_image(path))) and the helper mirrors the exact code it replaces.