From 6e7640ececa978def08ee0b355ce660c27bf8fa4 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 11:31:08 -0300 Subject: [PATCH 1/7] [ar-api] Add native video upload, status and segment annotation to the CLI (VID-53) Action Recognition projects take whole videos as Sources, but the CLI had no way to ingest or label one. `roboflow video` only reached the legacy async video *inference* job API. Extend the existing video group with three commands over the already-published SDK methods: - `video upload` streams original MP4/MOV bytes and reports the canonical video ID, waiting for a terminal state by default. - `video upload-status` reads ingestion state. The name keeps it unambiguous against `video status`, which still checks an inference job. - `video annotate` forwards a complete roboflow-video-coco file unchanged, so native frame indices, PTS and rational time bases survive as authored. Upload composes `upload_video(wait=False)` with `wait_for_video_upload` rather than the SDK's internal wait, so a bounded-wait timeout can still name the video ID to re-check. Credential precedence, `--json` schemas and the 0/1/2/3 exit-code contract follow the existing handlers. Co-Authored-By: Claude Opus 5 (1M context) --- CLI-COMMANDS.md | 47 ++- roboflow/cli/handlers/video.py | 341 +++++++++++++++++++- tests/cli/test_video_handler.py | 536 ++++++++++++++++++++++++++++++++ 3 files changed, 919 insertions(+), 5 deletions(-) diff --git a/CLI-COMMANDS.md b/CLI-COMMANDS.md index 2a7667af..7d92accd 100644 --- a/CLI-COMMANDS.md +++ b/CLI-COMMANDS.md @@ -471,8 +471,53 @@ roboflow workspace stats --start-date 2026-01-01 --end-date 2026-03-31 roboflow universe search "hard hats" --type dataset --limit 5 ``` +### Native video upload and segment annotation + +Action Recognition projects take whole videos as Sources. `video upload` streams the +original MP4/MOV bytes without re-encoding, then reports the **canonical video ID** to use +for every later action. That ID can differ from the ID reserved at the start of the upload, +because identical content is deduplicated onto the existing Source. + +```bash +# 1. Upload original bytes and wait for a terminal state (the default). +roboflow video upload -p my-ar-project -f clip.mov --json +# { "videoId": "aBcD1234", "status": "uploaded", "resolvedBatch": { ... } } +``` + +Pass `--no-wait` to return as soon as the bytes are stored, then poll separately. +`video upload-status` reports ingestion state — it is distinct from `video status`, which +checks a legacy video *inference* job. + +```bash +# 2. Poll ingestion yourself. +roboflow video upload -p my-ar-project -f clip.mov --no-wait --json +roboflow video upload-status aBcD1234 -p my-ar-project --json +roboflow video upload-status aBcD1234 -p my-ar-project --wait --poll-timeout 120 +``` + +```bash +# 3. Annotate segments from a complete roboflow-video-coco document. +# The file is forwarded unchanged, so native frame indices, PTS and +# rational time bases are preserved exactly as authored. +roboflow video annotate -p my-ar-project -i aBcD1234 -a segments.json --json +# { "success": true, "inDataset": true, "createdClasses": ["walking"] } +``` + +Upload accepts `-b/--batch`, `-t/--tag` (comma-separated), `--metadata` (JSON object) and +`-s/--split`. Annotate defaults to the API behaviour of adding the Source to the Dataset; +override with `--no-add-to-dataset`, set the split with `-s/--split`, and pass `--overwrite` +to replace segments that already differ (otherwise a conflicting save is rejected and an +identical re-submit succeeds). + +Exit codes follow the CLI contract: `0` success, `1` error, `2` auth, `3` not found. A +`failed` ingestion state and a `--wait` timeout both exit nonzero; the timeout message names +the video ID so you can re-check it with `video upload-status`. + ### Video inference +Separate from native upload: this submits a legacy asynchronous inference job for a trained +model version. + ```bash roboflow video infer -p my-project -v 3 -f video.mp4 --fps 10 roboflow video status @@ -566,7 +611,7 @@ Version numbers are always numeric — that's how `x/y` is disambiguated between | `asynctasks` | Inspect async background tasks (e.g. project forks) | | `trash` | List items in Trash | | `universe` | Search Roboflow Universe | -| `video` | Video inference | +| `video` | Native video upload/status/annotation, and video inference | | `batch` | Batch processing jobs *(coming soon)* | | `completion` | Install or generate shell completion scripts (bash, zsh, fish) | diff --git a/roboflow/cli/handlers/video.py b/roboflow/cli/handlers/video.py index 1fdc9905..c86691cf 100644 --- a/roboflow/cli/handlers/video.py +++ b/roboflow/cli/handlers/video.py @@ -1,14 +1,18 @@ -"""Video inference commands.""" +"""Video commands: native video Source ingestion/annotation and legacy video inference.""" from __future__ import annotations -from typing import Annotated +from typing import Annotated, Optional import typer from roboflow.cli._compat import SortedGroup, ctx_to_args -video_app = typer.Typer(cls=SortedGroup, help="Video inference operations", no_args_is_help=True) +video_app = typer.Typer( + cls=SortedGroup, + help="Native video upload/annotation and video inference operations", + no_args_is_help=True, +) @video_app.command("infer") @@ -29,11 +33,111 @@ def status( ctx: typer.Context, job_id: Annotated[str, typer.Argument(help="Job ID to check")], ) -> None: - """Check video inference job status.""" + """Check a legacy video inference job's status (not native upload ingestion).""" args = ctx_to_args(ctx, job_id=job_id) _video_status(args) +@video_app.command("upload") +def upload( + ctx: typer.Context, + project: Annotated[str, typer.Option("-p", "--project", help="Project ID, or workspace/project")], + video_file: Annotated[str, typer.Option("-f", "--file", help="Path to an original .mp4 or .mov file")], + batch: Annotated[Optional[str], typer.Option("-b", "--batch", help="Annotation batch name")] = None, + metadata: Annotated[ + Optional[str], typer.Option("--metadata", help='JSON object of metadata, e.g. \'{"camera": "one"}\'') + ] = None, + poll_interval: Annotated[ + float, typer.Option("--poll-interval", help="Seconds between status polls while waiting") + ] = 2.0, + poll_timeout: Annotated[float, typer.Option("--poll-timeout", help="Seconds to wait for a terminal state")] = 300.0, + split: Annotated[Optional[str], typer.Option("-s", "--split", help="Dataset split: train, valid or test")] = None, + tag: Annotated[Optional[str], typer.Option("-t", "--tag", help="Comma-separated tag names")] = None, + wait: Annotated[ + bool, typer.Option("--wait/--no-wait", help="Poll until the upload reaches a terminal state") + ] = True, +) -> None: + """Upload original video bytes as a native video Source. + + Streams the file unchanged and reports the canonical video ID to annotate. + """ + args = ctx_to_args( + ctx, + project=project, + video_file=video_file, + batch=batch, + metadata=metadata, + poll_interval=poll_interval, + poll_timeout=poll_timeout, + split=split, + tag=tag, + wait=wait, + ) + _video_upload(args) + + +@video_app.command("upload-status") +def upload_status( + ctx: typer.Context, + video_id: Annotated[str, typer.Argument(help="Video ID reported by 'roboflow video upload'")], + project: Annotated[str, typer.Option("-p", "--project", help="Project ID, or workspace/project")], + poll_interval: Annotated[ + float, typer.Option("--poll-interval", help="Seconds between status polls while waiting") + ] = 2.0, + poll_timeout: Annotated[float, typer.Option("--poll-timeout", help="Seconds to wait for a terminal state")] = 300.0, + wait: Annotated[ + bool, typer.Option("--wait/--no-wait", help="Poll until a terminal state instead of reading once") + ] = False, +) -> None: + """Check a native video upload's ingestion status and canonical video ID.""" + args = ctx_to_args( + ctx, + video_id=video_id, + project=project, + poll_interval=poll_interval, + poll_timeout=poll_timeout, + wait=wait, + ) + _video_upload_status(args) + + +@video_app.command("annotate") +def annotate( + ctx: typer.Context, + annotation_file: Annotated[ + str, typer.Option("-a", "--annotation-file", help="Path to a complete roboflow-video-coco JSON file") + ], + project: Annotated[str, typer.Option("-p", "--project", help="Project ID, or workspace/project")], + video_id: Annotated[str, typer.Option("-i", "--video-id", help="Canonical video ID from 'video upload'")], + add_to_dataset: Annotated[ + Optional[bool], + typer.Option( + "--add-to-dataset/--no-add-to-dataset", + help="Override the API default of adding the Source to the Dataset", + ), + ] = None, + overwrite: Annotated[ + bool, typer.Option("--overwrite", help="Replace different existing segments on this video") + ] = False, + split: Annotated[Optional[str], typer.Option("-s", "--split", help="Dataset split: train, valid or test")] = None, +) -> None: + """Annotate a native video Source's segments from a video-coco file. + + The file is read whole and forwarded unchanged, so native frame indices, + PTS and rational time bases survive exactly as authored. + """ + args = ctx_to_args( + ctx, + annotation_file=annotation_file, + project=project, + video_id=video_id, + add_to_dataset=add_to_dataset, + overwrite=overwrite, + split=split, + ) + _video_annotate(args) + + # --------------------------------------------------------------------------- # Business logic (unchanged from argparse version) # --------------------------------------------------------------------------- @@ -113,3 +217,232 @@ def _video_status(args) -> None: # noqa: ANN001 if progress: text_lines.append(f"Progress: {progress}") output(args, data, text="\n".join(text_lines)) + + +# --------------------------------------------------------------------------- +# Native video Source business logic +# --------------------------------------------------------------------------- + +_TERMINAL_UPLOAD_STATES = frozenset({"uploaded", "failed"}) + + +def _load_project(args): # noqa: ANN001 + """Load the project for a native video command, honoring CLI credential precedence.""" + from roboflow.adapters import rfapi + from roboflow.cli._output import output_error + from roboflow.cli._resolver import resolve_project_context + + resolved = resolve_project_context(args) + if resolved is None: + return None + api_key, workspace, project_slug = resolved + + try: + data = rfapi.get_project(api_key, workspace, project_slug) + except rfapi.RoboflowError as exc: + output_error( + args, + str(exc), + hint=f"Check that project '{workspace}/{project_slug}' exists and your API key can read it.", + exit_code=3, + ) + return None + + from roboflow.core.project import Project + + return Project(api_key, data["project"]) + + +def _emit_upload_status(args, status) -> None: # noqa: ANN001 + """Render an ingestion status, exiting nonzero when the upload failed.""" + from roboflow.cli._output import output, output_error + + state = status.get("status", "unknown") + video_id = status.get("videoId", "") + + if state == "failed": + output_error( + args, + f"Native video upload {video_id} failed during processing.", + hint=status.get("error") or "Re-upload the original file and check that it is a valid MP4/MOV.", + ) + return + + lines = [f"Video ID: {video_id}", f"Status: {state}"] + if status.get("duplicate"): + lines.append("Duplicate: yes (deduplicated onto an existing Source)") + resolved_batch = status.get("resolvedBatch") + if isinstance(resolved_batch, dict): + lines.append(f"Batch: {resolved_batch.get('name', '')} ({resolved_batch.get('id', '')})") + elif resolved_batch: + lines.append(f"Batch: {resolved_batch}") + if state not in _TERMINAL_UPLOAD_STATES: + lines.append(f"Still processing. Re-check with 'roboflow video upload-status {video_id} -p {args.project}'.") + + # The server status document is the JSON payload, so --json stays a stable + # passthrough of videoId/status/duplicate/resolvedBatch. + output(args, status, text="\n".join(lines)) + + +def _wait_for_upload(args, project, video_id): # noqa: ANN001 + """Bounded wait, reporting the video ID so a timeout stays actionable.""" + from roboflow.adapters import rfapi + from roboflow.cli._output import output_error + + try: + return project.wait_for_video_upload( + video_id, + poll_interval=args.poll_interval, + poll_timeout=args.poll_timeout, + ) + except ValueError as exc: + output_error(args, str(exc), hint="Use a positive --poll-interval and a nonnegative --poll-timeout.") + return None + except rfapi.RoboflowError as exc: + output_error( + args, + str(exc), + hint=f"Re-check with 'roboflow video upload-status {video_id} -p {args.project}'.", + ) + return None + + +def _video_upload(args) -> None: # noqa: ANN001 + import json as json_mod + + from roboflow.adapters import rfapi + from roboflow.cli._output import output_error + + metadata = None + if args.metadata: + try: + metadata = json_mod.loads(args.metadata) + except json_mod.JSONDecodeError as exc: + output_error(args, f"Invalid metadata JSON: {exc}", hint='Example: \'{"camera": "one"}\'') + return + if not isinstance(metadata, dict): + output_error(args, "Metadata must be a JSON object.", hint='Example: \'{"camera": "one"}\'') + return + + tags = [t.strip() for t in args.tag.split(",") if t.strip()] if args.tag else None + + project = _load_project(args) + if project is None: + return + + try: + # Upload without waiting so the reserved video ID is known even if a + # later bounded wait times out; `wait_for_video_upload` continues it. + status = project.upload_video( + args.video_file, + batch_name=args.batch, + tag_names=tags, + metadata=metadata, + split=args.split, + wait=False, + ) + except ValueError as exc: + output_error(args, str(exc), hint="Native video upload accepts original .mp4 and .mov files.") + return + except rfapi.RoboflowError as exc: + output_error(args, str(exc), hint="Check the project type, your plan limits and the video file.") + return + + video_id = status.get("videoId") + if args.wait and video_id and status.get("status") not in _TERMINAL_UPLOAD_STATES: + status = _wait_for_upload(args, project, video_id) + if status is None: + return + + _emit_upload_status(args, status) + + +def _video_upload_status(args) -> None: # noqa: ANN001 + from roboflow.adapters import rfapi + from roboflow.cli._output import output_error + + project = _load_project(args) + if project is None: + return + + if args.wait: + status = _wait_for_upload(args, project, args.video_id) + if status is None: + return + else: + try: + status = project.get_video_upload_status(args.video_id) + except rfapi.RoboflowError as exc: + not_found = getattr(exc, "status_code", None) == 404 + output_error( + args, + str(exc), + hint=f"Check the video ID reported by 'roboflow video upload -p {args.project}'." + if not_found + else None, + exit_code=3 if not_found else 1, + ) + return + + _emit_upload_status(args, status) + + +def _video_annotate(args) -> None: # noqa: ANN001 + import json as json_mod + + from roboflow.adapters.rfapi import AnnotationSaveError + from roboflow.cli._output import output, output_error + + try: + with open(args.annotation_file) as handle: + document = json_mod.load(handle) + except OSError as exc: + output_error(args, f"Cannot read annotation file: {exc}", hint="Pass the path to a video-coco JSON file.") + return + except json_mod.JSONDecodeError as exc: + output_error( + args, + f"Invalid JSON in {args.annotation_file}: {exc}", + hint="The file must be one complete roboflow-video-coco document.", + ) + return + + if not isinstance(document, dict): + output_error( + args, + f"{args.annotation_file} must contain a JSON object.", + hint="The file must be one complete roboflow-video-coco document.", + ) + return + + project = _load_project(args) + if project is None: + return + + try: + # `document` is forwarded as parsed: no re-encoding of frames, PTS or time bases. + result = project.annotate_video_segments( + args.video_id, + document, + overwrite=args.overwrite, + split=args.split, + add_to_dataset=args.add_to_dataset, + ) + except AnnotationSaveError as exc: + status_code = getattr(exc, "status_code", None) + if status_code == 409: + hint = "Different segments already exist on this video. Re-run with --overwrite to replace them." + elif status_code == 404: + hint = "Check the canonical video ID from 'roboflow video upload'." + else: + hint = "Check that the document is a complete video-coco with at least one segment." + output_error(args, str(exc), hint=hint, exit_code=3 if status_code == 404 else 1) + return + + lines = [f"Annotated video {args.video_id}."] + if "inDataset" in result: + lines.append(f"In dataset: {'yes' if result.get('inDataset') else 'no'}") + created = result.get("createdClasses") + if created: + lines.append(f"Created classes: {', '.join(map(str, created))}") + output(args, result, text="\n".join(lines)) diff --git a/tests/cli/test_video_handler.py b/tests/cli/test_video_handler.py index 6deb1bdf..7fab1e11 100644 --- a/tests/cli/test_video_handler.py +++ b/tests/cli/test_video_handler.py @@ -1,6 +1,7 @@ """Tests for the video CLI handler.""" import json +import os import unittest from unittest.mock import patch @@ -60,5 +61,540 @@ def test_status_passes_job_id_to_api(self, _mock_key, mock_api) -> None: mock_api.assert_called_once_with("fake-key", "my-unique-job-777") +AR_PROJECT_PAYLOAD = { + "project": { + "annotation": "actions", + "classes": {"walking": 2}, + "colors": {"walking": "#FF00FF"}, + "created": 1759000000.0, + "id": "model-evaluation-workspace/penguin-actions", + "images": 1, + "name": "Penguin Actions", + "public": False, + "splits": {"train": 1, "test": 0, "valid": 0}, + "type": "action-recognition", + "unannotated": 0, + "updated": 1759000001.0, + } +} + +# A complete video-coco document with native frame indices, PTS and a rational +# time base. Tests assert it reaches the SDK byte-for-value identical. +VIDEO_COCO_DOCUMENT = { + "info": {"version": "roboflow-video-coco"}, + "videos": [ + { + "id": 1, + "file_name": "clip.mov", + "fps": 30000 / 1001, + "time_base": [1, 600], + "duration": 12.659047, + "nb_frames": 379, + } + ], + "categories": [ + {"id": 0, "name": "actions"}, + {"id": 1, "name": "walking", "supercategory": "actions"}, + ], + "annotations": { + "segments": [ + { + "id": 1, + "video_id": 1, + "category_id": 1, + "start_frame": 12, + "end_frame": 96, + "start_pts": 240, + "end_pts": 1920, + "time_base": [1, 600], + } + ] + }, +} + + +def _write_json(directory, name, payload): + import json as json_mod + import os + + path = os.path.join(directory, name) + with open(path, "w") as handle: + json_mod.dump(payload, handle) + return path + + +class NativeVideoCliTest(unittest.TestCase): + """Shared fixtures that let real command dispatch build a real Project.""" + + def setUp(self) -> None: + import tempfile + + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.video_path = os.path.join(self.tmp.name, "clip.mp4") + with open(self.video_path, "wb") as handle: + handle.write(b"original video bytes") + + key_patch = patch("roboflow.config.load_roboflow_api_key", return_value="fake-key") + key_patch.start() + self.addCleanup(key_patch.stop) + + project_patch = patch("roboflow.adapters.rfapi.get_project", return_value=AR_PROJECT_PAYLOAD) + self.mock_get_project = project_patch.start() + self.addCleanup(project_patch.stop) + + @property + def project_ref(self) -> str: + return "model-evaluation-workspace/penguin-actions" + + +class TestNativeVideoRegistration(NativeVideoCliTest): + """The real CLI exposes the native video commands alongside inference.""" + + def test_native_commands_are_registered(self) -> None: + for command in ("upload", "upload-status", "annotate"): + with self.subTest(command=command): + result = runner.invoke(app, ["video", command, "--help"]) + self.assertEqual(result.exit_code, 0) + + def test_upload_status_is_distinct_from_inference_status(self) -> None: + group = runner.invoke(app, ["video", "--help"]) + self.assertEqual(group.exit_code, 0) + self.assertIn("upload-status", group.output) + # The legacy inference job command keeps its own name and contract. + self.assertIn("status", group.output) + + +class TestVideoUpload(NativeVideoCliTest): + """`roboflow video upload` streams original bytes and reports canonical IDs.""" + + @patch("roboflow.core.project.Project.wait_for_video_upload") + @patch("roboflow.core.project.Project.upload_video") + def test_forwards_all_options_and_waits_by_default(self, mock_upload, mock_wait) -> None: + mock_upload.return_value = {"videoId": "upload-1", "status": "pending"} + mock_wait.return_value = { + "videoId": "source-9", + "status": "uploaded", + "resolvedBatch": {"id": "b1", "name": "clips"}, + } + + result = runner.invoke( + app, + [ + "video", + "upload", + "-p", + self.project_ref, + "-f", + self.video_path, + "-b", + "clips", + "-t", + "indoor, penguin", + "--metadata", + '{"camera": "one"}', + "-s", + "valid", + ], + ) + + self.assertEqual(result.exit_code, 0, result.output) + mock_upload.assert_called_once_with( + self.video_path, + batch_name="clips", + tag_names=["indoor", "penguin"], + metadata={"camera": "one"}, + split="valid", + wait=False, + ) + # The bounded wait continues on the ID the first status reported. + mock_wait.assert_called_once_with("upload-1", poll_interval=2.0, poll_timeout=300.0) + self.assertIn("source-9", result.output) + self.assertIn("uploaded", result.output) + + @patch("roboflow.core.project.Project.wait_for_video_upload") + @patch("roboflow.core.project.Project.upload_video") + def test_no_wait_reports_reservation_without_polling(self, mock_upload, mock_wait) -> None: + mock_upload.return_value = {"videoId": "upload-1", "status": "pending"} + + result = runner.invoke( + app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path, "--no-wait"] + ) + + self.assertEqual(result.exit_code, 0, result.output) + mock_wait.assert_not_called() + data = json.loads(result.output) + self.assertEqual(data, {"videoId": "upload-1", "status": "pending"}) + + @patch("roboflow.core.project.Project.wait_for_video_upload") + @patch("roboflow.core.project.Project.upload_video") + def test_terminal_dedup_status_skips_the_wait(self, mock_upload, mock_wait) -> None: + mock_upload.return_value = { + "videoId": "source-2", + "status": "uploaded", + "duplicate": True, + "resolvedBatch": None, + } + + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + + self.assertEqual(result.exit_code, 0, result.output) + mock_wait.assert_not_called() + data = json.loads(result.output) + self.assertEqual(data["videoId"], "source-2") + self.assertIs(data["duplicate"], True) + self.assertIsNone(data["resolvedBatch"]) + + @patch("roboflow.core.project.Project.wait_for_video_upload") + @patch("roboflow.core.project.Project.upload_video") + def test_custom_poll_bounds_are_forwarded(self, mock_upload, mock_wait) -> None: + mock_upload.return_value = {"videoId": "upload-1", "status": "pending"} + mock_wait.return_value = {"videoId": "upload-1", "status": "uploaded"} + + result = runner.invoke( + app, + [ + "video", + "upload", + "-p", + self.project_ref, + "-f", + self.video_path, + "--poll-interval", + "0.5", + "--poll-timeout", + "30", + ], + ) + + self.assertEqual(result.exit_code, 0, result.output) + mock_wait.assert_called_once_with("upload-1", poll_interval=0.5, poll_timeout=30.0) + + @patch("roboflow.core.project.Project.upload_video") + def test_failed_processing_exits_nonzero(self, mock_upload) -> None: + mock_upload.return_value = {"videoId": "upload-1", "status": "failed"} + + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + + self.assertNotEqual(result.exit_code, 0) + payload = json.loads(result.output) + self.assertIn("failed", payload["error"]["message"]) + + @patch("roboflow.core.project.Project.upload_video") + def test_wait_timeout_names_the_video_id_to_recheck(self, mock_upload) -> None: + from roboflow.adapters.rfapi import RoboflowError + + mock_upload.return_value = {"videoId": "upload-7", "status": "pending"} + with patch( + "roboflow.core.project.Project.wait_for_video_upload", + side_effect=RoboflowError("Video upload upload-7 is still pending after 30s"), + ): + result = runner.invoke( + app, + ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path, "--poll-timeout", "30"], + ) + + self.assertNotEqual(result.exit_code, 0) + payload = json.loads(result.output) + self.assertIn("upload-status upload-7", payload["error"]["hint"]) + + @patch("roboflow.core.project.Project.upload_video") + def test_unsupported_extension_is_rejected_by_the_sdk(self, mock_upload) -> None: + mock_upload.side_effect = ValueError("Native video upload accepts .mp4 and .mov files") + + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + + self.assertNotEqual(result.exit_code, 0) + payload = json.loads(result.output) + self.assertIn(".mp4", payload["error"]["message"]) + + @patch("roboflow.core.project.Project.upload_video") + def test_malformed_metadata_never_reaches_the_api(self, mock_upload) -> None: + result = runner.invoke( + app, + ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path, "--metadata", "{not json"], + ) + + self.assertNotEqual(result.exit_code, 0) + mock_upload.assert_not_called() + self.mock_get_project.assert_not_called() + self.assertIn("Invalid metadata JSON", json.loads(result.output)["error"]["message"]) + + @patch("roboflow.core.project.Project.upload_video") + def test_non_object_metadata_is_rejected(self, mock_upload) -> None: + result = runner.invoke( + app, + ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path, "--metadata", "[1, 2]"], + ) + + self.assertNotEqual(result.exit_code, 0) + mock_upload.assert_not_called() + + @patch("roboflow.core.project.Project.upload_video") + def test_server_error_exits_nonzero(self, mock_upload) -> None: + from roboflow.adapters.rfapi import RoboflowError + + mock_upload.side_effect = RoboflowError("quota exceeded") + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + + self.assertNotEqual(result.exit_code, 0) + self.assertIn("quota exceeded", json.loads(result.output)["error"]["message"]) + + def test_missing_api_key_exits_with_auth_code(self) -> None: + with patch("roboflow.config.load_roboflow_api_key", return_value=None): + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + self.assertEqual(result.exit_code, 2) + + @patch("roboflow.core.project.Project.upload_video") + def test_explicit_api_key_takes_precedence(self, mock_upload) -> None: + mock_upload.return_value = {"videoId": "v1", "status": "uploaded"} + result = runner.invoke( + app, + ["--api-key", "explicit-key", "video", "upload", "-p", self.project_ref, "-f", self.video_path], + ) + self.assertEqual(result.exit_code, 0, result.output) + self.mock_get_project.assert_called_once_with("explicit-key", "model-evaluation-workspace", "penguin-actions") + + +class TestVideoUploadStatus(NativeVideoCliTest): + """`roboflow video upload-status` reads native ingestion state.""" + + @patch("roboflow.core.project.Project.get_video_upload_status") + def test_single_read_by_default(self, mock_status) -> None: + mock_status.return_value = {"videoId": "source-3", "status": "uploaded"} + + result = runner.invoke(app, ["--json", "video", "upload-status", "source-3", "-p", self.project_ref]) + + self.assertEqual(result.exit_code, 0, result.output) + mock_status.assert_called_once_with("source-3") + self.assertEqual(json.loads(result.output)["status"], "uploaded") + + @patch("roboflow.core.project.Project.wait_for_video_upload") + @patch("roboflow.core.project.Project.get_video_upload_status") + def test_wait_uses_the_bounded_poll(self, mock_status, mock_wait) -> None: + mock_wait.return_value = {"videoId": "source-3", "status": "uploaded"} + + result = runner.invoke( + app, + ["video", "upload-status", "source-3", "-p", self.project_ref, "--wait", "--poll-timeout", "12"], + ) + + self.assertEqual(result.exit_code, 0, result.output) + mock_status.assert_not_called() + mock_wait.assert_called_once_with("source-3", poll_interval=2.0, poll_timeout=12.0) + + @patch("roboflow.core.project.Project.get_video_upload_status") + def test_pending_state_points_at_the_recheck_command(self, mock_status) -> None: + mock_status.return_value = {"videoId": "source-3", "status": "pending"} + + result = runner.invoke(app, ["video", "upload-status", "source-3", "-p", self.project_ref]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertIn("upload-status source-3", result.output) + + @patch("roboflow.core.project.Project.get_video_upload_status") + def test_unknown_video_exits_not_found(self, mock_status) -> None: + from roboflow.adapters.rfapi import RoboflowError + + mock_status.side_effect = RoboflowError("not found", status_code=404) + result = runner.invoke(app, ["--json", "video", "upload-status", "nope", "-p", self.project_ref]) + + self.assertEqual(result.exit_code, 3) + + @patch("roboflow.core.project.Project.get_video_upload_status") + def test_failed_state_exits_nonzero(self, mock_status) -> None: + mock_status.return_value = {"videoId": "source-3", "status": "failed"} + result = runner.invoke(app, ["--json", "video", "upload-status", "source-3", "-p", self.project_ref]) + self.assertNotEqual(result.exit_code, 0) + + +class TestVideoAnnotate(NativeVideoCliTest): + """`roboflow video annotate` forwards the video-coco document unchanged.""" + + def setUp(self) -> None: + super().setUp() + self.document_path = _write_json(self.tmp.name, "segments.json", VIDEO_COCO_DOCUMENT) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_document_and_defaults_are_forwarded_unchanged(self, mock_annotate) -> None: + mock_annotate.return_value = {"success": True, "inDataset": True, "createdClasses": ["walking"]} + + result = runner.invoke( + app, + ["video", "annotate", "-p", self.project_ref, "-i", "source-9", "-a", self.document_path], + ) + + self.assertEqual(result.exit_code, 0, result.output) + mock_annotate.assert_called_once_with( + "source-9", + VIDEO_COCO_DOCUMENT, + overwrite=False, + split=None, + add_to_dataset=None, + ) + # Native frame/PTS/time-base values survive the read untouched. + sent = mock_annotate.call_args.args[1] + segment = sent["annotations"]["segments"][0] + self.assertEqual(segment["start_pts"], 240) + self.assertEqual(segment["end_pts"], 1920) + self.assertEqual(segment["time_base"], [1, 600]) + self.assertEqual(sent["videos"][0]["fps"], 30000 / 1001) + self.assertEqual(sent["videos"][0]["nb_frames"], 379) + self.assertIn("walking", result.output) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_explicit_overwrite_split_and_membership_are_forwarded(self, mock_annotate) -> None: + mock_annotate.return_value = {"success": True, "inDataset": False} + + result = runner.invoke( + app, + [ + "video", + "annotate", + "-p", + self.project_ref, + "-i", + "source-9", + "-a", + self.document_path, + "--overwrite", + "-s", + "test", + "--no-add-to-dataset", + ], + ) + + self.assertEqual(result.exit_code, 0, result.output) + mock_annotate.assert_called_once_with( + "source-9", + VIDEO_COCO_DOCUMENT, + overwrite=True, + split="test", + add_to_dataset=False, + ) + self.assertIn("In dataset: no", result.output) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_add_to_dataset_true_is_explicit(self, mock_annotate) -> None: + mock_annotate.return_value = {"success": True, "inDataset": True} + + runner.invoke( + app, + ["video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path, "--add-to-dataset"], + ) + + self.assertEqual(mock_annotate.call_args.kwargs["add_to_dataset"], True) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_json_output_is_the_server_response(self, mock_annotate) -> None: + mock_annotate.return_value = {"success": True, "inDataset": True, "createdClasses": []} + + result = runner.invoke( + app, + ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], + ) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(json.loads(result.output), {"success": True, "inDataset": True, "createdClasses": []}) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_conflict_suggests_overwrite(self, mock_annotate) -> None: + from roboflow.adapters.rfapi import AnnotationSaveError + + mock_annotate.side_effect = AnnotationSaveError("segments preserved", status_code=409) + result = runner.invoke( + app, + ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], + ) + + self.assertNotEqual(result.exit_code, 0) + self.assertIn("--overwrite", json.loads(result.output)["error"]["hint"]) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_zero_segment_rejection_is_reported(self, mock_annotate) -> None: + from roboflow.adapters.rfapi import AnnotationSaveError + + mock_annotate.side_effect = AnnotationSaveError("segments must not be empty", status_code=400) + result = runner.invoke( + app, + ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], + ) + + self.assertEqual(result.exit_code, 1) + self.assertIn("segments must not be empty", json.loads(result.output)["error"]["message"]) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_unknown_video_exits_not_found(self, mock_annotate) -> None: + from roboflow.adapters.rfapi import AnnotationSaveError + + mock_annotate.side_effect = AnnotationSaveError("source not found", status_code=404) + result = runner.invoke( + app, + ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], + ) + + self.assertEqual(result.exit_code, 3) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_malformed_json_never_reaches_the_api(self, mock_annotate) -> None: + bad = os.path.join(self.tmp.name, "bad.json") + with open(bad, "w") as handle: + handle.write('{"annotations": ') + + result = runner.invoke(app, ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", bad]) + + self.assertNotEqual(result.exit_code, 0) + mock_annotate.assert_not_called() + self.mock_get_project.assert_not_called() + self.assertIn("Invalid JSON", json.loads(result.output)["error"]["message"]) + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_non_object_document_is_rejected(self, mock_annotate) -> None: + listed = _write_json(self.tmp.name, "list.json", [1, 2, 3]) + + result = runner.invoke(app, ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", listed]) + + self.assertNotEqual(result.exit_code, 0) + mock_annotate.assert_not_called() + + @patch("roboflow.core.project.Project.annotate_video_segments") + def test_missing_file_never_reaches_the_api(self, mock_annotate) -> None: + result = runner.invoke( + app, + [ + "--json", + "video", + "annotate", + "-p", + self.project_ref, + "-i", + "s1", + "-a", + os.path.join(self.tmp.name, "absent.json"), + ], + ) + + self.assertNotEqual(result.exit_code, 0) + mock_annotate.assert_not_called() + + +class TestLegacyVideoContractsIntact(unittest.TestCase): + """The native commands must not disturb legacy video inference.""" + + @patch("roboflow.adapters.rfapi.get_video_job_status") + @patch("roboflow.config.load_roboflow_api_key", return_value="fake-key") + def test_status_still_reads_an_inference_job(self, _mock_key, mock_api) -> None: + mock_api.return_value = {"status": "completed", "progress": "100%"} + result = runner.invoke(app, ["video", "status", "job-legacy"]) + + self.assertEqual(result.exit_code, 0, result.output) + mock_api.assert_called_once_with("fake-key", "job-legacy") + + def test_infer_still_takes_a_version_number(self) -> None: + result = runner.invoke(app, ["video", "infer", "--help"]) + self.assertEqual(result.exit_code, 0) + self.assertIn("--version", result.output) + + if __name__ == "__main__": unittest.main() From 8ac66a1010327234db824400de65ae2cb8d6a1e6 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 11:45:00 -0300 Subject: [PATCH 2/7] [ar-api] Match the real video-coco import schema in CLI fixtures (VID-53) Staging verification showed the import schema puts `segments` at the top level and takes `time_base` as a rational object, with `images`/`annotations` kept empty. The test fixture used a nested `annotations.segments` shape that the API would reject, so it documented something untrue even though the forwarding assertion passed. Rebuild the fixture from the MOV actually submitted to staging. Its presentation timestamps are not frame_index * ticks_per_frame, which is what makes "forwarded unchanged" worth asserting. Also split the upload ValueError hint: a missing path and an unsupported container arrive as the same exception type but need different fixes. Co-Authored-By: Claude Opus 5 (1M context) --- roboflow/cli/handlers/video.py | 11 ++++- tests/cli/test_video_handler.py | 74 ++++++++++++++++++++------------- 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/roboflow/cli/handlers/video.py b/roboflow/cli/handlers/video.py index c86691cf..2b8cad57 100644 --- a/roboflow/cli/handlers/video.py +++ b/roboflow/cli/handlers/video.py @@ -342,7 +342,16 @@ def _video_upload(args) -> None: # noqa: ANN001 wait=False, ) except ValueError as exc: - output_error(args, str(exc), hint="Native video upload accepts original .mp4 and .mov files.") + # The SDK rejects a missing path and an unsupported container with the + # same type, so point each one at its own fix. + missing = "not found" in str(exc) + output_error( + args, + str(exc), + hint="Check the path to the video file." + if missing + else "Native video upload accepts original .mp4 and .mov files.", + ) return except rfapi.RoboflowError as exc: output_error(args, str(exc), hint="Check the project type, your plan limits and the video file.") diff --git a/tests/cli/test_video_handler.py b/tests/cli/test_video_handler.py index 7fab1e11..3e65c2a0 100644 --- a/tests/cli/test_video_handler.py +++ b/tests/cli/test_video_handler.py @@ -78,38 +78,40 @@ def test_status_passes_job_id_to_api(self, _mock_key, mock_api) -> None: } } -# A complete video-coco document with native frame indices, PTS and a rational -# time base. Tests assert it reaches the SDK byte-for-value identical. +# A complete video-coco document in the shape the import schema accepts, taken +# from a real MOV: `segments` is top level, `time_base` is a rational object, +# and `images`/`annotations` stay empty. Tests assert it reaches the SDK with +# every value identical. VIDEO_COCO_DOCUMENT = { - "info": {"version": "roboflow-video-coco"}, + "info": {"format": "roboflow-video-coco"}, "videos": [ { "id": 1, "file_name": "clip.mov", - "fps": 30000 / 1001, - "time_base": [1, 600], - "duration": 12.659047, - "nb_frames": 379, + "width": 1620, + "height": 1080, + "duration": 4.566667, + "fps": 30, + "frame_count": 137, + "time_base": {"numerator": 1, "denominator": 15360}, } ], - "categories": [ - {"id": 0, "name": "actions"}, - {"id": 1, "name": "walking", "supercategory": "actions"}, + "categories": [{"id": 1, "name": "hand_gesture"}], + # Real MOV presentation timestamps are not frame_index * ticks_per_frame, + # so they must survive the read exactly rather than being recomputed. + "segments": [ + { + "id": 1, + "video_id": 1, + "category_id": 1, + "start_frame": 5, + "end_frame": 64, + "start_pts": 3067, + "end_pts": 33275, + } ], - "annotations": { - "segments": [ - { - "id": 1, - "video_id": 1, - "category_id": 1, - "start_frame": 12, - "end_frame": 96, - "start_pts": 240, - "end_pts": 1920, - "time_base": [1, 600], - } - ] - }, + "images": [], + "annotations": [], } @@ -308,6 +310,15 @@ def test_unsupported_extension_is_rejected_by_the_sdk(self, mock_upload) -> None payload = json.loads(result.output) self.assertIn(".mp4", payload["error"]["message"]) + @patch("roboflow.core.project.Project.upload_video") + def test_missing_file_hint_differs_from_bad_container(self, mock_upload) -> None: + mock_upload.side_effect = ValueError("Video file not found: /tmp/absent.mp4") + + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + + self.assertNotEqual(result.exit_code, 0) + self.assertIn("Check the path", json.loads(result.output)["error"]["hint"]) + @patch("roboflow.core.project.Project.upload_video") def test_malformed_metadata_never_reaches_the_api(self, mock_upload) -> None: result = runner.invoke( @@ -434,12 +445,15 @@ def test_document_and_defaults_are_forwarded_unchanged(self, mock_annotate) -> N ) # Native frame/PTS/time-base values survive the read untouched. sent = mock_annotate.call_args.args[1] - segment = sent["annotations"]["segments"][0] - self.assertEqual(segment["start_pts"], 240) - self.assertEqual(segment["end_pts"], 1920) - self.assertEqual(segment["time_base"], [1, 600]) - self.assertEqual(sent["videos"][0]["fps"], 30000 / 1001) - self.assertEqual(sent["videos"][0]["nb_frames"], 379) + segment = sent["segments"][0] + self.assertEqual(segment["start_frame"], 5) + self.assertEqual(segment["end_frame"], 64) + self.assertEqual(segment["start_pts"], 3067) + self.assertEqual(segment["end_pts"], 33275) + video = sent["videos"][0] + self.assertEqual(video["time_base"], {"numerator": 1, "denominator": 15360}) + self.assertEqual(video["duration"], 4.566667) + self.assertEqual(video["frame_count"], 137) self.assertIn("walking", result.output) @patch("roboflow.core.project.Project.annotate_video_segments") From 019edfc584c334cb0b5dd409ce6f8da32a40c0ba Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 12:02:59 -0300 Subject: [PATCH 3/7] [ar-api] Return not-found exit code for waited video status (VID-55) --- roboflow/cli/handlers/video.py | 6 +++++- tests/cli/test_video_handler.py | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/roboflow/cli/handlers/video.py b/roboflow/cli/handlers/video.py index 2b8cad57..5a8a747f 100644 --- a/roboflow/cli/handlers/video.py +++ b/roboflow/cli/handlers/video.py @@ -299,10 +299,14 @@ def _wait_for_upload(args, project, video_id): # noqa: ANN001 output_error(args, str(exc), hint="Use a positive --poll-interval and a nonnegative --poll-timeout.") return None except rfapi.RoboflowError as exc: + not_found = getattr(exc, "status_code", None) == 404 output_error( args, str(exc), - hint=f"Re-check with 'roboflow video upload-status {video_id} -p {args.project}'.", + hint=f"Check the video ID reported by 'roboflow video upload -p {args.project}'." + if not_found + else f"Re-check with 'roboflow video upload-status {video_id} -p {args.project}'.", + exit_code=3 if not_found else 1, ) return None diff --git a/tests/cli/test_video_handler.py b/tests/cli/test_video_handler.py index 3e65c2a0..bf2b1c97 100644 --- a/tests/cli/test_video_handler.py +++ b/tests/cli/test_video_handler.py @@ -412,6 +412,16 @@ def test_unknown_video_exits_not_found(self, mock_status) -> None: self.assertEqual(result.exit_code, 3) + @patch("roboflow.core.project.Project.wait_for_video_upload") + def test_unknown_video_with_wait_exits_not_found(self, mock_wait) -> None: + from roboflow.adapters.rfapi import RoboflowError + + mock_wait.side_effect = RoboflowError("not found", status_code=404) + result = runner.invoke(app, ["--json", "video", "upload-status", "nope", "-p", self.project_ref, "--wait"]) + + self.assertEqual(result.exit_code, 3) + self.assertIn("Check the video ID", json.loads(result.output)["error"]["hint"]) + @patch("roboflow.core.project.Project.get_video_upload_status") def test_failed_state_exits_nonzero(self, mock_status) -> None: mock_status.return_value = {"videoId": "source-3", "status": "failed"} From 2e12845598219c3232b3fa6ae4749522e47f57c8 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 12:07:17 -0300 Subject: [PATCH 4/7] [ar-api] Clarify native video-coco CLI input shape (VID-55) --- CLI-COMMANDS.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CLI-COMMANDS.md b/CLI-COMMANDS.md index 7d92accd..8dd4bbbe 100644 --- a/CLI-COMMANDS.md +++ b/CLI-COMMANDS.md @@ -503,6 +503,12 @@ roboflow video annotate -p my-ar-project -i aBcD1234 -a segments.json --json # { "success": true, "inDataset": true, "createdClasses": ["walking"] } ``` +In `segments.json`, `segments` belongs at the document top level and +`videos[0].time_base` is a rational object such as +`{"numerator": 1, "denominator": 15360}`. `images` and `annotations` may be +omitted; if supplied, each must be an empty array. Use the original video's +probed PTS values rather than deriving them from frame indices or nominal FPS. + Upload accepts `-b/--batch`, `-t/--tag` (comma-separated), `--metadata` (JSON object) and `-s/--split`. Annotate defaults to the API behaviour of adding the Source to the Dataset; override with `--no-add-to-dataset`, set the split with `-s/--split`, and pass `--overwrite` From cd26dd7c066f88ea77987c3cd1055bf16e9f0026 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 14:41:12 -0300 Subject: [PATCH 5/7] [ar-api] Split CLI segment annotation into follow-up (VID-55) --- CLI-COMMANDS.md | 23 +--- roboflow/cli/handlers/video.py | 104 +-------------- tests/cli/test_video_handler.py | 221 +------------------------------- 3 files changed, 7 insertions(+), 341 deletions(-) diff --git a/CLI-COMMANDS.md b/CLI-COMMANDS.md index 8dd4bbbe..ccf463a4 100644 --- a/CLI-COMMANDS.md +++ b/CLI-COMMANDS.md @@ -471,7 +471,7 @@ roboflow workspace stats --start-date 2026-01-01 --end-date 2026-03-31 roboflow universe search "hard hats" --type dataset --limit 5 ``` -### Native video upload and segment annotation +### Native video upload and status Action Recognition projects take whole videos as Sources. `video upload` streams the original MP4/MOV bytes without re-encoding, then reports the **canonical video ID** to use @@ -495,25 +495,8 @@ roboflow video upload-status aBcD1234 -p my-ar-project --json roboflow video upload-status aBcD1234 -p my-ar-project --wait --poll-timeout 120 ``` -```bash -# 3. Annotate segments from a complete roboflow-video-coco document. -# The file is forwarded unchanged, so native frame indices, PTS and -# rational time bases are preserved exactly as authored. -roboflow video annotate -p my-ar-project -i aBcD1234 -a segments.json --json -# { "success": true, "inDataset": true, "createdClasses": ["walking"] } -``` - -In `segments.json`, `segments` belongs at the document top level and -`videos[0].time_base` is a rational object such as -`{"numerator": 1, "denominator": 15360}`. `images` and `annotations` may be -omitted; if supplied, each must be an empty array. Use the original video's -probed PTS values rather than deriving them from frame indices or nominal FPS. - Upload accepts `-b/--batch`, `-t/--tag` (comma-separated), `--metadata` (JSON object) and -`-s/--split`. Annotate defaults to the API behaviour of adding the Source to the Dataset; -override with `--no-add-to-dataset`, set the split with `-s/--split`, and pass `--overwrite` -to replace segments that already differ (otherwise a conflicting save is rejected and an -identical re-submit succeeds). +`-s/--split`. Exit codes follow the CLI contract: `0` success, `1` error, `2` auth, `3` not found. A `failed` ingestion state and a `--wait` timeout both exit nonzero; the timeout message names @@ -617,7 +600,7 @@ Version numbers are always numeric — that's how `x/y` is disambiguated between | `asynctasks` | Inspect async background tasks (e.g. project forks) | | `trash` | List items in Trash | | `universe` | Search Roboflow Universe | -| `video` | Native video upload/status/annotation, and video inference | +| `video` | Native video upload/status and video inference | | `batch` | Batch processing jobs *(coming soon)* | | `completion` | Install or generate shell completion scripts (bash, zsh, fish) | diff --git a/roboflow/cli/handlers/video.py b/roboflow/cli/handlers/video.py index 5a8a747f..99ba621e 100644 --- a/roboflow/cli/handlers/video.py +++ b/roboflow/cli/handlers/video.py @@ -1,4 +1,4 @@ -"""Video commands: native video Source ingestion/annotation and legacy video inference.""" +"""Video commands: native video Source ingestion and legacy video inference.""" from __future__ import annotations @@ -10,7 +10,7 @@ video_app = typer.Typer( cls=SortedGroup, - help="Native video upload/annotation and video inference operations", + help="Native video upload and video inference operations", no_args_is_help=True, ) @@ -59,7 +59,7 @@ def upload( ) -> None: """Upload original video bytes as a native video Source. - Streams the file unchanged and reports the canonical video ID to annotate. + Streams the file unchanged and reports the canonical video ID. """ args = ctx_to_args( ctx, @@ -101,43 +101,6 @@ def upload_status( _video_upload_status(args) -@video_app.command("annotate") -def annotate( - ctx: typer.Context, - annotation_file: Annotated[ - str, typer.Option("-a", "--annotation-file", help="Path to a complete roboflow-video-coco JSON file") - ], - project: Annotated[str, typer.Option("-p", "--project", help="Project ID, or workspace/project")], - video_id: Annotated[str, typer.Option("-i", "--video-id", help="Canonical video ID from 'video upload'")], - add_to_dataset: Annotated[ - Optional[bool], - typer.Option( - "--add-to-dataset/--no-add-to-dataset", - help="Override the API default of adding the Source to the Dataset", - ), - ] = None, - overwrite: Annotated[ - bool, typer.Option("--overwrite", help="Replace different existing segments on this video") - ] = False, - split: Annotated[Optional[str], typer.Option("-s", "--split", help="Dataset split: train, valid or test")] = None, -) -> None: - """Annotate a native video Source's segments from a video-coco file. - - The file is read whole and forwarded unchanged, so native frame indices, - PTS and rational time bases survive exactly as authored. - """ - args = ctx_to_args( - ctx, - annotation_file=annotation_file, - project=project, - video_id=video_id, - add_to_dataset=add_to_dataset, - overwrite=overwrite, - split=split, - ) - _video_annotate(args) - - # --------------------------------------------------------------------------- # Business logic (unchanged from argparse version) # --------------------------------------------------------------------------- @@ -398,64 +361,3 @@ def _video_upload_status(args) -> None: # noqa: ANN001 return _emit_upload_status(args, status) - - -def _video_annotate(args) -> None: # noqa: ANN001 - import json as json_mod - - from roboflow.adapters.rfapi import AnnotationSaveError - from roboflow.cli._output import output, output_error - - try: - with open(args.annotation_file) as handle: - document = json_mod.load(handle) - except OSError as exc: - output_error(args, f"Cannot read annotation file: {exc}", hint="Pass the path to a video-coco JSON file.") - return - except json_mod.JSONDecodeError as exc: - output_error( - args, - f"Invalid JSON in {args.annotation_file}: {exc}", - hint="The file must be one complete roboflow-video-coco document.", - ) - return - - if not isinstance(document, dict): - output_error( - args, - f"{args.annotation_file} must contain a JSON object.", - hint="The file must be one complete roboflow-video-coco document.", - ) - return - - project = _load_project(args) - if project is None: - return - - try: - # `document` is forwarded as parsed: no re-encoding of frames, PTS or time bases. - result = project.annotate_video_segments( - args.video_id, - document, - overwrite=args.overwrite, - split=args.split, - add_to_dataset=args.add_to_dataset, - ) - except AnnotationSaveError as exc: - status_code = getattr(exc, "status_code", None) - if status_code == 409: - hint = "Different segments already exist on this video. Re-run with --overwrite to replace them." - elif status_code == 404: - hint = "Check the canonical video ID from 'roboflow video upload'." - else: - hint = "Check that the document is a complete video-coco with at least one segment." - output_error(args, str(exc), hint=hint, exit_code=3 if status_code == 404 else 1) - return - - lines = [f"Annotated video {args.video_id}."] - if "inDataset" in result: - lines.append(f"In dataset: {'yes' if result.get('inDataset') else 'no'}") - created = result.get("createdClasses") - if created: - lines.append(f"Created classes: {', '.join(map(str, created))}") - output(args, result, text="\n".join(lines)) diff --git a/tests/cli/test_video_handler.py b/tests/cli/test_video_handler.py index bf2b1c97..ad480502 100644 --- a/tests/cli/test_video_handler.py +++ b/tests/cli/test_video_handler.py @@ -78,52 +78,6 @@ def test_status_passes_job_id_to_api(self, _mock_key, mock_api) -> None: } } -# A complete video-coco document in the shape the import schema accepts, taken -# from a real MOV: `segments` is top level, `time_base` is a rational object, -# and `images`/`annotations` stay empty. Tests assert it reaches the SDK with -# every value identical. -VIDEO_COCO_DOCUMENT = { - "info": {"format": "roboflow-video-coco"}, - "videos": [ - { - "id": 1, - "file_name": "clip.mov", - "width": 1620, - "height": 1080, - "duration": 4.566667, - "fps": 30, - "frame_count": 137, - "time_base": {"numerator": 1, "denominator": 15360}, - } - ], - "categories": [{"id": 1, "name": "hand_gesture"}], - # Real MOV presentation timestamps are not frame_index * ticks_per_frame, - # so they must survive the read exactly rather than being recomputed. - "segments": [ - { - "id": 1, - "video_id": 1, - "category_id": 1, - "start_frame": 5, - "end_frame": 64, - "start_pts": 3067, - "end_pts": 33275, - } - ], - "images": [], - "annotations": [], -} - - -def _write_json(directory, name, payload): - import json as json_mod - import os - - path = os.path.join(directory, name) - with open(path, "w") as handle: - json_mod.dump(payload, handle) - return path - class NativeVideoCliTest(unittest.TestCase): """Shared fixtures that let real command dispatch build a real Project.""" @@ -154,7 +108,7 @@ class TestNativeVideoRegistration(NativeVideoCliTest): """The real CLI exposes the native video commands alongside inference.""" def test_native_commands_are_registered(self) -> None: - for command in ("upload", "upload-status", "annotate"): + for command in ("upload", "upload-status"): with self.subTest(command=command): result = runner.invoke(app, ["video", command, "--help"]) self.assertEqual(result.exit_code, 0) @@ -429,179 +383,6 @@ def test_failed_state_exits_nonzero(self, mock_status) -> None: self.assertNotEqual(result.exit_code, 0) -class TestVideoAnnotate(NativeVideoCliTest): - """`roboflow video annotate` forwards the video-coco document unchanged.""" - - def setUp(self) -> None: - super().setUp() - self.document_path = _write_json(self.tmp.name, "segments.json", VIDEO_COCO_DOCUMENT) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_document_and_defaults_are_forwarded_unchanged(self, mock_annotate) -> None: - mock_annotate.return_value = {"success": True, "inDataset": True, "createdClasses": ["walking"]} - - result = runner.invoke( - app, - ["video", "annotate", "-p", self.project_ref, "-i", "source-9", "-a", self.document_path], - ) - - self.assertEqual(result.exit_code, 0, result.output) - mock_annotate.assert_called_once_with( - "source-9", - VIDEO_COCO_DOCUMENT, - overwrite=False, - split=None, - add_to_dataset=None, - ) - # Native frame/PTS/time-base values survive the read untouched. - sent = mock_annotate.call_args.args[1] - segment = sent["segments"][0] - self.assertEqual(segment["start_frame"], 5) - self.assertEqual(segment["end_frame"], 64) - self.assertEqual(segment["start_pts"], 3067) - self.assertEqual(segment["end_pts"], 33275) - video = sent["videos"][0] - self.assertEqual(video["time_base"], {"numerator": 1, "denominator": 15360}) - self.assertEqual(video["duration"], 4.566667) - self.assertEqual(video["frame_count"], 137) - self.assertIn("walking", result.output) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_explicit_overwrite_split_and_membership_are_forwarded(self, mock_annotate) -> None: - mock_annotate.return_value = {"success": True, "inDataset": False} - - result = runner.invoke( - app, - [ - "video", - "annotate", - "-p", - self.project_ref, - "-i", - "source-9", - "-a", - self.document_path, - "--overwrite", - "-s", - "test", - "--no-add-to-dataset", - ], - ) - - self.assertEqual(result.exit_code, 0, result.output) - mock_annotate.assert_called_once_with( - "source-9", - VIDEO_COCO_DOCUMENT, - overwrite=True, - split="test", - add_to_dataset=False, - ) - self.assertIn("In dataset: no", result.output) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_add_to_dataset_true_is_explicit(self, mock_annotate) -> None: - mock_annotate.return_value = {"success": True, "inDataset": True} - - runner.invoke( - app, - ["video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path, "--add-to-dataset"], - ) - - self.assertEqual(mock_annotate.call_args.kwargs["add_to_dataset"], True) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_json_output_is_the_server_response(self, mock_annotate) -> None: - mock_annotate.return_value = {"success": True, "inDataset": True, "createdClasses": []} - - result = runner.invoke( - app, - ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], - ) - - self.assertEqual(result.exit_code, 0, result.output) - self.assertEqual(json.loads(result.output), {"success": True, "inDataset": True, "createdClasses": []}) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_conflict_suggests_overwrite(self, mock_annotate) -> None: - from roboflow.adapters.rfapi import AnnotationSaveError - - mock_annotate.side_effect = AnnotationSaveError("segments preserved", status_code=409) - result = runner.invoke( - app, - ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], - ) - - self.assertNotEqual(result.exit_code, 0) - self.assertIn("--overwrite", json.loads(result.output)["error"]["hint"]) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_zero_segment_rejection_is_reported(self, mock_annotate) -> None: - from roboflow.adapters.rfapi import AnnotationSaveError - - mock_annotate.side_effect = AnnotationSaveError("segments must not be empty", status_code=400) - result = runner.invoke( - app, - ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], - ) - - self.assertEqual(result.exit_code, 1) - self.assertIn("segments must not be empty", json.loads(result.output)["error"]["message"]) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_unknown_video_exits_not_found(self, mock_annotate) -> None: - from roboflow.adapters.rfapi import AnnotationSaveError - - mock_annotate.side_effect = AnnotationSaveError("source not found", status_code=404) - result = runner.invoke( - app, - ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", self.document_path], - ) - - self.assertEqual(result.exit_code, 3) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_malformed_json_never_reaches_the_api(self, mock_annotate) -> None: - bad = os.path.join(self.tmp.name, "bad.json") - with open(bad, "w") as handle: - handle.write('{"annotations": ') - - result = runner.invoke(app, ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", bad]) - - self.assertNotEqual(result.exit_code, 0) - mock_annotate.assert_not_called() - self.mock_get_project.assert_not_called() - self.assertIn("Invalid JSON", json.loads(result.output)["error"]["message"]) - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_non_object_document_is_rejected(self, mock_annotate) -> None: - listed = _write_json(self.tmp.name, "list.json", [1, 2, 3]) - - result = runner.invoke(app, ["--json", "video", "annotate", "-p", self.project_ref, "-i", "s1", "-a", listed]) - - self.assertNotEqual(result.exit_code, 0) - mock_annotate.assert_not_called() - - @patch("roboflow.core.project.Project.annotate_video_segments") - def test_missing_file_never_reaches_the_api(self, mock_annotate) -> None: - result = runner.invoke( - app, - [ - "--json", - "video", - "annotate", - "-p", - self.project_ref, - "-i", - "s1", - "-a", - os.path.join(self.tmp.name, "absent.json"), - ], - ) - - self.assertNotEqual(result.exit_code, 0) - mock_annotate.assert_not_called() - - class TestLegacyVideoContractsIntact(unittest.TestCase): """The native commands must not disturb legacy video inference.""" From e251764954edf28440baabe71c1899ae59a066e9 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 15:09:21 -0300 Subject: [PATCH 6/7] [ar-api] Map native video API errors to the CLI exit-code contract (VID-53) Upload, upload-status and the project lookup now go through the shared output_api_error helper, so a rejected API key exits 2 and only a real 404 exits 3. get_project carries the HTTP status so the lookup can tell them apart. Fold duplicated video CLI tests into table-driven cases. Co-Authored-By: Claude Opus 5.5 --- roboflow/adapters/rfapi.py | 2 +- roboflow/cli/handlers/video.py | 36 +++---- tests/cli/test_video_handler.py | 166 ++++++++++++++------------------ 3 files changed, 88 insertions(+), 116 deletions(-) diff --git a/roboflow/adapters/rfapi.py b/roboflow/adapters/rfapi.py index 6a4b1cd4..40ca178b 100644 --- a/roboflow/adapters/rfapi.py +++ b/roboflow/adapters/rfapi.py @@ -54,7 +54,7 @@ def get_project(api_key, workspace_url, project_url): url = f"{API_URL}/{workspace_url}/{project_url}?api_key={api_key}" response = requests.get(url) if response.status_code != 200: - raise RoboflowError(response.text) + raise RoboflowError(response.text, status_code=response.status_code) result = response.json() return result diff --git a/roboflow/cli/handlers/video.py b/roboflow/cli/handlers/video.py index 99ba621e..f1cdd9b3 100644 --- a/roboflow/cli/handlers/video.py +++ b/roboflow/cli/handlers/video.py @@ -192,7 +192,7 @@ def _video_status(args) -> None: # noqa: ANN001 def _load_project(args): # noqa: ANN001 """Load the project for a native video command, honoring CLI credential precedence.""" from roboflow.adapters import rfapi - from roboflow.cli._output import output_error + from roboflow.cli._output import output_api_error from roboflow.cli._resolver import resolve_project_context resolved = resolve_project_context(args) @@ -203,11 +203,10 @@ def _load_project(args): # noqa: ANN001 try: data = rfapi.get_project(api_key, workspace, project_slug) except rfapi.RoboflowError as exc: - output_error( + output_api_error( args, - str(exc), + exc, hint=f"Check that project '{workspace}/{project_slug}' exists and your API key can read it.", - exit_code=3, ) return None @@ -216,6 +215,10 @@ def _load_project(args): # noqa: ANN001 return Project(api_key, data["project"]) +def _unknown_video_hint(args) -> str: # noqa: ANN001 + return f"Check the video ID reported by 'roboflow video upload -p {args.project}'." + + def _emit_upload_status(args, status) -> None: # noqa: ANN001 """Render an ingestion status, exiting nonzero when the upload failed.""" from roboflow.cli._output import output, output_error @@ -250,7 +253,7 @@ def _emit_upload_status(args, status) -> None: # noqa: ANN001 def _wait_for_upload(args, project, video_id): # noqa: ANN001 """Bounded wait, reporting the video ID so a timeout stays actionable.""" from roboflow.adapters import rfapi - from roboflow.cli._output import output_error + from roboflow.cli._output import output_api_error, output_error try: return project.wait_for_video_upload( @@ -262,14 +265,11 @@ def _wait_for_upload(args, project, video_id): # noqa: ANN001 output_error(args, str(exc), hint="Use a positive --poll-interval and a nonnegative --poll-timeout.") return None except rfapi.RoboflowError as exc: - not_found = getattr(exc, "status_code", None) == 404 - output_error( + output_api_error( args, - str(exc), - hint=f"Check the video ID reported by 'roboflow video upload -p {args.project}'." - if not_found - else f"Re-check with 'roboflow video upload-status {video_id} -p {args.project}'.", - exit_code=3 if not_found else 1, + exc, + hint=f"Re-check with 'roboflow video upload-status {video_id} -p {args.project}'.", + not_found_hint=_unknown_video_hint(args), ) return None @@ -335,7 +335,7 @@ def _video_upload(args) -> None: # noqa: ANN001 def _video_upload_status(args) -> None: # noqa: ANN001 from roboflow.adapters import rfapi - from roboflow.cli._output import output_error + from roboflow.cli._output import output_api_error project = _load_project(args) if project is None: @@ -349,15 +349,7 @@ def _video_upload_status(args) -> None: # noqa: ANN001 try: status = project.get_video_upload_status(args.video_id) except rfapi.RoboflowError as exc: - not_found = getattr(exc, "status_code", None) == 404 - output_error( - args, - str(exc), - hint=f"Check the video ID reported by 'roboflow video upload -p {args.project}'." - if not_found - else None, - exit_code=3 if not_found else 1, - ) + output_api_error(args, exc, not_found_hint=_unknown_video_hint(args)) return _emit_upload_status(args, status) diff --git a/tests/cli/test_video_handler.py b/tests/cli/test_video_handler.py index ad480502..e8b194f1 100644 --- a/tests/cli/test_video_handler.py +++ b/tests/cli/test_video_handler.py @@ -113,13 +113,6 @@ def test_native_commands_are_registered(self) -> None: result = runner.invoke(app, ["video", command, "--help"]) self.assertEqual(result.exit_code, 0) - def test_upload_status_is_distinct_from_inference_status(self) -> None: - group = runner.invoke(app, ["video", "--help"]) - self.assertEqual(group.exit_code, 0) - self.assertIn("upload-status", group.output) - # The legacy inference job command keeps its own name and contract. - self.assertIn("status", group.output) - class TestVideoUpload(NativeVideoCliTest): """`roboflow video upload` streams original bytes and reports canonical IDs.""" @@ -151,6 +144,10 @@ def test_forwards_all_options_and_waits_by_default(self, mock_upload, mock_wait) '{"camera": "one"}', "-s", "valid", + "--poll-interval", + "0.5", + "--poll-timeout", + "30", ], ) @@ -164,7 +161,7 @@ def test_forwards_all_options_and_waits_by_default(self, mock_upload, mock_wait) wait=False, ) # The bounded wait continues on the ID the first status reported. - mock_wait.assert_called_once_with("upload-1", poll_interval=2.0, poll_timeout=300.0) + mock_wait.assert_called_once_with("upload-1", poll_interval=0.5, poll_timeout=30.0) self.assertIn("source-9", result.output) self.assertIn("uploaded", result.output) @@ -201,31 +198,6 @@ def test_terminal_dedup_status_skips_the_wait(self, mock_upload, mock_wait) -> N self.assertIs(data["duplicate"], True) self.assertIsNone(data["resolvedBatch"]) - @patch("roboflow.core.project.Project.wait_for_video_upload") - @patch("roboflow.core.project.Project.upload_video") - def test_custom_poll_bounds_are_forwarded(self, mock_upload, mock_wait) -> None: - mock_upload.return_value = {"videoId": "upload-1", "status": "pending"} - mock_wait.return_value = {"videoId": "upload-1", "status": "uploaded"} - - result = runner.invoke( - app, - [ - "video", - "upload", - "-p", - self.project_ref, - "-f", - self.video_path, - "--poll-interval", - "0.5", - "--poll-timeout", - "30", - ], - ) - - self.assertEqual(result.exit_code, 0, result.output) - mock_wait.assert_called_once_with("upload-1", poll_interval=0.5, poll_timeout=30.0) - @patch("roboflow.core.project.Project.upload_video") def test_failed_processing_exits_nonzero(self, mock_upload) -> None: mock_upload.return_value = {"videoId": "upload-1", "status": "failed"} @@ -255,45 +227,48 @@ def test_wait_timeout_names_the_video_id_to_recheck(self, mock_upload) -> None: self.assertIn("upload-status upload-7", payload["error"]["hint"]) @patch("roboflow.core.project.Project.upload_video") - def test_unsupported_extension_is_rejected_by_the_sdk(self, mock_upload) -> None: - mock_upload.side_effect = ValueError("Native video upload accepts .mp4 and .mov files") - - result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) - - self.assertNotEqual(result.exit_code, 0) - payload = json.loads(result.output) - self.assertIn(".mp4", payload["error"]["message"]) + def test_sdk_file_rejections_get_their_own_hint(self, mock_upload) -> None: + cases = [ + ("Native video upload accepts .mp4 and .mov files", "accepts original .mp4 and .mov"), + ("Video file not found: /tmp/absent.mp4", "Check the path"), + ] + for message, hint in cases: + with self.subTest(message=message): + mock_upload.side_effect = ValueError(message) + + result = runner.invoke( + app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path] + ) + + self.assertEqual(result.exit_code, 1) + error = json.loads(result.output)["error"] + self.assertEqual(error["message"], message) + self.assertIn(hint, error["hint"]) @patch("roboflow.core.project.Project.upload_video") - def test_missing_file_hint_differs_from_bad_container(self, mock_upload) -> None: - mock_upload.side_effect = ValueError("Video file not found: /tmp/absent.mp4") - - result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) - - self.assertNotEqual(result.exit_code, 0) - self.assertIn("Check the path", json.loads(result.output)["error"]["hint"]) - - @patch("roboflow.core.project.Project.upload_video") - def test_malformed_metadata_never_reaches_the_api(self, mock_upload) -> None: - result = runner.invoke( - app, - ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path, "--metadata", "{not json"], - ) - - self.assertNotEqual(result.exit_code, 0) + def test_invalid_metadata_never_reaches_the_api(self, mock_upload) -> None: + cases = [("{not json", "Invalid metadata JSON"), ("[1, 2]", "Metadata must be a JSON object")] + for metadata, message in cases: + with self.subTest(metadata=metadata): + result = runner.invoke( + app, + [ + "--json", + "video", + "upload", + "-p", + self.project_ref, + "-f", + self.video_path, + "--metadata", + metadata, + ], + ) + + self.assertEqual(result.exit_code, 1) + self.assertIn(message, json.loads(result.output)["error"]["message"]) mock_upload.assert_not_called() self.mock_get_project.assert_not_called() - self.assertIn("Invalid metadata JSON", json.loads(result.output)["error"]["message"]) - - @patch("roboflow.core.project.Project.upload_video") - def test_non_object_metadata_is_rejected(self, mock_upload) -> None: - result = runner.invoke( - app, - ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path, "--metadata", "[1, 2]"], - ) - - self.assertNotEqual(result.exit_code, 0) - mock_upload.assert_not_called() @patch("roboflow.core.project.Project.upload_video") def test_server_error_exits_nonzero(self, mock_upload) -> None: @@ -305,6 +280,20 @@ def test_server_error_exits_nonzero(self, mock_upload) -> None: self.assertNotEqual(result.exit_code, 0) self.assertIn("quota exceeded", json.loads(result.output)["error"]["message"]) + def test_project_lookup_failure_follows_exit_code_contract(self) -> None: + from roboflow.adapters.rfapi import RoboflowError + + for status_code, exit_code in ((401, 2), (404, 3), (500, 1)): + with self.subTest(status_code=status_code): + self.mock_get_project.side_effect = RoboflowError("project lookup failed", status_code=status_code) + + result = runner.invoke( + app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path] + ) + + self.assertEqual(result.exit_code, exit_code) + self.assertIn("project lookup failed", json.loads(result.output)["error"]["message"]) + def test_missing_api_key_exits_with_auth_code(self) -> None: with patch("roboflow.config.load_roboflow_api_key", return_value=None): result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) @@ -357,24 +346,24 @@ def test_pending_state_points_at_the_recheck_command(self, mock_status) -> None: self.assertEqual(result.exit_code, 0, result.output) self.assertIn("upload-status source-3", result.output) - @patch("roboflow.core.project.Project.get_video_upload_status") - def test_unknown_video_exits_not_found(self, mock_status) -> None: + def test_api_errors_follow_exit_code_contract(self) -> None: from roboflow.adapters.rfapi import RoboflowError - mock_status.side_effect = RoboflowError("not found", status_code=404) - result = runner.invoke(app, ["--json", "video", "upload-status", "nope", "-p", self.project_ref]) - - self.assertEqual(result.exit_code, 3) - - @patch("roboflow.core.project.Project.wait_for_video_upload") - def test_unknown_video_with_wait_exits_not_found(self, mock_wait) -> None: - from roboflow.adapters.rfapi import RoboflowError - - mock_wait.side_effect = RoboflowError("not found", status_code=404) - result = runner.invoke(app, ["--json", "video", "upload-status", "nope", "-p", self.project_ref, "--wait"]) - - self.assertEqual(result.exit_code, 3) - self.assertIn("Check the video ID", json.loads(result.output)["error"]["hint"]) + reads = [("get_video_upload_status", []), ("wait_for_video_upload", ["--wait"])] + for method, flags in reads: + for status_code, exit_code in ((401, 2), (404, 3), (500, 1)): + with self.subTest(method=method, status_code=status_code): + with patch( + f"roboflow.core.project.Project.{method}", + side_effect=RoboflowError("status read failed", status_code=status_code), + ): + result = runner.invoke( + app, ["--json", "video", "upload-status", "nope", "-p", self.project_ref, *flags] + ) + + self.assertEqual(result.exit_code, exit_code) + if status_code == 404: + self.assertIn("Check the video ID", json.loads(result.output)["error"]["hint"]) @patch("roboflow.core.project.Project.get_video_upload_status") def test_failed_state_exits_nonzero(self, mock_status) -> None: @@ -386,15 +375,6 @@ def test_failed_state_exits_nonzero(self, mock_status) -> None: class TestLegacyVideoContractsIntact(unittest.TestCase): """The native commands must not disturb legacy video inference.""" - @patch("roboflow.adapters.rfapi.get_video_job_status") - @patch("roboflow.config.load_roboflow_api_key", return_value="fake-key") - def test_status_still_reads_an_inference_job(self, _mock_key, mock_api) -> None: - mock_api.return_value = {"status": "completed", "progress": "100%"} - result = runner.invoke(app, ["video", "status", "job-legacy"]) - - self.assertEqual(result.exit_code, 0, result.output) - mock_api.assert_called_once_with("fake-key", "job-legacy") - def test_infer_still_takes_a_version_number(self) -> None: result = runner.invoke(app, ["video", "infer", "--help"]) self.assertEqual(result.exit_code, 0) From beadcddb9cd83b00d20c9692c76e54d50d250b59 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbosa Date: Mon, 5 Oct 2026 15:25:36 -0300 Subject: [PATCH 7/7] [ar-api] Validate video upload inputs before storing bytes (VID-53) Bad --poll-interval/--poll-timeout values and a missing file are now rejected before any network call, so an upload never stores bytes and then reports failure without an ID. Upload API errors follow the exit-code contract, and their hint no longer blames the file when the failing call came after the PUT. Co-Authored-By: Claude Opus 5.5 --- roboflow/cli/handlers/video.py | 50 ++++++++++++++++++--------- tests/cli/test_video_handler.py | 61 ++++++++++++++++++++++++--------- 2 files changed, 79 insertions(+), 32 deletions(-) diff --git a/roboflow/cli/handlers/video.py b/roboflow/cli/handlers/video.py index f1cdd9b3..02b909ae 100644 --- a/roboflow/cli/handlers/video.py +++ b/roboflow/cli/handlers/video.py @@ -250,10 +250,24 @@ def _emit_upload_status(args, status) -> None: # noqa: ANN001 output(args, status, text="\n".join(lines)) +def _poll_bounds_are_valid(args) -> bool: # noqa: ANN001 + """Reject bad wait bounds before any network call, so an upload never starts and then fails.""" + from roboflow.cli._output import output_error + + if args.wait and (args.poll_interval <= 0 or args.poll_timeout < 0): + output_error( + args, + f"Invalid wait bounds: --poll-interval {args.poll_interval}, --poll-timeout {args.poll_timeout}.", + hint="Use a positive --poll-interval and a nonnegative --poll-timeout.", + ) + return False + return True + + def _wait_for_upload(args, project, video_id): # noqa: ANN001 """Bounded wait, reporting the video ID so a timeout stays actionable.""" from roboflow.adapters import rfapi - from roboflow.cli._output import output_api_error, output_error + from roboflow.cli._output import output_api_error try: return project.wait_for_video_upload( @@ -261,9 +275,6 @@ def _wait_for_upload(args, project, video_id): # noqa: ANN001 poll_interval=args.poll_interval, poll_timeout=args.poll_timeout, ) - except ValueError as exc: - output_error(args, str(exc), hint="Use a positive --poll-interval and a nonnegative --poll-timeout.") - return None except rfapi.RoboflowError as exc: output_api_error( args, @@ -276,9 +287,16 @@ def _wait_for_upload(args, project, video_id): # noqa: ANN001 def _video_upload(args) -> None: # noqa: ANN001 import json as json_mod + import os from roboflow.adapters import rfapi - from roboflow.cli._output import output_error + from roboflow.cli._output import output_api_error, output_error + + if not os.path.isfile(args.video_file): + output_error(args, f"Video file not found: {args.video_file}", hint="Check the path to the video file.") + return + if not _poll_bounds_are_valid(args): + return metadata = None if args.metadata: @@ -309,19 +327,17 @@ def _video_upload(args) -> None: # noqa: ANN001 wait=False, ) except ValueError as exc: - # The SDK rejects a missing path and an unsupported container with the - # same type, so point each one at its own fix. - missing = "not found" in str(exc) - output_error( - args, - str(exc), - hint="Check the path to the video file." - if missing - else "Native video upload accepts original .mp4 and .mov files.", - ) + output_error(args, str(exc), hint="Native video upload accepts original .mp4 and .mov files.") return except rfapi.RoboflowError as exc: - output_error(args, str(exc), hint="Check the project type, your plan limits and the video file.") + # The failing call may come after the bytes were stored; a re-upload then + # deduplicates onto that Source instead of creating a second one. + output_api_error( + args, + exc, + hint="Check the project type and plan limits. If the bytes were already stored, " + "re-uploading the same file reuses that Source.", + ) return video_id = status.get("videoId") @@ -337,6 +353,8 @@ def _video_upload_status(args) -> None: # noqa: ANN001 from roboflow.adapters import rfapi from roboflow.cli._output import output_api_error + if not _poll_bounds_are_valid(args): + return project = _load_project(args) if project is None: return diff --git a/tests/cli/test_video_handler.py b/tests/cli/test_video_handler.py index e8b194f1..9995d681 100644 --- a/tests/cli/test_video_handler.py +++ b/tests/cli/test_video_handler.py @@ -227,23 +227,33 @@ def test_wait_timeout_names_the_video_id_to_recheck(self, mock_upload) -> None: self.assertIn("upload-status upload-7", payload["error"]["hint"]) @patch("roboflow.core.project.Project.upload_video") - def test_sdk_file_rejections_get_their_own_hint(self, mock_upload) -> None: + def test_local_rejections_never_reach_the_api(self, mock_upload) -> None: + absent = os.path.join(self.tmp.name, "absent.mp4") cases = [ - ("Native video upload accepts .mp4 and .mov files", "accepts original .mp4 and .mov"), - ("Video file not found: /tmp/absent.mp4", "Check the path"), + (["-f", absent], "Video file not found", "Check the path"), + # A bad bound must fail before the upload stores bytes it then cannot report. + (["-f", self.video_path, "--poll-interval", "0"], "Invalid wait bounds", "positive --poll-interval"), + (["-f", self.video_path, "--poll-timeout", "-1"], "Invalid wait bounds", "nonnegative --poll-timeout"), ] - for message, hint in cases: - with self.subTest(message=message): - mock_upload.side_effect = ValueError(message) - - result = runner.invoke( - app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path] - ) + for flags, message, hint in cases: + with self.subTest(flags=flags): + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, *flags]) self.assertEqual(result.exit_code, 1) error = json.loads(result.output)["error"] - self.assertEqual(error["message"], message) + self.assertIn(message, error["message"]) self.assertIn(hint, error["hint"]) + mock_upload.assert_not_called() + self.mock_get_project.assert_not_called() + + @patch("roboflow.core.project.Project.upload_video") + def test_unsupported_container_gets_its_own_hint(self, mock_upload) -> None: + mock_upload.side_effect = ValueError("Native video upload accepts .mp4 and .mov files") + + result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + + self.assertEqual(result.exit_code, 1) + self.assertIn("accepts original .mp4 and .mov", json.loads(result.output)["error"]["hint"]) @patch("roboflow.core.project.Project.upload_video") def test_invalid_metadata_never_reaches_the_api(self, mock_upload) -> None: @@ -271,14 +281,23 @@ def test_invalid_metadata_never_reaches_the_api(self, mock_upload) -> None: self.mock_get_project.assert_not_called() @patch("roboflow.core.project.Project.upload_video") - def test_server_error_exits_nonzero(self, mock_upload) -> None: + def test_upload_api_errors_follow_exit_code_contract(self, mock_upload) -> None: from roboflow.adapters.rfapi import RoboflowError - mock_upload.side_effect = RoboflowError("quota exceeded") - result = runner.invoke(app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path]) + for status_code, exit_code in ((401, 2), (404, 3), (503, 1), (None, 1)): + with self.subTest(status_code=status_code): + mock_upload.side_effect = RoboflowError("upload failed", status_code=status_code) - self.assertNotEqual(result.exit_code, 0) - self.assertIn("quota exceeded", json.loads(result.output)["error"]["message"]) + result = runner.invoke( + app, ["--json", "video", "upload", "-p", self.project_ref, "-f", self.video_path] + ) + + self.assertEqual(result.exit_code, exit_code) + error = json.loads(result.output)["error"] + self.assertEqual(error["message"], "upload failed") + if status_code != 401: + # A failure after the PUT must not send the user hunting for a file problem. + self.assertIn("re-uploading the same file reuses that Source", error["hint"]) def test_project_lookup_failure_follows_exit_code_contract(self) -> None: from roboflow.adapters.rfapi import RoboflowError @@ -346,6 +365,16 @@ def test_pending_state_points_at_the_recheck_command(self, mock_status) -> None: self.assertEqual(result.exit_code, 0, result.output) self.assertIn("upload-status source-3", result.output) + @patch("roboflow.core.project.Project.wait_for_video_upload") + def test_invalid_wait_bounds_never_reach_the_api(self, mock_wait) -> None: + result = runner.invoke( + app, ["--json", "video", "upload-status", "v1", "-p", self.project_ref, "--wait", "--poll-interval", "0"] + ) + + self.assertEqual(result.exit_code, 1) + mock_wait.assert_not_called() + self.mock_get_project.assert_not_called() + def test_api_errors_follow_exit_code_contract(self) -> None: from roboflow.adapters.rfapi import RoboflowError