dataset_manager: remove imports that were deprecated in Python 3.9 #8745
Conversation
This is a continuation of cvat-ai#8626.
WalkthroughThe changes in this pull request primarily involve updates to import statements and type annotations across multiple files within 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: 2
🧹 Outside diff range and nitpick comments (2)
cvat/apps/dataset_manager/util.py (1)
Line range hint
94-98: Add parameter documentation for improved clarityWhile the type hints are correct, consider adding docstring documentation to describe the parameters and their purpose:
@contextmanager def get_export_cache_lock( export_path: os.PathLike[str], *, ttl: int | timedelta, block: bool = True, acquire_timeout: Optional[int | timedelta] = None, -) -> Generator[Lock, Any, Any]: +) -> Generator[Lock, Any, Any]: + """Acquire a lock for export cache operations. + + Args: + export_path: Path to the export file + ttl: Time-to-live for the lock in seconds or as timedelta + block: Whether to block waiting for the lock + acquire_timeout: Maximum time to wait for lock acquisition + + Returns: + Generator yielding the acquired lock + + Raises: + LockNotAvailableError: If the lock cannot be acquired + ValueError: If ttl or acquire_timeout are invalid + """cvat/apps/dataset_manager/project.py (1)
113-113: LGTM! Consider documenting type hint changesThe type hint updates follow PEP 585 by using built-in collection types. This is a good modernization that improves code readability.
Consider adding a note in the changelog about the type hint modernization across the codebase to help other contributors follow the same pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cvat/apps/dataset_manager/annotation.py(1 hunks)cvat/apps/dataset_manager/bindings.py(21 hunks)cvat/apps/dataset_manager/project.py(2 hunks)cvat/apps/dataset_manager/tests/utils.py(2 hunks)cvat/apps/dataset_manager/util.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cvat/apps/dataset_manager/annotation.py
🔇 Additional comments (17)
cvat/apps/dataset_manager/tests/utils.py (2)
9-9: LGTM! Import is correctly maintained
The Optional import from typing is still valid in Python 3.9 and is necessary for the type hints used in the code.
26-29: LGTM! Type hint modernization is correct
The change from Type[BaseException] to type[BaseException] aligns with PEP 585's built-in generics support in Python 3.9+.
Let's verify consistency of this type hint usage across the codebase:
✅ Verification successful
Type hint change is isolated and correctly implemented
The search results confirm that:
- There are no remaining instances of the old
Type[BaseException]pattern in the codebase - The new
type[BaseException]pattern is only used in the modified file - No other
__exit__method signatures withBaseExceptionwere found that would need similar updates
This indicates the change is an isolated improvement in the test utilities and doesn't require modifications elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of Type[BaseException] that might need updating
# and verify the consistency of the new type[BaseException] pattern
echo "Checking for old Type[BaseException] pattern..."
rg "Type\[BaseException\]" --type python
echo "Checking current usage of type[BaseException]..."
rg "type\[BaseException\]" --type python
Length of output: 359
Script:
#!/bin/bash
# Let's try without the --type flag since python type is not recognized
echo "Checking for old Type[BaseException] pattern..."
rg "Type\[BaseException\]"
echo -e "\nChecking current usage of type[BaseException]..."
rg "type\[BaseException\]"
# Also check for general context of __exit__ method signatures
echo -e "\nChecking for __exit__ method signatures..."
rg "__exit__.*?\(.*?BaseException"
Length of output: 503
cvat/apps/dataset_manager/util.py (2)
11-11: LGTM: Import changes align with Python 3.9 deprecations
The migration of Generator and Sequence imports from typing to collections.abc is correct and follows Python 3.9's deprecation notices. The Optional import is correctly maintained in typing.
Also applies to: 16-16
Line range hint 186-194: Verify Python version compatibility requirements
The match-case statement is a Python 3.10+ feature. Please ensure that the project's minimum Python version requirement is compatible with this syntax.
cvat/apps/dataset_manager/project.py (2)
10-10: LGTM! Clean import optimization
The import statement has been correctly optimized to keep only the necessary types from the typing module.
7-7: Verify minimum Python version requirement
The change from typing.Mapping to collections.abc.Mapping is correct for Python 3.9+, but this is a breaking change for earlier versions.
✅ Verification successful
The change to use collections.abc.Mapping is appropriate
The codebase targets Python 3.9 as confirmed by the pyproject.toml configuration (target-version = ['py39']). Therefore, using collections.abc.Mapping instead of typing.Mapping is the correct approach and aligns with Python's type annotation improvements in 3.9+.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Python version requirements in project configuration files
# Check setup.py, pyproject.toml, or requirements.txt for Python version
fd -e toml -e txt -e py -x rg -l "python.*version|requires.*python"
# Check for any remaining typing imports that should be migrated
rg "from typing import .*Mapping.*"
Length of output: 325
Script:
#!/bin/bash
# Check the content of setup.py for Python version requirements
cat cvat-cli/setup.py | rg "python.*version|requires.*python" -A 2 -B 2
# Check if there are any other files that might contain Python version requirements
fd "requirements" -x cat {} | rg "python"
# Check for any Python version related configuration in the root directory
cat pyproject.toml 2>/dev/null || echo "No pyproject.toml found"
Length of output: 1144
cvat/apps/dataset_manager/bindings.py (11)
11-12: Appropriate Use of Updated Imports
The imports from collections and collections.abc are correct and align with Python 3 standards, ensuring compatibility with Python 3.9 and the deprecation of certain imports.
17-17: Correct Use of Typing Module Imports
The import statements from the typing module are appropriate and necessary for type annotations used throughout the code.
991-991: Avoid Mutable Default Arguments in attrib
This line has the same issue as previously mentioned regarding mutable default arguments. Please use factory=list instead of default=[].
1001-1001: Avoid Mutable Default Arguments in attrib
Same as above. Replace default=[] with factory=list to prevent shared mutable defaults.
1451-1451: Correct Type Annotation in Method Signature
The type annotation for labels: list[dict] in the add_labels method is appropriate and aligns with the updated type hinting practices.
1472-1479: Consistency in Constructor Type Annotations
The __init__ methods for MediaProvider and MediaProvider2D correctly use type annotations with built-in generic types, ensuring clarity and consistency.
1530-1530: Consistent Use of Type Annotations in MediaProvider3D
The constructor's type annotations are consistent with the rest of the codebase and enhance readability.
1885-1885: Appropriate Type Annotation for mangle_image_name Function
The use of defaultdict[tuple[str, str], int] in the function signature accurately represents the expected parameter type.
1906-1906: Correct Use of Type Annotations in get_defaulted_subset
The function signature correctly uses list[str] for the subsets parameter, enhancing type clarity.
Line range hint 2068-2082: Effective Use of Type Annotations in Shape Conversion Methods
The methods _convert_shapes and convert use type annotations effectively to clarify expected input and output types.
281-286: Verify Compatibility of Built-in Generic Type Annotations
The use of built-in generic types like dict[str, int] and set[int] requires Python 3.9 or newer. Please verify that the project's minimum Python version is set to 3.9 or higher to ensure compatibility.
Run the following script to check if the project's minimum required Python version is set to 3.9 or higher:
✅ Verification successful
Built-in Generic Type Annotations are Compatible
The codebase explicitly requires Python 3.9 or higher as confirmed in setup.py (python_requires=">=3.9"). Additionally, the black configuration in pyproject.toml targets Python 3.9 (target-version = ['py39']). Therefore, the use of built-in generic types like dict[str, int] and set[int] is fully compatible with the project's minimum Python version requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'python_requires' is set to '>=3.9' in setup.py or pyproject.toml
# Test: Search for 'python_requires' with version >=3.9 in the setup files
rg 'python_requires\s*=\s*[\'"]>=?3\.9' setup.py pyproject.toml
Length of output: 116
Script:
#!/bin/bash
# Let's try a more comprehensive search for Python version requirements
# Check setup.py
fd "setup.py$" --exec cat {}
# Check pyproject.toml
fd "pyproject.toml$" --exec cat {}
# Check requirements.txt
fd "requirements.txt$" --exec cat {}
# Also check for any Docker files that might specify Python version
fd "Dockerfile$" --exec grep -i "python" {}
Length of output: 5921
| source: str = attrib(default='manual') | ||
| group: int = attrib(default=0) | ||
| rotation: int = attrib(default=0) | ||
| z_order: int = attrib(default=0) | ||
| task_id: int = attrib(default=None) | ||
| subset: str = attrib(default=None) | ||
| outside: bool = attrib(default=False) | ||
| elements: List['ProjectData.LabeledShape'] = attrib(default=[]) | ||
| elements: list['ProjectData.LabeledShape'] = attrib(default=[]) |
There was a problem hiding this comment.
Avoid Mutable Default Arguments in attrib
Using a mutable default value default=[] in attrib can lead to unexpected behavior, as all instances will share the same list. Instead, use factory=list to generate a new list for each instance.
Apply this diff to fix the issue:
- elements: list['ProjectData.LabeledShape'] = attrib(default=[])
+ elements: list['ProjectData.LabeledShape'] = attrib(factory=list)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elements: list['ProjectData.LabeledShape'] = attrib(default=[]) | |
| elements: list['ProjectData.LabeledShape'] = attrib(factory=list) |
| self._tasks_data: Dict[int, TaskData] = {} | ||
| self._frame_info: Dict[Tuple[int, int], Literal["path", "width", "height", "subset"]] = dict() | ||
| self._tasks_data: dict[int, TaskData] = {} | ||
| self._frame_info: dict[tuple[int, int], Literal["path", "width", "height", "subset"]] = dict() |
There was a problem hiding this comment.
Correct Type Annotation for _frame_info Dictionary
The type annotation dict[tuple[int, int], Literal["path", "width", "height", "subset"]] seems incorrect. Using Literal here implies the values are single strings, but the actual values are likely dictionaries containing these keys. Consider updating the ValueType to dict[str, Any] to accurately reflect the structure.
Apply this diff to fix the type annotation:
- self._frame_info: dict[tuple[int, int], Literal["path", "width", "height", "subset"]] = dict()
+ self._frame_info: dict[tuple[int, int], dict[str, Any]] = dict()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._frame_info: dict[tuple[int, int], Literal["path", "width", "height", "subset"]] = dict() | |
| self._frame_info: dict[tuple[int, int], dict[str, Any]] = dict() |
Motivation and context
This is a continuation of #8626.
How has this been tested?
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
New Features
parse_export_file_pathfunction for improved clarity.Bug Fixes
get_export_cache_lockfunction to ensure non-negative values.Documentation
Refactor