Skip to content

fix(cli): validate boolean cache flags, fix CLI-over-env precedence#70

Merged
inureyes merged 1 commit into
mainfrom
fix/cli-flag-validation-help-cleanup
May 22, 2026
Merged

fix(cli): validate boolean cache flags, fix CLI-over-env precedence#70
inureyes merged 1 commit into
mainfrom
fix/cli-flag-validation-help-cleanup

Conversation

@inureyes
Copy link
Copy Markdown
Member

Summary

Tightens the boolean cache-flag CLI contract, fixes a CLI-over-env precedence bug for those flags, and cleans up the affected --help text.

Changes

Flag validation

--prompt-cache-enabled and --apc-enabled (on both the mlxcel-server binary and the mlxcel serve subcommand) now use require_equals + num_args = 0..=1 + default_missing_value = "true" + ArgAction::Set. This gives one well-defined contract: bare --prompt-cache-enabled means true, --prompt-cache-enabled=false disables it, and a stray space-separated --prompt-cache-enabled false is rejected with a clear parser error instead of silently keeping the default and treating false as an unexpected argument.

CLI-over-env precedence (bug fix)

A new long_cli_flag_was_set helper (defined in server::cli_input, re-exported from server) scans the process argv for an explicit long flag. Startup previously passed a hardcoded false for "was this set on the CLI", so the env-var fallback was always consulted and an explicit --prompt-cache-enabled=false / --apc-enabled=false could be silently overridden by MLXCEL_PROMPT_CACHE_ENABLED / APC_ENABLED. Both binaries now pass long_cli_flag_was_set(...), so an explicit CLI value wins while the compiled-in default stays overridable by env when the flag is absent.

Help-text cleanup

  • Removed stale internal tracker identifiers from the --help surface and option doc comments on the touched flags.
  • Repointed two stale doc links to this repository's layout: docs/distributed.md for the pipeline-parallelism workflow and the project README for the model-surgery operations. (One identical pre-existing broken link at the --pp-auto option was corrected in the same file for consistency.)
  • Added the pipeline_tensor_parallel value to the --node-role help list so it matches the roles the server accepts.

Tests

Updated the #[ignore]d prompt-cache integration tests (prompt_cache_e2e, prompt_cache_prefill_bench) to invoke --prompt-cache-enabled=true|false with the now-required = syntax; their previous space-separated form would fail to parse under the tightened contract.

Out of scope

A version bump and the en/ko docs + manpages from the originating change are intentionally excluded: this repository uses a flat docs/ tree with no corresponding files, and the version is left at 0.0.29.

Verification

  • cargo fmt --all -- --check — clean
  • cargo clippy --features metal,accelerate --lib --bin mlxcel --bin mlxcel-server --tests -- -D warnings — clean
  • Built mlxcel-server and confirmed parsing:
    • --prompt-cache-enabled=false → parses (then fails at model load, i.e. past arg parsing)
    • --prompt-cache-enabled false → rejected: 'false' cannot be used with '--prompt-cache-enabled[=<BOOL>]'
    • --prompt-cache-enabled (bare) → parses via default_missing_value

The boolean cache flags `--prompt-cache-enabled` and `--apc-enabled` accepted a loosely space-separated value, which parsed inconsistently with the `=`-delimited form shown in `--help`. Add `require_equals`, `num_args = 0..=1`, `default_missing_value = "true"`, and `ArgAction::Set` to both flags on both server entrypoints (the `mlxcel-server` binary and the `mlxcel serve` subcommand). The flags now have one well-defined contract: bare `--prompt-cache-enabled` means true, `--prompt-cache-enabled=false` disables it, and a stray `--prompt-cache-enabled false` is rejected with a clear parser error instead of silently leaving the default in place and treating `false` as an unexpected argument.

Restore CLI-over-env precedence for those two flags. A new `long_cli_flag_was_set` helper (defined in `server::cli_input`, re-exported from `server`) scans the process argv for an explicit long flag. Startup previously passed a hardcoded `false` for "was this set on the CLI", so the env-var fallback was always consulted and an explicit `--prompt-cache-enabled=false` / `--apc-enabled=false` could be silently overridden by `MLXCEL_PROMPT_CACHE_ENABLED` / `APC_ENABLED`. Both binaries now pass `long_cli_flag_was_set("prompt-cache-enabled")` and `long_cli_flag_was_set("apc-enabled")`, so an explicit CLI value wins while the compiled-in default stays overridable by env when the flag is absent.

Clean up the `--help` surface and the option doc comments on the touched flags: drop stale internal tracker identifiers, and repoint the two doc links to this repository's layout — `docs/distributed.md` for the pipeline-parallelism workflow and the project README for the model-surgery operations. Add the `pipeline_tensor_parallel` value to the `--node-role` help list so it matches the roles the server actually accepts.

Update the `#[ignore]`d prompt-cache integration tests (`prompt_cache_e2e`, `prompt_cache_prefill_bench`) to pass `--prompt-cache-enabled=true|false` using the now-required `=` syntax; their previous space-separated invocations would fail to parse under the tightened contract.
@inureyes inureyes added type:bug Bug fixes, error corrections, or issue resolutions priority:medium Medium priority area:cli Command-line interface / CLI flags labels May 22, 2026
@inureyes inureyes merged commit a074dfb into main May 22, 2026
4 checks passed
@inureyes inureyes deleted the fix/cli-flag-validation-help-cleanup branch May 22, 2026 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:cli Command-line interface / CLI flags priority:medium Medium priority type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant