Add MOT and CVAT video annotation import/export support#678
Add MOT and CVAT video annotation import/export support#678
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds first-class video annotation support (MOT and CVAT video): importer detection/flattening and conversion, Label Studio video parsing/serialization, QueryResult exports for MOT and CVAT video, video-sequence builder, tests, a small token-auth condition tweak, and a package version bump. Changes
Sequence DiagramsequenceDiagram
participant User
participant Importer as AnnotationImporter
participant Metadata as MetadataAnnotations
participant Query as QueryResult
participant Exporter as Export Converter
participant Storage
User->>Importer: import_annotations(type="mot"/"cvat_video", source)
activate Importer
Importer->>Importer: is_video_format? / _is_video_annotation
Importer->>Metadata: parse -> IRVideoBBoxFrameAnnotation / IRVideoSequence
Metadata-->>Importer: video-aware annotations
Importer->>Importer: _flatten_* => per-video sequences
Importer-->>User: flattened annotations
deactivate Importer
User->>Query: export_as_mot()/export_as_cvat_video()
activate Query
Query->>Query: _get_all_video_annotations() / _annotations_to_sequences()
Query->>Exporter: pass sequences (+optional video_file)
Exporter->>Storage: write MOT dirs or CVAT ZIP(s)
Exporter-->>Query: export path(s)
Query-->>User: return export path(s)
deactivate Query
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/data_engine/conftest.py (1)
27-30: Consider keeping the shared datasource fixture configurable.Hardcoding
DatasourceType.REPOSITORYin the common helper means every test that reusesds/other_dsnow bypasses the BUCKET/storage branches inDatasourceStateand loader code. A repo-specific fixture or asource_typeparameter would keep the new video tests deterministic without narrowing coverage for the rest oftests/data_engine/.dagshub/data_engine/model/query_result.py (1)
1086-1091: Consider documenting the multiple-key strategy forvideo_filesdict.The code adds three different keys for the same video file (full path, basename, and stem). While this appears intentional to handle different lookup patterns from the converter, a brief comment explaining this would improve maintainability.
💡 Suggested comment
if local_video is None: raise FileNotFoundError( f"Could not find local downloaded video file for '{ann_filename}' " f"under '{local_download_root}'." ) ann_path = Path(ann_filename) + # Register multiple keys so the converter can look up by full path, basename, or stem video_files[ann_filename] = local_video video_files[ann_path.name] = local_video video_files[ann_path.stem] = local_video
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32d9e47a-3ec9-4069-ae90-95375126deb1
📒 Files selected for processing (10)
dagshub/__init__.pydagshub/auth/token_auth.pydagshub/data_engine/annotation/importer.pydagshub/data_engine/annotation/metadata.pydagshub/data_engine/annotation/video.pydagshub/data_engine/model/query_result.pytests/data_engine/annotation_import/test_cvat_video.pytests/data_engine/annotation_import/test_mot.pytests/data_engine/conftest.pytests/mocks/repo_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.9)
🧰 Additional context used
🪛 GitHub Check: Flake8
dagshub/data_engine/annotation/importer.py
[failure] 165-165: dagshub/data_engine/annotation/importer.py#L165
Line too long (136 > 120 characters) (E501)
dagshub/data_engine/model/query_result.py
[failure] 793-793: dagshub/data_engine/model/query_result.py#L793
Undefined name 'IRVideoSequence' (F821)
🪛 Ruff (0.15.6)
tests/data_engine/annotation_import/test_cvat_video.py
[warning] 37-37: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
[warning] 236-236: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
tests/data_engine/annotation_import/test_mot.py
[warning] 144-144: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
[warning] 158-158: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
dagshub/data_engine/model/query_result.py
[error] 793-793: Undefined name IRVideoSequence
(F821)
🔇 Additional comments (8)
dagshub/auth/token_auth.py (1)
40-40: Readability improvement is correct.Using
is nothere keeps the same behavior and reads more clearly.dagshub/data_engine/model/query_result.py (7)
18-25: LGTM!The new imports for CVAT and MOT video export functionality are well-organized and correctly placed alongside existing annotation converter imports.
806-828: LGTM!The helper methods
_prepare_video_file_for_exportand_get_annotation_filenamecorrectly handle edge cases including path resolution with/without datasource prefix and various filename formats (None, list, tuple, string).
830-838: LGTM!Good refactoring to extract the annotation field resolution logic into a reusable helper method. The alphabetical sorting ensures deterministic behavior when no explicit field is provided.
953-964: Potential edge case:source_namescould be empty whilevideo_annotationsis non-empty.If all video annotations have
Nonefilenames,source_nameswill be empty, but the code proceeds ashas_multiple_sources = False. This leads to Line 1014 accessingsource_names[0]which would raise anIndexError.However, I see Line 1014 has a fallback:
Path(source_names[0]).stem if source_names else "sequence", so this is handled correctly.
974-1019: LGTM!The export logic properly handles both single and multi-source scenarios with appropriate error handling for missing video files. The conditional video download for dimension probing is a good optimization.
1103-1132: LGTM!The single-source CVAT export path correctly handles video file resolution and passes appropriate parameters to the converter. The error handling is comprehensive.
865-865: LGTM!Good refactoring to use the new
_resolve_annotation_fieldhelper, which consolidates the annotation field selection logic and improves code reuse across export methods.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
dagshub/data_engine/model/query_result.py (1)
790-793:⚠️ Potential issue | 🟠 MajorImport
IRVideoSequencefor the return annotation.Line 793 references
IRVideoSequence, but this module only importsIRVideoBBoxFrameAnnotation. Ruff/Pyflakes still treat that forward ref as undefined here, so linting will keep failing until the symbol is imported.🐛 Proposed fix
-from dagshub_annotation_converter.ir.video import IRVideoBBoxFrameAnnotation +from dagshub_annotation_converter.ir.video import IRVideoBBoxFrameAnnotation, IRVideoSequence
🧹 Nitpick comments (1)
tests/data_engine/annotation_import/test_mot.py (1)
295-329: Add a regression for ambiguous video identifiers.This only exercises unique filenames with
ann.filenamepresent on every frame. It won’t catchcam_a/video.mp4+cam_b/video.mp4, or multiple datapoints that rely on the datapoint path becausefilenameis missing, which are the cases the new grouping code is most likely to break. A small regression case here would lock down the intended behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d642dc65-5715-4c7d-bb0c-50ea13f132b4
📒 Files selected for processing (3)
dagshub/data_engine/annotation/importer.pydagshub/data_engine/model/query_result.pytests/data_engine/annotation_import/test_mot.py
✅ Files skipped from review due to trivial changes (1)
- dagshub/data_engine/annotation/importer.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
dagshub/data_engine/model/query_result.py (4)
803-807:⚠️ Potential issue | 🟠 MajorFix unresolved
IRVideoSequencetype reference at Line 806.
List["IRVideoSequence"]is currently flagged as undefined (F821). Add a type-checking import forIRVideoSequencein this module.Suggested fix
if TYPE_CHECKING: + from dagshub_annotation_converter.ir.video import IRVideoSequence import datasets as hf_ds#!/bin/bash # Verify the unresolved symbol and import presence rg -n 'IRVideoSequence|TYPE_CHECKING' dagshub/data_engine/model/query_result.py
809-816:⚠️ Potential issue | 🔴 CriticalDo not group sequences by optional filename only.
When filename is missing, the fallback
""merges annotations from different datapoints into a single sequence. Preserve a stable per-datapoint source key through grouping.
985-993:⚠️ Potential issue | 🔴 CriticalPreserve full source identity; basename/stem introduces collisions.
Using
.name/.stemcan collapse distinct videos (cam_a/video.mp4vscam_b/video.mp4, orvideo.mp4vsvideo.mov). Use full repo-relative identifiers (or sequence keys) for source detection andvideo_filesmapping.Also applies to: 1062-1069, 1093-1097
994-999:⚠️ Potential issue | 🟠 MajorAvoid unconditional video downloads in export paths.
Both exporters download videos eagerly even when dimensions are already available from annotations or explicit args. Gate downloads/probing to only sequences that still need width/height inference.
Also applies to: 1074-1080
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 740caa4c-cfd8-423b-acb9-379c7e0d707d
📒 Files selected for processing (8)
dagshub/data_engine/annotation/importer.pydagshub/data_engine/annotation/metadata.pydagshub/data_engine/annotation/video.pydagshub/data_engine/model/query_result.pysetup.pytests/data_engine/annotation_import/test_annotation_parsing.pytests/data_engine/annotation_import/test_cvat_video.pytests/data_engine/annotation_import/test_mot.py
✅ Files skipped from review due to trivial changes (1)
- setup.py
🚧 Files skipped from review as they are similar to previous changes (3)
- dagshub/data_engine/annotation/metadata.py
- dagshub/data_engine/annotation/video.py
- dagshub/data_engine/annotation/importer.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
🧰 Additional context used
🪛 GitHub Check: Flake8
tests/data_engine/annotation_import/test_mot.py
[failure] 10-10: tests/data_engine/annotation_import/test_mot.py#L10
'dagshub_annotation_converter.ir.video.IRVideoSequence' imported but unused (F401)
dagshub/data_engine/model/query_result.py
[failure] 806-806: dagshub/data_engine/model/query_result.py#L806
Undefined name 'IRVideoSequence' (F821)
🪛 Ruff (0.15.9)
tests/data_engine/annotation_import/test_mot.py
[warning] 205-205: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
[warning] 219-219: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
dagshub/data_engine/model/query_result.py
[error] 806-806: Undefined name IRVideoSequence
(F821)
tests/data_engine/annotation_import/test_cvat_video.py
[warning] 37-37: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
[warning] 276-276: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🔇 Additional comments (1)
tests/data_engine/annotation_import/test_annotation_parsing.py (1)
175-213: Good regression coverage for LS video task serialization.This test validates the key contract (
data.video,videorectangle, andframesCountconsistency) on the new video-track path.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/data_engine/annotation_import/test_mot.py (2)
205-205: Consider usingnext(iter(...))for clarity.Static analysis suggests
next(iter(result.values()))instead of slicing. While the performance difference is negligible in tests, the iterator approach is more idiomatic when you only need the first item.♻️ Proposed refactor
- anns = list(result.values())[0] + anns = next(iter(result.values()))
219-219: Consider usingnext(iter(...))for clarity.Same suggestion as line 205:
next(iter(result.values()))is more idiomatic when extracting the first value.♻️ Proposed refactor
- assert len(list(result.values())[0]) == 2 + assert len(next(iter(result.values()))) == 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 885a4f8d-69aa-4620-957e-005d11a8e97f
📒 Files selected for processing (3)
dagshub/data_engine/model/query_result.pysetup.pytests/data_engine/annotation_import/test_mot.py
✅ Files skipped from review due to trivial changes (1)
- dagshub/data_engine/model/query_result.py
🚧 Files skipped from review as they are similar to previous changes (1)
- setup.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🧰 Additional context used
🪛 Ruff (0.15.9)
tests/data_engine/annotation_import/test_mot.py
[warning] 205-205: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
[warning] 219-219: Prefer next(iter(result.values())) over single element slice
Replace with next(iter(result.values()))
(RUF015)
🔇 Additional comments (9)
tests/data_engine/annotation_import/test_mot.py (9)
21-24: LGTM: Clean fixture design.The
autousefixture provides a sensible default while individual tests can override it as needed.
27-53: LGTM: Comprehensive edge case coverage.The tests cover all relevant scenarios for video annotation detection, including edge cases like empty dicts and mixed key types.
58-73: LGTM: Well-structured parametrized test.The parametrized approach efficiently verifies video format detection for all supported annotation types.
79-133: LGTM: Thorough flattening logic validation.The tests comprehensively verify video annotation flattening across different input formats and naming scenarios, including proper handling of nested paths.
136-156: LGTM: Critical dimension propagation test.Verifies that video dimensions are correctly propagated from frame-level annotations to the sequence object, which is essential for proper MOT export.
158-192: LGTM: Export path computation validation.The tests ensure correct directory structure computation across various datasource prefix scenarios, preventing path duplication and ensuring proper layout.
262-275: LGTM: Label Studio conversion validation.The tests verify proper JSON task generation and confirm that empty annotations are correctly filtered out.
281-440: LGTM: Comprehensive MOT export validation.The export tests thoroughly cover:
- Directory structure and file creation
- Dimension handling (explicit and fallback)
- Error cases (missing annotations)
- Multi-video scenarios
- Path resolution with proper mocking
The tests properly isolate dependencies and verify key behaviors across diverse scenarios.
446-498: LGTM: Well-designed test utilities.The helper functions effectively reduce duplication and create realistic test fixtures with sensible defaults, improving overall test maintainability.
2 participants
Based on changes to dagshub-annotation-converter to the same effect: DagsHub/dagshub-annotation-converter#25
Implementation Details
The PR extends annotation support through a multi-layered approach:
Importer Module Enhancements (
importer.py):AnnotationTypeliteral to include "mot", and "cvat_video"is_video_formatproperty to identify video-based annotation types_flatten_video_annotationsto convert per-frame annotations into single video-entry dictionariesconvert_to_ls_tasksfor Label Studio video task generationremap_annotationsto handle cases where video format annotations lack explicit filenames_is_video_annotation_dict,_flatten_cvat_fs_annotations,_flatten_mot_fs_annotationsMetadata Annotations Support (
metadata.py):VideoRectangleAnnotationsupport in Label Studio task lookup registryQuery Result Export API (
query_result.py):export_as_mot()- supports single/multi-video export with optional dimension parametersexport_as_cvat_video()- handles video CVAT XML generation with video name customization_get_all_video_annotations,_prepare_video_file_for_export,_get_annotation_filename_resolve_annotation_fieldto auto-select available annotation field with alphabetical fallbackTest Coverage:
test_mot.py(383 lines): Video annotation flattening, directory/zip import, MOT export structure, dimension handlingtest_cvat_video.py(276 lines): CVAT video XML import/export, track/box element generation, multi-datapoint supportCode Quality and Testing Infrastructure:
ruff.tomllinting configuration with comprehensive rule set including:__init__.pyandconftest.pyDatasourceType.REPOSITORYassignment in test fixtures (tests/data_engine/conftest.py)idproperty toMockRepoAPItest utilitytoken_auth.py(no behavioral change - logical equivalence in boolean expression)Documentation Note: Per PR comments, the supported formats list at dagshub.com/docs/use_cases/annotation/#supported-formats-for-annotation_imports requires manual update to reflect MOT, CVAT Video support.