CLI: Add ability to create CVAT agents based on AA functions #8821
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 in this pull request introduce new functionalities to the CVAT command-line interface (CLI), particularly for managing native functions in Enterprise/Cloud environments. Key additions include new subcommands for creating, deleting, and running agents for these functions. The documentation has been updated to reflect these changes, and several internal modifications enhance error handling and function loading processes. Dependency specifications have also been refined to ensure compatibility and security, while new classes and methods have been introduced to facilitate efficient handling of annotation requests. 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
cli-agent
branch
5 times, most recently
from
cdf9320 to
188b916
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 (7)
changelog.d/20241212_193004_roman_cli_agent.md (1)
3-3: Remove unnecessary escape characters in markdownThe backslashes before the square brackets are unnecessary in Markdown. You can write
[CLI]without escaping the brackets.Apply this diff to clean up the markdown:
- - \[CLI\] Added commands for working with native functions + - [CLI] Added commands for working with native functionscvat-cli/requirements/base.txt (1)
1-1: Avoid pinning 'cvat-sdk' to an exact versionChanging the
cvat-sdkdependency from~=2.23.2to==2.23.2pins it to an exact version. This may prevent compatibility with future patch releases and can lead to dependency resolution issues. Consider using a compatible version specifier to allow for minor updates.Apply this diff to use a compatible version specifier:
- cvat-sdk==2.23.2 + cvat-sdk~=2.23.2cvat-cli/README.md (1)
25-29: Documentation enhancement neededWhile the new commands are well-documented in terms of their basic purpose, consider adding:
- Brief usage examples for each command
- Required permissions or prerequisites
- Expected output formats
Example addition:
- Functions (Enterprise/Cloud only): + Functions (Enterprise/Cloud only): + - `create-native` - create a function that can be powered by an agent + Example: cvat-cli function create-native --name "detection" --model "/path/to/model" + - `delete` - delete a function + Example: cvat-cli function delete <function_id> + - `run-agent` - process requests for a native function + Example: cvat-cli function run-agent <function_id> + + Note: These commands require Enterprise/Cloud access and appropriate permissions.site/content/en/docs/api_sdk/cli/_index.md (2)
32-35: Enhance command descriptions with parameter detailsWhile the commands are clearly listed, consider enhancing the descriptions with:
- Required and optional parameters for each command
- Expected input/output formats
- Any prerequisites or dependencies
Example enhancement:
- create-native - create a function that can be powered by an agent + create-native - create a function that can be powered by an agent + Required: function name, function module + Optional: function parameters (-p key=type:value)
327-328: Enhance Enterprise/Cloud notice visibilityConsider making the Enterprise/Cloud requirement more prominent and adding helpful links:
- **Note**: The functionality described in this section can only be used - with the CVAT Enterprise or CVAT Cloud. + > **Important**: Enterprise/Cloud Feature + > The functionality described in this section requires CVAT Enterprise or CVAT Cloud. + > For more information, see [Enterprise Features](link-to-enterprise-docs) or + > [Cloud Features](link-to-cloud-docs).cvat-sdk/cvat_sdk/auto_annotation/driver.py (2)
Line range hint
267-298: Consider Adding Parameter and Exception Documentation in the Docstring ofannotate_taskWhile the
annotate_taskfunction has a comprehensive docstring describing its purpose and behavior, it's recommended to include detailed documentation for each parameter and any exceptions that the function may raise, such asBadFunctionError. This will enhance clarity and help other developers understand how to use the function effectively.
26-53: Recommend Adding Unit Tests for New Mapping ClassesThe newly introduced classes
_SublabelNameMapping,_LabelNameMapping, and_SpecNameMappingare critical for label and sublabel mapping functionality. Adding unit tests for these classes would ensure their correctness and prevent future regressions.Would you like assistance in creating unit tests for these classes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
changelog.d/20241212_193004_roman_cli_agent.md(1 hunks)cvat-cli/README.md(1 hunks)cvat-cli/requirements/base.txt(1 hunks)cvat-cli/src/cvat_cli/__main__.py(2 hunks)cvat-cli/src/cvat_cli/_internal/agent.py(1 hunks)cvat-cli/src/cvat_cli/_internal/commands_all.py(1 hunks)cvat-cli/src/cvat_cli/_internal/commands_functions.py(1 hunks)cvat-cli/src/cvat_cli/_internal/commands_tasks.py(4 hunks)cvat-cli/src/cvat_cli/_internal/common.py(2 hunks)cvat-sdk/cvat_sdk/auto_annotation/driver.py(7 hunks)dev/update_version.py(1 hunks)site/content/en/docs/api_sdk/cli/_index.md(2 hunks)
🔇 Additional comments (5)
cvat-cli/src/cvat_cli/_internal/commands_all.py (1)
6-6: Integration of new 'function' command is well-structured
The addition of the function command integrates seamlessly with the existing command group. This enhances the CLI's functionality while maintaining consistency.
Also applies to: 12-12
cvat-cli/src/cvat_cli/__main__.py (2)
14-19: LGTM: Clean import organization
The addition of CriticalError to the imports is well-organized and follows the existing import structure.
37-37: Verify error propagation from agent operations
The addition of CriticalError to exception handling is appropriate. However, since this is part of new agent functionality, we should ensure proper error propagation.
✅ Verification successful
Error propagation for agent operations is properly implemented
The error handling implementation is correct and comprehensive:
CriticalErroris properly defined incommon.pyas a custom exception for critical failures- Agent implementation in
agent.pyraisesCriticalErrorin appropriate scenarios:- Invalid function provider
- Unsupported function types
- Label incompatibilities
- Missing labels
- Attribute validation failures
- Main error handler in
__main__.pycorrectly catches and processesCriticalErroralong with other network-related exceptions - Error messages are properly logged with detailed context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper error handling in agent-related code
# Look for error raising patterns in agent implementation
rg -p "raise CriticalError" --type python
rg -p "except.*CriticalError" --type python
Length of output: 151
Script:
#!/bin/bash
# Search for CriticalError usage patterns without file type restriction
rg -p "raise CriticalError"
rg -p "except.*CriticalError"
# Search for files that might contain CriticalError definition
rg -p "class CriticalError"
Length of output: 568
Script:
#!/bin/bash
# Let's check the implementation of CriticalError and its context
cat cvat-cli/src/cvat_cli/_internal/common.py
# Check the agent implementation to understand error scenarios
cat cvat-cli/src/cvat_cli/_internal/agent.py
# Check main error handling implementation
cat cvat-cli/src/cvat_cli/__main__.py
Length of output: 19559
dev/update_version.py (1)
162-163: Consider version constraint implications
The change from flexible (~=) to strict (==) version matching for cvat-sdk is significant:
- Pros: Ensures exact version compatibility, prevents unexpected updates
- Cons: May make maintenance more challenging, requires manual updates for patches
Consider if this strict matching is necessary, or if semantic versioning with ~= would be more appropriate for maintenance.
✅ Verification successful
Strict version pinning for cvat-sdk is appropriate
Based on the codebase analysis:
- The cvat-sdk is a core dependency used in critical components (CLI and main SDK)
- The version update script is part of the release management tooling
- Strict version pinning (==) helps ensure consistent behavior across installations and prevents compatibility issues
- This aligns with best practices for internal dependencies where version compatibility is crucial
The change from flexible to strict versioning is a positive architectural decision that enhances reliability and predictability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version constraints across the project
# Look for other SDK version references that might need alignment
# Check for other version constraints
rg -p "cvat-sdk[=~><]" --type txt
rg -p "cvat-sdk.*version" --type python
Length of output: 158
Script:
#!/bin/bash
# Let's check for any Python files containing cvat-sdk references
rg -p "cvat-sdk" -g "*.py"
# Also check requirements files and setup configurations
fd "requirements.*\.txt|setup.*\.py|pyproject.toml" --exec cat {}
Length of output: 5193
Script:
#!/bin/bash
# Let's check the git history for this specific change to understand the context
git log -p dev/update_version.py | grep -A 5 -B 5 "cvat-sdk"
# Also check the specific requirements file being modified
fd "cvat-cli/requirements/base.txt" --exec cat {}
Length of output: 51
cvat-sdk/cvat_sdk/auto_annotation/driver.py (1)
64-71: Verify Handling of None Values Returned by _build_label_id_mapping
In the _build_label_id_mapping method, when spec_nm.map_label(fun_label.name) returns None, the method also returns None. Please verify that all callers of _build_label_id_mapping properly handle None values to prevent potential TypeError exceptions due to unexpected None references.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8821 +/- ##
===========================================
- Coverage 73.93% 73.87% -0.06%
===========================================
Files 409 408 -1
Lines 43930 44109 +179
Branches 3986 3986
===========================================
+ Hits 32478 32587 +109
- Misses 11452 11522 +70
|
I made it depend on some internal SDK functions, so I want to make sure that the versions match exactly.
| help="parameter for the function", | ||
| ) | ||
|
|
||
| original_executor = parser.get_default("_executor") |
There was a problem hiding this comment.
Consider adding a named constant for this string.
There was a problem hiding this comment.
I'm not sure if that's needed here. It is basically a secret command-line argument, and we don't usually create constants for those.
| ``` | ||
| cvat-cli function create-native "Faster R-CNN" \ | ||
| --function-module cvat_sdk.auto_annotation.functions.torchvision_detection \ | ||
| -p model_name=str:fasterrcnn_resnet50_fpn_v2 |
There was a problem hiding this comment.
Consider passing arguments as strings by default, this may reduce the number of required type specifications for model parameters.
There was a problem hiding this comment.
This isn't directly related to this PR, because I'm just reusing the -p argument that was implemented for the task auto-annotate command.
But for the record, I don't want to do it that way, because it creates opportunities for misinterpretation. E.g. if a shell script runs a command with -p "$x", intending it to be a string parameter, it may unexpectedly be parsed as a different type instead (depending on the value).
| _, response = client.api_client.call_api( | ||
| "/api/functions", | ||
| "POST", | ||
| body=remote_function, | ||
| ) |
There was a problem hiding this comment.
Do you think if SDK-level functions for the functions API could be useful?
There was a problem hiding this comment.
In principle, yes, but I'm not going to implement that in this PR. For now, I consider everything here to be a CLI-only feature.
Ideally, I'd prefer to add some stubs for the enterprise function API to the server, so that the normal SDK auto-generation can work. But that'll have to be another time.
| def _validate_function_compatibility(self, remote_function: dict) -> None: | ||
| function_id = remote_function["id"] | ||
|
|
||
| if remote_function["provider"] != "native": |
There was a problem hiding this comment.
Consider adding a named constant for native.
There was a problem hiding this comment.
Yeah, good point. Added.
| f"Function #{remote_function['id']} is incompatible with function object: " | ||
| ) | ||
|
|
||
| if remote_function["kind"] != "detector": |
There was a problem hiding this comment.
Consider adding a named constant or enum for detector.
| raise | ||
|
|
||
| self._client.logger.error("Acquire request failed; will retry", exc_info=True) | ||
| time.sleep(_POLLING_INTERVAL.total_seconds()) |
There was a problem hiding this comment.
Consider randomizing the delay to make server load more even in the case of multiple workers launched, if such scenario is expected to be frequent.
There was a problem hiding this comment.
Good idea. Implemented.
|
Motivation and context
How has this been tested?
Checklist
developbranch[ ] 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
New Features
create-native,delete, andrun-agent.Bug Fixes
CriticalErrorexception to the main function.Chores