lablup / lablup/backend.ai

Accept `--output` on subcommands, not only at the root group

Open
#13,367 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
670
Forks
183
Avg merge
17h 7m
Merged PRs (30d)
358

Description

### Main idea

Let `--output` be accepted on the subcommand that actually produces the output, not only on the root group.

```console
$ ./backend.ai --output=json service list # works
$ ./backend.ai service list --output=json # Error: No such option: --output
```

The root-level-only form is not a bug — it is the design decided in #240 ("using a root-level option `--output=json`") and it is exactly how Click resolves options: an option is parsed only at the position of the group that declares it. But the trailing form is the better default UX, and today it is not merely unsupported — it is undiscoverable and inconsistent.

**Why the trailing form is more appropriate**

| | root-level only (today) | accepted on the leaf command |
|---|---|---|
| `backend.ai service list -h` shows the flag | ❌ never listed | ✅ |
| Appending to a recalled shell-history line | ❌ must edit mid-line | ✅ |
| Matches peer CLIs | — | `kubectl get pods -o json`, `gh pr list --json`, `docker ps --format`, `aws s3api list-buckets --output json` |
| Flag sits next to the thing it formats | ❌ | ✅ |
| One declaration site | ✅ | ✅ if injected by the group class (see below) |

clig.dev — the guide #240 itself cites — puts the discoverability of a command's own flags ahead of parser purity. A formatting option that cannot be seen or typed where the output is produced fails that test.

This is not hypothetical churn: the design has already broken down once. `session watch` re-declares its own leaf-level `--output` with the identical `Choice(["json", "console"])` (`src/ai/backend/client/cli/session/lifecycle.py:1381`) and hand-rolls `if output == "json"` branches instead of going through the output framework, because the root-level flag was not reachable where it was needed.

**Proposal**

Accept `--output` in both positions; the leaf value wins over the root value. Do not remove the root-level option — it is the correct place for a setting that applies to a whole invocation, and removing it breaks every existing script.

Implement it once, in `ExtendedCommandGroup` (`src/ai/backend/cli/extensions.py`), so no per-command decorator churn across the ~140 commands and 273 `ctx.output.*` call sites:

- when resolving a subcommand, recursively append the `--output` param to leaf commands and merge the parsed value into `CliContextInfo.info`;
- **skip any command that already declares an `--output` / `-o` param** — `admin export {users,sessions,...}` uses `-o/--output ` for the destination file in both v1 (`src/ai/backend/client/cli/admin/export.py`) and v2 (`src/ai/backend/client/cli/v2/admin/export.py`). A blanket injection would silently break them;
- the recursion has to reach through plain `click.Group` subgroups too — `service`, `vfolder`, etc. are declared with a bare `@click.group()`, not `ExtendedCommandGroup`.

Follow-ups this unblocks:
- delete the duplicated leaf-level `--output` in `session watch` and route it through the output framework (part of #1925);
- `src/ai/backend/client/cli/main.py:18-57` currently declares a second `--output` and builds a `CLIContext` that never runs — the client group is loaded under the `_` entrypoint key, which copies only `.commands` and discards the group callback. Worth deleting alongside, together with the now-dead `case CLIContext()` branch in `pass_ctx_obj` (`src/ai/backend/client/cli/extensions.py:46`).

### Alternative ideas

- **Name the leaf option `--format` instead**, leaving `--output` root-only. Sidesteps the `admin export -o PATH` collision entirely and reads correctly (`--format json`), at the cost of two names for one concept.
- **Hoist a trailing `--output` in `argv` before Click parses.** ~5 lines, but it would rewrite `admin export users --output report.csv` into a format flag. Rejected.
- **Do nothing, and document it.** The root-level form does work and is stable; the cost is the recurring "why doesn't this work" and copy-pasted leaf-level re-declarations like `session watch`.

### Anything else?

Not applicable to the v2 CLI (`./bai`) — v2 commands always emit JSON through `print_result()` and expose no `--output` at all. If v2 ever grows a console renderer, whatever is decided here should be the shape it adopts.

Related: #240 (root-level `--output` decision), #1925 (consistent use of the output framework).

Contributor guide

Open the contributing guide

Research direction

Start in src/ai/backend/client/cli/extensions.py at ExtendedCommandGroup, then inspect the plain click.group() subgroups such as service and vfolder. Review the existing --output declarations in session/lifecycle.py and both admin/export.py files before tracing main.py. Done means --output works at the root and leaf positions, the leaf value wins, and existing destination-file options remain unaffected.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
cli
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.