Repository navigation
docs: sync PR template clippy and test commands with CONTRIBUTING.md - #1583
Merged
inureyes merged 1 commit intoSep 2, 2026
Merged
Conversation
The PR template's pre-submit checklist listed `cargo clippy --workspace --all-targets -- -D warnings` and `cargo test --workspace --profile test-fast`, while CONTRIBUTING.md specifies `--features metal,accelerate` on both commands plus `--no-fail-fast -- --test-threads=1` on the test line. A contributor who followed the template ran the wrong gate: no Metal/Accelerate feature set and a parallel test run, which CONTRIBUTING.md documents as the condition that crashed `mlxcel-core` on macOS (lablup#1092). Copy the two commands verbatim from the CONTRIBUTING.md local quality gates so the template and the contributor guide cannot disagree. Docs-only change; verified with `grep -F` that both template lines now match CONTRIBUTING.md byte for byte. Closes lablup#1232
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
Copies the clippy and test commands from the CONTRIBUTING.md local quality gates into the PR template's checklist, so a contributor who follows the template runs the same
--features metal,acceleratebuild and the same serialized (--no-fail-fast -- --test-threads=1) test run that the contributor guide requires.Related issues
Closes #1232
Type of change
docs— documentation onlyTest plan
grep -Fthat both template lines are byte-identical to the corresponding lines inCONTRIBUTING.mdcargo fmt --all -- --checkmain: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 #1238 against current
main. The template still listscargo clippyandcargo testas checkboxes even though CONTRIBUTING.md notes they are not gated at PR time; that is intentional, since CONTRIBUTING.md asks contributors to run them locally before pushing.