lablup / lablup/mlxcel

fix(cli): unreachable 2D-parallelism guard in validate_pipeline_parallel_args, decide whether to delete it or widen the early return

Open
#1,131 0 comments 0 reactions 0 assignees View on GitHub
priority:low status:ready type:bug
Dominant language
Rust
Stars
467
Forks
54
Avg merge
4h 25m
Merged PRs (30d)
310

Description

## Summary

`validate_pipeline_parallel_args` in `src/commands/generate.rs` contains an `ensure!` whose condition is the exact logical negation of an early return eight lines above it. The guard can never fail and its message can never reach a user. This was found while reviewing #1129 (which closes #1112) and deliberately left out of scope there, because #1112's acceptance criteria required the validation logic to be unchanged.

The fix is not obvious, because the dead guard is evidence of one of two different intents. This issue asks a maintainer to decide which reading is correct before any code is written.

## Background

### The tautology

`src/commands/generate.rs`, `validate_pipeline_parallel_args`:

```rust
if pp.pp_layers.is_none() && pp.pp_size <= 1 {
return Ok(());
}

// ... comment block ...
let tp_size = args.tensor_parallel.tp_size;
if tp_size > 1 {
ensure!(
pp.pp_size >= 2 || pp.pp_layers.is_some(),
"2D parallelism requires --pp-size >= 2 (or an explicit --pp-layers spec) \
alongside --tp-size > 1"
);
```

The early return fires when `pp.pp_layers.is_none() && pp.pp_size <= 1`. Past that point the negation holds: `pp.pp_layers.is_some() || pp.pp_size >= 2`, which is precisely the `ensure!` condition. The guard is therefore always satisfied whenever it is evaluated, so it is dead code and its error message is unreachable.

#1129 corrected the flag name inside that message (it previously said `--tensor-parallel-size`, which no binary accepts) but deliberately did not touch the logic, since #1112 scoped the change to the message text only.

### The test records the same tautology without naming it

`validate_pipeline_parallel_args_rejects_2d_without_pp_enabled` in `src/commands/generate_tests.rs` sets `pp_size = 1`, `tp_size = 2`, and asserts `is_ok()`. Its comment reads: \"The validator returns early when PP is disabled, so this case is harmless\". So the test name claims the validator *rejects* this input while the assertion confirms it *accepts* it. The name has to change whichever way the logic question is resolved.

## Proposed Solution

This is a decision to make first, not a patch to write first. The two readings lead to entirely different changes, and picking between them requires knowing the intended semantics of `--tp-size` without a pipeline topology.

**Reading (a): the guard is redundant and should be deleted.** Under this reading, `--tp-size 2 --pp-size 1` is TP-only by design and is correctly accepted today. The early return is doing exactly what it should, the `ensure!` adds nothing, and the correct change is to remove the dead `ensure!` and rename the misleading test to describe what it actually asserts (that a TP-only invocation is accepted). No user-visible behaviour changes.

**Reading (b): the early return is too broad.** Under this reading, a run with `tp_size > 1` that names no pipeline topology was meant to be rejected rather than silently downgraded to TP-only, and the early return should never have swallowed that case. The fix is to add a `tp_size` condition to the early return (so it only short-circuits when TP is also disabled), which makes the existing `ensure!` reachable and lets its message finally surface. The test then keeps its current name and flips to asserting an error.

Note that (b) is a behaviour change: command lines that work today (`--tp-size 2` with default `--pp-size`) would start failing. That needs a deprecation thought, for example a warning first and a hard error in a later release, rather than an immediate hard failure.

## Implementation Notes

- Whichever reading wins, `validate_pipeline_parallel_args_rejects_2d_without_pp_enabled` in `src/commands/generate_tests.rs` must be renamed or re-asserted so that its name and its assertion agree, and the comment explaining the early return should be updated to match the resolved intent.
- Under (a), the `total_ranks > 0` sanity check immediately after the dead `ensure!` should be reviewed at the same time: `saturating_mul` of two values that are already known to be at least 1 and at least 2 respectively cannot produce 0, so it may be dead for the same reason.
- Under (b), check the cluster-TOML validator layer as well, since the in-code comment says the full `pp_size * tp_size == nodes` check lives there. A rejection added at the CLI layer should not contradict or duplicate what the cluster layer already enforces for remote topologies.

### Separable follow-up: machine-check flag names in error strings

The sweep that surfaced the original `--tensor-parallel-size` defect exists only as a shell pipeline in #1129's description:

```
grep -o -- '--[a-z][a-z0-9-]*' src/commands/generate.rs | sort -u | comm -23 - <(mlxcel generate --help | ...)
```

`tests/cli_help_consistency.rs` is the established home for machine-checked CLI-surface invariants. A test asserting that every `--flag` named in an error-message string literal actually appears in `--help` would close the whole class of defect instead of the one instance #1112 fixed. This is genuinely separable from the logic decision above and could be its own issue if a maintainer prefers to split it.

## Acceptance Criteria

- [ ] A maintainer records a decision on the issue: reading (a) (guard is redundant, TP-only is intended) or reading (b) (early return is too broad, TP without a pipeline topology should be rejected).
- [ ] The chosen change is implemented in `src/commands/generate.rs`: under (a), the unreachable `ensure!` is removed; under (b), the early return gains a `tp_size` condition so the guard becomes reachable.
- [ ] Under (b) only: a deprecation path is agreed and implemented, so command lines that are accepted today do not start hard-failing without warning.
- [ ] `validate_pipeline_parallel_args_rejects_2d_without_pp_enabled` in `src/commands/generate_tests.rs` is renamed or re-asserted so its name matches its assertion, with the stale comment updated.
- [ ] No remaining branch in `validate_pipeline_parallel_args` is unreachable by construction, including the `total_ranks > 0` check if reading (a) is chosen.
- [ ] The flag-name-in-error-strings invariant is either added to `tests/cli_help_consistency.rs` or split into its own issue and linked from here.

Contributor guide

Open the contributing guide

Research direction

Read validate_pipeline_parallel_args in src/commands/generate.rs and validate_pipeline_parallel_args_rejects_2d_without_pp_enabled in src/commands/generate_tests.rs first. Confirm the intended semantics of --tp-size without pipeline topology with a maintainer, then implement the chosen path, align the test name, assertion, and comment, and check that no branch remains unreachable. Review tests/cli_help_consistency.rs only if the separate flag-invariant work is included.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
cli, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.