Skip to content

Fix boolean parsing for capture-onnx-graph options - #2647

Open
Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 3 commits into
microsoft:mainfrom
sylvesterkaczmarek:fix/capture-onnx-boolean-args
Open

Fix boolean parsing for capture-onnx-graph options#2647
Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 3 commits into
microsoft:mainfrom
sylvesterkaczmarek:fix/capture-onnx-boolean-args

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek Sylvester Kaczmarek (sylvesterkaczmarek) commented Sep 3, 2026

Copy link
Copy Markdown

Describe your changes

Fixes boolean argument parsing for capture-onnx-graph.

The --exclude_embeds, --exclude_lm_head, and --enable_cuda_graph options used type=bool. With argparse, values such as false are non-empty strings and therefore evaluate to True.

These options now use argparse's standard store_true flag semantics: absence leaves the option disabled (or None for enable_cuda_graph, preserving its existing tri-state default), while presence enables it.

Regression tests cover both default and enabled states.

Checklist before requesting a review

  • Add unit tests for this change.
  • Make sure focused validation passes (ruff and direct parser smoke test).
  • Update documents if necessary. No documentation change required.

Copilot AI lite review requested due to automatic review settings September 3, 2026 18:03
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change correctly addresses argparse boolean parsing semantics and is covered by targeted regression tests for valid and invalid inputs.

Pull request overview

This pull request fixes incorrect boolean parsing for the capture-onnx-graph CLI subcommand by replacing type=bool (which treats any non-empty string as True) with an explicit boolean parser, and adds regression tests to validate accepted/invalid values.

Changes:

  • Added a parse_bool argparse type that accepts common true/false string forms and rejects invalid values.
  • Updated --exclude_embeds, --exclude_lm_head, and --enable_cuda_graph to use type=parse_bool.
  • Added unit tests covering true/false variants and invalid input rejection.
File summaries
File Description
olive/cli/capture_onnx.py Introduces parse_bool and wires it into capture-onnx-graph boolean options to ensure correct CLI parsing.
test/cli/test_capture_onnx_args.py Adds regression tests verifying boolean parsing behavior and invalid value handling.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread olive/cli/capture_onnx.py
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Addressed the review: replaced the explicit parser with argparse store_true flags for all three options and updated the regression tests to cover absence/presence semantics. enable_cuda_graph keeps its existing None default so the current tri-state behavior is preserved. Ruff passes and a direct parser smoke test passes locally.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

shaahji The current head uses store_true for all three flags and preserves the None default for enable_cuda_graph. The requested change and regression updates are in place. Please re-review when available.

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.

3 participants