Skip to content

fix(cli): show correct short help for sys, untruncate build/quantize - #1254

Merged
xieofxie merged 5 commits into
mainfrom
hualxie/fix_help
Jul 31, 2026
Merged

fix(cli): show correct short help for sys, untruncate build/quantize#1254
xieofxie merged 5 commits into
mainfrom
hualxie/fix_help

Conversation

@xieofxie

Copy link
Copy Markdown
Contributor

Problem

winml --help showed a garbage description for sys:

sys   Register ``dll_path`` in a fresh subprocess; yield the ``to_dict

_parse_click_help extracted the first decorated function's docstring from each command module. In sys.py the first decorated function is an internal @contextlib.contextmanager helper, not the @click.command, so its docstring leaked into the command listing — the very first screen a new user sees.

Two other rows (build, quantize) also truncated mid-sentence at an 80-column width.

Fix

  • cli.py_parse_click_help now only reads the function carrying a @click.command / @click.group decorator, and prefers an explicit short_help= decorator argument when present. Still AST-only (no module import), so startup stays fast. Added _command_decorator and _short_help_kwarg helpers.
  • build.py / quantize.py — added concise short_help= so their rows no longer truncate.

Result

build     Build a WinML-optimized ONNX model from HuggingFace or ONNX.
quantize  Quantize an ONNX model (QDQ, RTN weight-only, or FP16).
sys       Display system information for WinML CLI export.

Tests

Added TestParseClickHelp in tests/cli/test_help_cli.py:

  • sys regression test — asserts the real command docstring and that dll_path/subprocess never leak.
  • Skips a decorated non-Click helper preceding the command.
  • Prefers an explicit short_help= over the docstring.

All 55 tests in test_help_cli.py pass; ruff check clean.

_parse_click_help grabbed the first decorated function's docstring, so
'winml --help' showed an internal @contextlib.contextmanager docstring
for 'sys' instead of the command's. It now reads only the function
carrying a @click.command/@click.group decorator and honors an explicit
short_help= argument. Added short_help to build and quantize so their
rows no longer truncate at 80 columns.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@xieofxie
xieofxie requested a review from a team as a code owner July 30, 2026 07:04

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix — the root cause is clear (sys.py's @contextlib.contextmanager isolated_ep_register was the first decorated function, so its docstring leaked as the command's short help). The AST-based approach correctly targets only @click.command/@click.group. A couple of minor inline notes.

Comment thread src/winml/modelkit/cli.py
Comment thread src/winml/modelkit/cli.py
Comment thread src/winml/modelkit/commands/build.py
Address review feedback: clarify why we stop searching (return '') once
the Click command function is found even when it has no docstring or
short_help, rather than falling through to an unrelated helper's docstring.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix. The root cause was clear — _parse_click_help grabbed the first decorated function's docstring, which for sys.py was a @contextlib.contextmanager helper instead of the Click command. The new AST-based _command_decorator filter is precise and handles both @click.command and bare @command forms.

Explicit short_help= on build and quantize is the right call since their docstrings are multi-paragraph and Click's auto-truncation cut them off.

Tests are solid — regression test on actual sys.py, synthetic tests for decorator ordering and short_help= precedence.

Comment thread src/winml/modelkit/cli.py
Comment thread src/winml/modelkit/cli.py
Comment thread src/winml/modelkit/commands/build.py

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second pass — checked edge cases in AST parsing, decorator form variations, and multi-command modules. All solid.

Comment thread src/winml/modelkit/cli.py
Comment thread src/winml/modelkit/cli.py
Comment thread tests/cli/test_help_cli.py
Comment thread tests/cli/test_help_cli.py
Address review feedback: _command_decorator now accepts the attribute
form only when its owner is the 'click' name (plus bare @command/@group
from 'from click import ...'), so a non-Click decorator like
@typer.command can no longer be mistaken for a Click command. Add a
regression test covering the @typer.command case.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. AST-based Click command detection is correct and well-tested. All inline comments are non-blocking.

xieofxie and others added 2 commits July 31, 2026 11:19
Address review feedback: add an explicit test that a Click command with
no docstring and no short_help returns '' without falling through to a
later decorated helper, and document why ast.AsyncFunctionDef is skipped.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@xieofxie
xieofxie merged commit 16152fd into main Jul 31, 2026
9 checks passed
@xieofxie
xieofxie deleted the hualxie/fix_help branch July 31, 2026 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants