Make the CLI hierarchical #8787
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 changes introduced in this pull request enhance the command-line interface (CLI) and software development kit (SDK) for the CVAT project. New CLI commands for project management ( 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 (
|
SpecLad
force-pushed
the
rd/cli-hierarchy
branch
from
3a39ea2 to
029eedb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
cvat-cli/src/cvat_cli/_internal/command_base.py (2)
106-113: Enhance output for better user experience inGenericListCommandIn the
executemethod, when listing resources without JSON output, only the resource IDs are printed. Consider displaying additional information such as resource names or statuses to provide more meaningful output to the user.
125-126: Provide feedback after deletion inGenericDeleteCommandAfter deleting resources, consider providing feedback on which resources were successfully deleted and if any were not found. This enhances user awareness of the command's effect.
Apply this diff to add informative output:
def execute(self, client: Client, *, ids: Sequence[int]) -> None: self.repo(client).remove_by_ids(ids) + print(f"Deleted {self.resource_type_str}s with IDs: {' '.join(map(str, ids))}")cvat-cli/src/cvat_cli/_internal/commands_tasks.py (1)
389-391: Include filename in creation message for clarityIn
TaskCreateFromBackup, consider including the backup filename in the output message to inform the user which backup was used to create the task.Apply this diff to enhance the message:
def execute(self, client: Client, *, filename: str, status_check_period: int) -> None: task = client.tasks.create_from_backup( filename=filename, status_check_period=status_check_period, pbar=DeferredTqdmProgressReporter(), ) - print(f"Created task ID", task.id) + print(f"Created task ID {task.id} from backup '{filename}'")cvat-cli/src/cvat_cli/_internal/commands_projects.py (1)
36-46: Consider enhancing project creation optionsThe project creation command could benefit from additional commonly used options:
--assigneefor setting the project owner--organizationfor enterprise setups- Description field for project documentation
Would you like me to provide an implementation for these additional options?
cvat-cli/README.md (1)
9-12: Enhance project functionality documentationThe project functionality section should include more details about each operation and its options.
Add examples for each project operation:
- Projects: - Create a new project ```bash cvat-cli project create "my_project" --labels '[{"name": "car"}]' ``` - Delete projects ```bash cvat-cli project delete <project_id> ``` - List all projects ```bash cvat-cli project ls cvat-cli project ls --json # output in JSON format ```tests/python/cli/test_cli_misc.py (1)
84-94: Enhance test assertions and update command formatThe test assertions could be more descriptive and commands should use new format.
- personal_task_ids = list(map(int, self.run_cli("task", "ls", organization="").split())) + personal_task_ids = list(map(int, self.run_cli("tasks", "list", organization="").split())) assert personal_task_id in personal_task_ids - assert org_task_id not in personal_task_ids + assert org_task_id not in personal_task_ids, "Organization task should not be visible in personal context" - org_task_ids = list(map(int, self.run_cli("task", "ls", organization=org).split())) + org_task_ids = list(map(int, self.run_cli("tasks", "list", organization=org).split())) assert personal_task_id not in org_task_ids - assert org_task_id in org_task_ids + assert org_task_id in org_task_ids, "Organization task should be visible in org context" - all_task_ids = list(map(int, self.run_cli("task", "ls").split())) + all_task_ids = list(map(int, self.run_cli("tasks", "list").split())) assert personal_task_id in all_task_ids - assert org_task_id in all_task_ids + assert org_task_id in all_task_ids, "Both tasks should be visible without context"tests/python/cli/util.py (2)
92-92: Add class documentationAdd docstring to explain the purpose and usage of TestCliBase.
class TestCliBase: + """Base class for CLI tests providing common setup and utilities. + + This class provides: + - Automatic setup of test environment including DB and Redis + - Client configuration with admin credentials + - Utility method for running CLI commands with organization context + """
115-134: Enhance run_cli method with better error handling and documentationAdd docstring and improve error handling for the run_cli method.
def run_cli( self, cmd: str, *args: str, expected_code: int = 0, organization: Optional[str] = None ) -> str: + """Execute a CLI command with common arguments and organization context. + + Args: + cmd: The command to execute + args: Additional command arguments + expected_code: Expected return code (default: 0) + organization: Optional organization context + + Returns: + Command output as string + + Raises: + AssertionError: If command return code doesn't match expected_code + """ common_args = [ f"--auth={self.user}:{self.password}", f"--server-host={self.host}", f"--server-port={self.port}", ] if organization is not None: + if not isinstance(organization, str): + raise ValueError("Organization must be a string") common_args.append(f"--organization={organization}") run_cli( self, *common_args, cmd, *args, expected_code=expected_code, ) + output = self.stdout.getvalue() + if not output and expected_code == 0: + self._client.logger.warning("Command succeeded but produced no output") + return output - return self.stdout.getvalue()cvat-sdk/cvat_sdk/core/proxies/projects.py (1)
101-101: Document batch deletion capabilityAdd documentation for the new batch deletion functionality.
ModelListMixin[Project], ModelRetrieveMixin[Project], - ModelBatchDeleteMixin, + ModelBatchDeleteMixin, # type: ignore[type-arg] + # Enables batch deletion of projects using remove_by_ids method + # Example: client.projects.remove_by_ids([1, 2, 3])cvat-sdk/cvat_sdk/core/proxies/model_proxy.py (2)
166-185: Consider adding bulk deletion support for better performanceThe implementation correctly handles different response statuses and provides appropriate logging. However, making individual API calls for each ID might be inefficient for large sequences.
Consider adding support for bulk deletion if the API supports it, or implementing batching to reduce the number of API calls.
Example implementation with batching:
def remove_by_ids(self, ids: Sequence[int], /) -> None: """ Delete a list of objects from the server, ignoring those which don't exist. """ type_name = self._entity_type.__name__ + BATCH_SIZE = 100 # Adjust based on API limits - for object_id in ids: + # Convert to list to support multiple iterations if needed + id_list = list(ids) + + for i in range(0, len(id_list), BATCH_SIZE): + batch = id_list[i:i + BATCH_SIZE] + + # If bulk deletion is supported by the API: + # (_, response) = self.api.bulk_destroy(batch, _check_status=False) + # Otherwise, continue with individual deletions: + for object_id in batch: (_, response) = self.api.destroy(object_id, _check_status=False)
168-170: Enhance docstring with error handling detailsThe docstring should provide more information about error handling behavior, specifically:
- How different HTTP status codes are handled
- Whether the operation continues after encountering errors
- What logging to expect for different scenarios
Example enhancement:
""" Delete a list of objects from the server, ignoring those which don't exist. + +The method attempts to delete each object individually and: +- Logs success message for successful deletions (HTTP 2xx) +- Logs info message for non-existent objects (HTTP 404) +- Logs error message for other failures +- Continues processing remaining objects even after encountering errors """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
changelog.d/20241206_184906_roman_cli_hierarchy.md(1 hunks)cvat-cli/README.md(2 hunks)cvat-cli/src/cvat_cli/__main__.py(1 hunks)cvat-cli/src/cvat_cli/_internal/command_base.py(2 hunks)cvat-cli/src/cvat_cli/_internal/commands_all.py(1 hunks)cvat-cli/src/cvat_cli/_internal/commands_projects.py(1 hunks)cvat-cli/src/cvat_cli/_internal/commands_tasks.py(7 hunks)cvat-sdk/cvat_sdk/core/proxies/model_proxy.py(2 hunks)cvat-sdk/cvat_sdk/core/proxies/projects.py(2 hunks)cvat-sdk/cvat_sdk/core/proxies/tasks.py(3 hunks)site/content/en/docs/api_sdk/cli/_index.md(5 hunks)tests/python/cli/test_cli_misc.py(1 hunks)tests/python/cli/test_cli_projects.py(1 hunks)tests/python/cli/test_cli_tasks.py(13 hunks)tests/python/cli/util.py(2 hunks)
🧰 Additional context used
🪛 LanguageTool
cvat-cli/README.md
[uncategorized] ~3-~3: A comma might be missing here.
Context: ...interface for working with CVAT. At the moment it implements a basic feature set but m...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
site/content/en/docs/api_sdk/cli/_index.md
[style] ~10-~10: For conciseness, consider replacing this expression with an adverb.
Context: ...d line interface for working with CVAT. At the moment it implements a basic feature set but m...
(AT_THE_MOMENT)
[uncategorized] ~109-~109: You might be missing the article “the” here.
Context: ...ge "file1.jpg", as the current user, in organization "myorg": ```bash cvat-cli --org myo...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (16)
cvat-cli/src/cvat_cli/_internal/command_base.py (2)
61-81: Implementation of DeprecatedAlias class is appropriate
The DeprecatedAlias class effectively wraps deprecated commands and logs appropriate warnings, facilitating a smooth transition for users to the new command structure.
83-90: Abstract base class GenericCommand is well-defined
The abstract methods repo and resource_type_str ensure that all subclasses provide necessary implementations, promoting consistency across command classes.
cvat-cli/src/cvat_cli/_internal/commands_tasks.py (1)
33-37: Refactoring with generic base classes enhances code maintainability
The introduction of GenericTaskCommand and refactoring TaskList and TaskDelete to inherit from generic base classes improve code reuse and simplify maintenance.
changelog.d/20241206_184906_roman_cli_hierarchy.md (1)
1-20: Changelog entries are accurate and well-structured
The changelog effectively summarizes the additions, changes, and deprecations introduced in this pull request, providing clear information to users and maintainers.
cvat-cli/src/cvat_cli/_internal/commands_all.py (1)
1-27: New command mapping supports CLI hierarchy and backward compatibility
The commands_all.py module consolidates command definitions and correctly maps legacy commands to the new hierarchical structure using DeprecatedAlias, ensuring backward compatibility and a smooth transition for users.
cvat-cli/src/cvat_cli/__main__.py (1)
13-13: Updated import to reflect the new command structure
The import statement now references commands_all.py, aligning the main execution script with the updated command hierarchy.
tests/python/cli/test_cli_projects.py (1)
50-54: Add verification for project deletion
The test should verify that all associated resources (tasks, annotations, etc.) are properly cleaned up after project deletion.
cvat-sdk/cvat_sdk/core/proxies/projects.py (1)
18-18: Verify batch deletion implementation
Ensure the batch deletion functionality is properly implemented in the mixin.
✅ Verification successful
Batch deletion is properly implemented
The ModelBatchDeleteMixin class in cvat-sdk/cvat_sdk/core/proxies/model_proxy.py has a well-implemented remove_by_ids method that:
- Takes a sequence of IDs as input
- Handles deletion of multiple objects
- Properly handles non-existent objects
- Is being used in production code (evidenced by usage in tasks.py and CLI)
- Has test coverage (as seen in test_tasks.py)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if batch deletion is implemented in the mixin
ast-grep --pattern 'class ModelBatchDeleteMixin {
$$$
remove_by_ids($$$) {
$$$
}
}'
Length of output: 95
Script:
#!/bin/bash
# Let's try to find the ModelBatchDeleteMixin class definition
rg "class ModelBatchDeleteMixin" -A 10
# Also search for any file containing ModelBatchDeleteMixin
fd ModelBatchDeleteMixin
# Let's also check for any batch deletion related methods
rg "remove_by_ids|batch_delete" -A 5
Length of output: 3256
tests/python/cli/test_cli_tasks.py (2)
284-290: LGTM: Legacy alias test implementation is correct.
The test properly verifies:
- Backward compatibility with old command format
- Deprecation warning presence
- New command format suggestion in the warning
135-136: Verify command name changes across the codebase.
The commands have been renamed for consistency:
export→export-datasetupload→import-datasetdump→backup
Let's verify that all usages have been updated.
Also applies to: 152-153, 182-189
✅ Verification successful
Command name changes have been properly updated across the codebase
Based on the verification results:
- The old command names (
export,upload,dump) are not found in any active code - The new command names (
export-dataset,import-dataset,backup) are correctly used in the test files - Documentation in
site/content/en/docs/api_sdk/cli/_index.mdhas been updated with the new command format - The changelog entries confirm this is part of a broader CLI restructuring where commands now follow the format
cvat-cli <resource> <action>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for old command names that should have been updated
echo "Searching for old command names..."
rg -l 'cvat-cli\s+(export|upload|dump)\s' --type md --type python
# Search for new command names to verify adoption
echo "Verifying new command names..."
rg -l 'cvat-cli\s+task\s+(export-dataset|import-dataset|backup)\s' --type md --type python
Length of output: 372
Script:
#!/bin/bash
# Search for old command names that should have been updated
echo "Searching for old command names..."
rg -l 'cvat-cli\s+(export|upload|dump)\b'
# Search for new command names to verify adoption
echo "Verifying new command names..."
rg -l 'cvat-cli\s+task\s+(export-dataset|import-dataset|backup)\b'
# Also search for these commands without cvat-cli prefix
echo "Searching for command patterns without cvat-cli prefix..."
rg '\b(export|upload|dump)\b' 'tests/python/cli/test_cli_tasks.py' -A 2 -B 2
rg '\b(export-dataset|import-dataset|backup)\b' 'tests/python/cli/test_cli_tasks.py' -A 2 -B 2
Length of output: 1574
Script:
#!/bin/bash
# Let's check the documentation file found earlier and other potential docs
echo "Checking documentation content..."
rg '\b(export|upload|dump)\b' 'site/content/en/docs/api_sdk/cli/_index.md' -A 2 -B 2
# Let's also check for any other potential test files or CLI-related files
echo "Checking for other CLI-related files..."
fd -e py -e md cli
Length of output: 926
Script:
#!/bin/bash
# Let's check the remaining CLI-related files for any old command names
echo "Checking other CLI test files..."
rg '\b(export|upload|dump)\b' 'tests/python/cli/test_cli_misc.py' 'tests/python/cli/test_cli_projects.py' -A 2 -B 2
# Let's also check the changelog files for context about these changes
echo "Checking changelog entries..."
cat changelog.d/20241206_135902_roman_cli_logging.md changelog.d/20241206_184906_roman_cli_hierarchy.md
Length of output: 1138
site/content/en/docs/api_sdk/cli/_index.md (3)
46-55: LGTM: Clear and well-structured command format documentation.
The command structure is well explained with:
- Clear format specification
- Detailed breakdown of each component
- Logical organization of information
65-68: LGTM: Clear deprecation notice for legacy aliases.
The documentation properly:
- Acknowledges backward compatibility
- Marks old syntax as deprecated
- Directs users to the new format
284-313: LGTM: Well-documented project commands.
The new project commands section:
- Maintains consistency with task command documentation
- Provides clear examples
- Covers all basic operations (create, delete, list)
cvat-sdk/cvat_sdk/core/proxies/tasks.py (2)
338-347: LGTM: Well-implemented backward compatibility.
The wrapper properly:
- Maintains compatibility with keyword arguments
- Documents the compatibility purpose
- Delegates to the generic implementation
290-290: LGTM: Clean mixin integration.
The ModelBatchDeleteMixin is properly integrated into the TasksRepo class hierarchy.
cvat-sdk/cvat_sdk/core/proxies/model_proxy.py (1)
9-9: LGTM! Proper use of type hints
The addition of Sequence from collections.abc follows Python type hinting best practices and is correctly placed with other collection imports.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8787 +/- ##
===========================================
- Coverage 73.92% 73.89% -0.03%
===========================================
Files 408 408
Lines 43884 43889 +5
Branches 3986 3986
===========================================
- Hits 32441 32433 -8
- Misses 11443 11456 +13
|
SpecLad
force-pushed
the
rd/cli-hierarchy
branch
from
d96eb18 to
1a7dbda
Compare
SpecLad
force-pushed
the
rd/cli-hierarchy
branch
from
1a7dbda to
8cc57f2
Compare
For task functionality and everything else.
SpecLad
force-pushed
the
rd/cli-hierarchy
branch
from
dea13c8 to
2118472
Compare
|
Motivation and context
Currently, the CLI has one level of subcommands, and all subcommands work on tasks. This leaves no room for subcommands that work on other CVAT resources.
This change redesigns the CLI interface by adding another level of subcommand hierarchy. Instead of running
cvat-cli <action>, the user will now runcvat-cli <resource> <action>. Previously available commands are left available as deprecated aliases.As a proof of concept, this PR adds some basic project actions.
I have also used this opportunity to correct some of the task action names, specifically
export,import,dumpandupload. These names don't correspond to either SDK function names, API endpoints, or UI labels corresponding to these actions. In the new subcommand hierarchy, I renamed those commands tobackup,create-from-backup,export-datasetandimport-dataset, which are more consistent with how other CVAT components call these actions.I rewrote the introduction and usage sections of the cli README and reference in order to reduce clutter and remove the need to resynchronize the help output after every interface change.
How has this been tested?
CLI tests.
Checklist
developbranch[ ] 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
project create,project delete,project ls.Documentation
Bug Fixes
Tests