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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive support for video annotations (MOT and CVAT video formats) to the Dagshub annotation system, enabling import and export of frame-based video annotations alongside existing image annotation support. Changes
Sequence DiagramsequenceDiagram
actor User
participant Importer as AnnotationImporter
participant Converter as Video/MOT<br/>Converters
participant Metadata as MetadataAnnotations
participant QueryResult as QueryResult
participant LS as Label Studio
User->>Importer: import_annotations(source, type="mot"/"cvat_video")
Importer->>Converter: load_mot_from_fs() or<br/>parse_cvat_xml()
Converter-->>Importer: List[IRVideoBBoxFrameAnnotation]
Importer->>Importer: _flatten_video_annotations()
Importer-->>User: Dict[video_name, annotations]
User->>Metadata: to_ls_task(video_annotations)
Metadata->>Metadata: Detect IRVideoAnnotationTrack
Metadata->>Converter: from_ir_track() to VideoRectangleAnnotation
Metadata->>LS: task.data["video"] + videorectangle results
Metadata-->>User: Label Studio Task
User->>QueryResult: export_as_mot() / export_as_cvat_video()
QueryResult->>QueryResult: _get_all_video_annotations()
QueryResult->>QueryResult: _annotations_to_sequences()
QueryResult->>Converter: export_mot_to_dir() or<br/>export_cvat_video_to_zip()
Converter-->>QueryResult: Generated export
QueryResult-->>User: Exported video labels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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.
|
Tests are failing |
| Path(ann_filename).name | ||
| for ann_filename in (self._get_annotation_filename(ann) for ann in video_annotations) | ||
| for ann_filename in (ann.filename for ann in video_annotations) | ||
| if ann_filename |
There was a problem hiding this comment.
Can be simplified to use ann.filename everywhere
| Path(ann_filename).name | ||
| for ann_filename in (self._get_annotation_filename(ann) for ann in video_annotations) | ||
| for ann_filename in (ann.filename for ann in video_annotations) | ||
| if ann_filename |
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.