[python] Migrate CLI from argparse to click #86
Conversation
Summary: Migrate CLI implementation from argparse to click for better developer experience and cleaner code structure. Changes: - Replace argparse with click decorators in cutracer/cli.py - Convert validation/cli.py to use click.command and click.option - Update scripts/parse_instr_hist_trace.py to use click - Add click>=8.0.0 dependency to pyproject.toml Benefits of click over argparse: - Decorator-based syntax is more readable - Built-in support for command groups - Better error messages and help formatting - Easier to compose and reuse options Test Plan: python3 -m py_compile python/cutracer/cli.py python/cutracer/validation/cli.py scripts/parse_instr_hist_trace.py
meta-cla
bot
added
the
CLA Signed
FindHao
changed the title
There was a problem hiding this comment.
Pull request overview
This PR migrates the CLI implementation from argparse to click across three Python files, introducing a more modern decorator-based syntax. However, the migration introduces critical breaking changes to the existing test suite that must be addressed before merging.
Key Changes:
- Replaced argparse ArgumentParser with click decorators and command groups
- Updated validation CLI to use click.command, click.option, and click.argument decorators
- Converted parse_instr_hist_trace.py script to use click decorators
- Added click>=8.0.0 as a dependency
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| python/pyproject.toml | Added click>=8.0.0 dependency |
| python/cutracer/cli.py | Converted main entry point to use click.group decorator with version option; removed argparse parser setup |
| python/cutracer/validation/cli.py | Migrated validate command from argparse to click decorators; changed from returning exit codes to calling sys.exit() directly |
| scripts/parse_instr_hist_trace.py | Converted script to use click.command decorator with click.option for all arguments; updated error handling to use click.UsageError |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def validate_command( | ||
| file: Path, | ||
| file_format: str, | ||
| quiet: bool, | ||
| json_output: bool, | ||
| verbose: bool, | ||
| ) -> None: |
There was a problem hiding this comment.
The return type annotation has changed from int to None, but the function now calls sys.exit() directly instead of returning exit codes. This is a breaking change for the existing tests in python/tests/test_cli.py which expect the function to return an integer exit code. The tests call exit_code = main() and then assert on the returned value. With the current implementation using sys.exit(), these tests will fail. Consider using click's standalone_mode=False to preserve the return value behavior for testing compatibility.
| "--cutracer-trace", | ||
| "cutracer_trace_input", | ||
| type=click.Path(exists=True), | ||
| help="Path to the CUTRICER histogram CSV file.", |
There was a problem hiding this comment.
The help text contains "CUTRICER" which should be "CUTRACER" to match the correct project name used throughout the codebase.
| kernel_hash_hex, | ||
| output, | ||
| ): | ||
| """Parse and merge trace files from Chrome's tracer and CUTRICER. |
There was a problem hiding this comment.
The docstring contains "CUTRICER" which should be "CUTRACER" to match the correct project name used throughout the codebase.
| args = parser.parse_args() | ||
| \b | ||
| 1. Merge mode (requires --cutracer-log, --chrome-trace, and --cutracer-trace): | ||
| Merges Chrome trace, CUTRICER histogram, and log data. |
There was a problem hiding this comment.
The docstring contains "CUTRICER" which should be "CUTRACER" to match the correct project name used throughout the codebase.
| 3. CUTRICER histogram only (--cutracer-trace): | ||
| Parses a CUTRICER histogram CSV file. |
There was a problem hiding this comment.
The docstring contains "CUTRICER" which should be "CUTRACER" to match the correct project name used throughout the codebase.
| print(f"Successfully parsed Chrome trace and saved to {args.output}") | ||
| elif args.cutracer_trace_input: | ||
| df.to_csv(output, index=False) | ||
| print(f"Successfully parsed Chrome trace and saved to {output}") |
There was a problem hiding this comment.
For consistency with click best practices and the validation CLI (python/cutracer/validation/cli.py:57-69), consider using click.echo() instead of print() for output. Click's echo function provides better cross-platform compatibility and integrates better with click's testing utilities.
| "--cutracer-log", | ||
| "cutracer_log_input", | ||
| type=click.Path(exists=True), | ||
| help="Path to the CUTRICER log file to enable merge mode.", |
There was a problem hiding this comment.
The help text contains "CUTRICER" which should be "CUTRACER" to match the correct project name used throughout the codebase.
| df.to_csv(args.output, index=False) | ||
| print(f"Successfully parsed CUTRICER histogram and saved to {args.output}") | ||
| df.to_csv(output, index=False) | ||
| print(f"Successfully parsed CUTRICER histogram and saved to {output}") |
There was a problem hiding this comment.
For consistency with click best practices and the validation CLI (python/cutracer/validation/cli.py:57-69), consider using click.echo() instead of print() for output. Click's echo function provides better cross-platform compatibility and integrates better with click's testing utilities.
| @click.group(epilog=EXAMPLES) | ||
| @click.version_option(version=_get_package_version(), prog_name="cutraceross") | ||
| def main() -> None: | ||
| """CUTracer: CUDA trace validation and analysis tools.""" |
There was a problem hiding this comment.
The return type annotation has changed from int to None, but the existing tests in python/tests/test_cli.py expect main() to return an integer exit code. Tests like test_validate_valid_json() call exit_code = main() and assert on the value. With click, the function returns None and uses sys.exit() internally. This will break all existing CLI tests. Consider either updating the tests to use assertRaises(SystemExit) or making click's standalone_mode=False to preserve the return value behavior for testing.
FindHao
force-pushed
the
argparse-to-click
branch
from
e89be62 to
0b02f1d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FILE is the path to the trace file to validate. | ||
| """ | ||
| file_path = file |
There was a problem hiding this comment.
The variable assignment file_path = file is unnecessary since the parameter file is already a Path object and could be used directly throughout the function. This adds no value and makes the code slightly less clear.
| import shutil | ||
|
|
||
| shutil.copy(self.test_dir / "reg_trace_sample.ndjson", unknown_file) | ||
|
|
There was a problem hiding this comment.
The import statement for shutil is placed inside the conditional block. It would be better to move this import to the top of the file with other imports for consistency and clarity, or use CliRunner's isolated_filesystem() context manager for creating temporary test files.
Summary
This PR replaces
argparsewithclickfor all CLI implementations in the cutracer package, providing a cleaner and more maintainable command-line interface.Changes
python/pyproject.toml: Addclick>=8.0.0dependencypython/cutracer/cli.py: Convert main CLI entry point to use@click.group()and@click.version_option()python/cutracer/validation/cli.py: Replace_add_validate_args()function with click decorators (@click.command(),@click.option(),@click.argument())scripts/parse_instr_hist_trace.py: Migrate standalone script to click-based CLIWhy click?
add_argument()callsadd_subparsers()setup@group.command()Testing
Usage (unchanged)