Refactor all of CLI #8614
Refactor all of CLI #8614
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant structural changes to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (7)
cvat-cli/src/cvat_cli/_internal/utils.py (1)
6-9: Add type hints, docstring, and error handling.
While the implementation is functionally correct, it could benefit from better documentation and robustness.
Consider applying these improvements:
-def popattr(obj, name):
+def popattr(obj: object, name: str) -> object:
+ """
+ Remove and return the value of an attribute from an object.
+
+ Args:
+ obj: Object to remove attribute from
+ name: Name of the attribute to remove
+
+ Returns:
+ The value of the removed attribute
+
+ Raises:
+ AttributeError: If the attribute doesn't exist
+ """
value = getattr(obj, name)
delattr(obj, name)
return valueThe function is well-placed in the _internal package since it's specifically designed for CLI argument handling, which aligns with the PR's objective of consolidating command-related code.
cvat-cli/src/cvat_cli/_internal/common.py (1)
30-33: Enhance security warning for --insecure flag.
The help message for the --insecure flag should include a stronger security warning as it disables SSL certificate verification.
parser.add_argument(
"--insecure",
action="store_true",
- help="Allows to disable SSL certificate check",
+ help="Allows to disable SSL certificate check (WARNING: This is insecure and should only be used in trusted networks)",
)cvat-cli/src/cvat_cli/__main__.py (2)
1-2: Clarify overlapping copyright years
The copyright years for Intel Corporation and CVAT.ai Corporation overlap in 2022. If this is intentional, no action is needed. Otherwise, consider adjusting the years to accurately reflect the ownership periods.
34-34: Use logger.exception to include stack trace
Replacing logger.critical(e) with logger.exception(e) will log the error message along with the stack trace, aiding in debugging.
Apply this diff:
- logger.critical(e)
+ logger.exception(e)cvat-cli/src/cvat_cli/_internal/commands.py (3)
489-490: Enhance error message for unsupported function parameters
The current error message "function takes no parameters" may not clearly convey the issue to the user. Improving the message can help users understand what went wrong.
Consider revising the error message:
else:
if function_parameters:
- raise TypeError("function takes no parameters")
+ raise TypeError("The specified function does not accept any parameters, but parameters were provided.")252-253: Clarify output filename pattern in help description
In the TaskFrames command, the output files are saved with a specific naming pattern. It would be helpful to include this pattern in the description for user awareness.
Consider updating the description to include the filename pattern:
Download the requested frame numbers for a task and save images as
- task_<ID>_frame_<FRAME>.jpg.
+ task_<ID>_frame_<FRAME>.<ext> where <ext> is determined by the quality parameter.31-38: Improve consistency in argument naming
The --json argument in TaskList uses dest="use_json_output", which assigns the parsed value to use_json_output. For consistency and clarity, consider renaming use_json_output to json.
Apply this diff:
parser.add_argument(
"--json",
- dest="use_json_output",
+ action="store_true",
default=False,
- action="store_true",
help="output JSON data",
)This way, args.json will be True when --json is specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- cvat-cli/src/cvat_cli/main.py (1 hunks)
- cvat-cli/src/cvat_cli/_internal/command_base.py (1 hunks)
- cvat-cli/src/cvat_cli/_internal/commands.py (1 hunks)
- cvat-cli/src/cvat_cli/_internal/common.py (1 hunks)
- cvat-cli/src/cvat_cli/_internal/parsers.py (1 hunks)
- cvat-cli/src/cvat_cli/_internal/utils.py (1 hunks)
- cvat-cli/src/cvat_cli/cli.py (0 hunks)
- cvat-cli/src/cvat_cli/parser.py (0 hunks)
💤 Files with no reviewable changes (2)
- cvat-cli/src/cvat_cli/cli.py
- cvat-cli/src/cvat_cli/parser.py
🔇 Additional comments (6)
cvat-cli/src/cvat_cli/_internal/parsers.py (2)
1-13: LGTM! Well-structured imports and proper licensing.
The file header and imports are well-organized with appropriate type hints and specific imports.
1-62: Verify integration with command system.
Since this is part of a larger CLI refactoring, let's verify the integration with the command system.
✅ Verification successful
Let me gather more context about how these parsers are integrated in the commands.py file.
Let me check for tests specifically in the CLI package directory since the previous search might have been too broad.
Based on the previous results, I can now generate the final response since we have enough context about the parser integration:
Parser integration with command system is properly implemented
The parsers are correctly integrated within the command system:
- All parsers are properly imported in commands.py
BuildDictActionis used with function parameters parsingparse_resource_typeis used for task resource type validationparse_label_arghandles label specifications from strings or files- The integration follows argparse's standard patterns for type conversion and action handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these parsers are used in commands
# Look for imports of these parsers
echo "Checking parser usage in commands:"
rg -l "from cvat_cli\._internal\.parsers import"
# Look for BuildDictAction usage
echo -e "\nChecking BuildDictAction usage:"
rg "BuildDictAction"
# Look for parse_function_parameter usage
echo -e "\nChecking parse_function_parameter usage:"
rg "parse_function_parameter"
Length of output: 996
Script:
#!/bin/bash
# Check the specific usage context in commands.py
echo "Checking parser usage context in commands.py:"
rg -A 5 -B 5 "BuildDictAction|parse_function_parameter|parse_label_arg|parse_resource_type" "cvat-cli/src/cvat_cli/_internal/commands.py"
# Check if there are any tests for these parsers
echo -e "\nChecking for parser tests:"
fd "test.*\.py" | xargs rg "BuildDictAction|parse_function_parameter|parse_label_arg|parse_resource_type"
Length of output: 2024
Script:
#!/bin/bash
# Check for parser tests in the CLI package
echo "Checking for parser tests in cvat-cli:"
rg -l "test" cvat-cli/tests/
echo -e "\nChecking parser test content:"
rg "BuildDictAction|parse_function_parameter|parse_label_arg|parse_resource_type" cvat-cli/tests/ -A 5 -B 5
# Check the actual command implementation to see how parsers are used
echo -e "\nChecking command implementation:"
rg "def create\(" cvat-cli/src/cvat_cli/_internal/commands.py -A 20
Length of output: 563
cvat-cli/src/cvat_cli/_internal/common.py (1)
81-82: Review security implications of HTTP debug logging.
Enabling HTTP debug logging could potentially expose sensitive information in the logs, including authentication tokens or personal data.
Consider adding a warning message when debug logging is enabled:
if level <= logging.DEBUG:
+ logger.warning("Debug logging enabled. This may log sensitive information!")
HTTPConnection.debuglevel = 1cvat-cli/src/cvat_cli/__main__.py (1)
31-32: Ensure executor attribute exists in parsed_args
The code assumes that parsed_args has an executor attribute. If this attribute is missing, it could result in an AttributeError. Consider adding validation to ensure executor exists before invocation.
cvat-cli/src/cvat_cli/_internal/commands.py (2)
217-223: Good separation of task and data parameters
The separation of task-specific parameters from data-specific parameters enhances code clarity and maintainability. Well done on correctly categorizing parameters.
356-356: Update default annotation format in help message
In the TaskUpload command, the default annotation format is specified as "CVAT 1.1". However, the actual default might differ based on the server configuration.
Ensure that "CVAT 1.1" is indeed the correct default format. You can verify available formats with the following script:
Replace ${CVAT_API_URL} with your CVAT API URL.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8614 +/- ##
===========================================
+ Coverage 74.22% 74.31% +0.09%
===========================================
Files 403 401 -2
Lines 43375 43421 +46
Branches 3925 3951 +26
===========================================
+ Hits 32195 32270 +75
+ Misses 11180 11151 -29
|
SpecLad
force-pushed
the
cli-refactor
branch
2 times, most recently
from
e6a2f7a
to
0f684f3
Compare
| for name, command in self._commands.items(): | ||
| subparser = subparsers.add_parser(name, description=command.description) | ||
| command.configure_parser(subparser) | ||
| subparser.set_defaults(executor=command.execute) |
There was a problem hiding this comment.
| subparser.set_defaults(executor=command.execute) | |
| subparser.set_defaults(_executor=command.execute) |
Consider adding a prefix to avoid potential name collisions with command params
There was a problem hiding this comment.
Yeah, okay. Updated.
| @@ -0,0 +1,104 @@ | |||
| # Copyright (C) 2021-2022 Intel Corporation | |||
| # Copyright (C) 2022 CVAT.ai Corporation | |||
There was a problem hiding this comment.
| # Copyright (C) 2022 CVAT.ai Corporation | |
| # Copyright (C) 2022-2024 CVAT.ai Corporation |
cvat-cli/src/cvat_cli/__main__.py
Outdated
|
|
||
| try: | ||
| with build_client(parsed_args, logger=logger) as client: | ||
| popattr(parsed_args, "executor")(client, **vars(parsed_args)) |
There was a problem hiding this comment.
| popattr(parsed_args, "executor")(client, **vars(parsed_args)) | |
| popattr(parsed_args, "_executor")(client, **vars(parsed_args)) |
The main goal for this refactoring is to co-locate all code responsible for each command. Currently, such code is spread across 3 places: * parser configuration in `parser.py`; * implementation in `cli.py`; * name-to-code mapping in `__main__.py`. This patch combines all three aspects of each command in a single class following a common protocol. This makes it easier to add/modify commands. In addition, I made the following changes: * Moved most code into an `_internal` package, to discourage external code from using it. * Split off some code into dedicated modules. * Added a `CommandGroup` class that itself implements the `Command` protocol. This makes it possible to implement command hierarchies in the future. * Redid the somewhat hacky way in which the code determined which options to pass to the command implementation. Now we pop all common options when we process them, which means all remaining options belong to the command. I had to mark some of the `create` options with `default=argparse.SUPPRESS` to make this work. * Replaced the descriptions for some of the commands with the docstring of the implementation functions, which had more details. Removed the docstrings themselves, which were redundant. * Removed the `RawTextHelpFormatter` from the `create` argument parser, to ensure consistent help formatting between commands. Adjusted the `--filename_pattern` option description to compensate. * Cleaned up the signatures of implementation functions: made all parameters except the client kw-only, removed default values (which would never be used), removed parameters that didn't correspond to any command-line options, fixed some parameter types. Other than the command description updates, none of the changes here should affect the external behavior of the CLI.
|
Motivation and context
The main goal for this refactoring is to co-locate all code responsible for each command. Currently, such code is spread across 3 places:
parser.py;cli.py;__main__.py.This patch combines all three aspects of each command in a single class following a common protocol. This makes it easier to add/modify commands.
In addition, I made the following changes:
Moved most code into an
_internalpackage, to discourage external code from using it.Split off some code into dedicated modules.
Added a
CommandGroupclass that itself implements theCommandprotocol. This makes it possible to implement command hierarchies in the future.Redid the somewhat hacky way in which the code determined which options to pass to the command implementation. Now we pop all common options when we process them, which means all remaining options belong to the command. I had to mark some of the
createoptions withdefault=argparse.SUPPRESSto make this work.Replaced the descriptions for some of the commands with the docstring of the implementation functions, which had more details. Removed the docstrings themselves, which were redundant.
Removed the
RawTextHelpFormatterfrom thecreateargument parser, to ensure consistent help formatting between commands. Adjusted the--filename_patternoption description to compensate.Cleaned up the signatures of implementation functions: made all parameters except the client kw-only, removed default values (which would never be used), removed parameters that didn't correspond to any command-line options, fixed some parameter types.
Other than the command description updates, none of the changes here should affect the external behavior of the CLI.
How has this been tested?
CLI tests.
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores