From 9f9aac6d9cdfa6c1c06cb7c61eeb3c55cf8e1fd7 Mon Sep 17 00:00:00 2001 From: Tony Lampada Date: Tue, 13 Oct 2026 22:15:16 -0300 Subject: [PATCH 1/2] Raise on failed model uploads; report failed images from directory upload (#545) Version.deploy() and Workspace.deploy_model() now raise RoboflowError when the upload URL request or the signed PUT fails, instead of printing and returning. Workspace.upload_dataset() per-image flow returns a summary (uploaded, duplicates, failed). `roboflow model upload` and `roboflow image upload ` exit 1 with a structured error naming what failed; a zip upload whose final status is failed also exits 1. Co-Authored-By: Claude Opus 5.5 --- roboflow/cli/handlers/image.py | 53 +++++++++++++++---- roboflow/core/version.py | 54 +++++++------------ roboflow/core/workspace.py | 58 ++++++++++---------- tests/cli/test_image_handler.py | 94 ++++++++++++++++++++++++++++++++- tests/cli/test_model_handler.py | 75 ++++++++++++++++++++++++++ tests/test_project.py | 41 +++++++++++++- tests/test_version.py | 46 ++++++++++++++++ tests/test_workspace.py | 44 +++++++++++++++ 8 files changed, 387 insertions(+), 78 deletions(-) diff --git a/roboflow/cli/handlers/image.py b/roboflow/cli/handlers/image.py index 8d3a2786..2ca95817 100644 --- a/roboflow/cli/handlers/image.py +++ b/roboflow/cli/handlers/image.py @@ -356,7 +356,7 @@ def _handle_upload_single(args, api_key: str, path: str) -> None: # noqa: ANN00 def _handle_upload_directory(args, api_key: str, path: str) -> None: # noqa: ANN001 - import os + import json import roboflow from roboflow.cli._output import output, output_error, suppress_sdk_output @@ -393,11 +393,20 @@ def _handle_upload_directory(args, api_key: str, path: str) -> None: # noqa: AN output_error(args, str(exc)) return - if isinstance(result, dict): + if "failed" not in result: + # Zip flow returns the server's task status status = result.get("status", "unknown") + task_id = result.get("task_id") or result.get("taskId") + if status == "failed": + output_error( + args, + f"Zip upload of {path} to {args.project} failed (task_id={task_id}): {result.get('error') or result}", + hint="Check the dataset format, then retry; run without --zip-upload to see per-image errors.", + ) + return data = { "status": status, - "task_id": result.get("task_id") or result.get("taskId"), + "task_id": task_id, "path": path, "project": args.project, "result": result, @@ -405,15 +414,37 @@ def _handle_upload_directory(args, api_key: str, path: str) -> None: # noqa: AN output(args, data, text=f"Imported {path} to {args.project} (zip upload, status={status})") return - # Per-image fallback — count files via image extensions - count = 0 - image_exts = {".jpg", ".jpeg", ".png", ".bmp", ".gif", ".tiff", ".webp"} - for root, _dirs, files in os.walk(path): - for f in files: - if os.path.splitext(f)[1].lower() in image_exts: - count += 1 + failed = result["failed"] + if failed: + total = len(failed) + result["uploaded"] + result["duplicates"] + lines = [f" {f['path']} ({f['stage']}): {f['error']}" for f in failed[:10]] + if len(failed) > 10: + lines.append(f" ... and {len(failed) - 10} more") + message = f"{len(failed)} of {total} images failed to upload from {path} to {args.project}:\n" + "\n".join( + lines + ) + error = { + "message": message, + "uploaded": result["uploaded"], + "duplicates": result["duplicates"], + "failed_count": len(failed), + "failed": failed, + } + output_error( + args, + json.dumps(error), + hint="Fix the listed files and re-run; images already uploaded are skipped as duplicates.", + ) + return - data = {"status": "imported", "path": path, "count": count} + count = result["uploaded"] + result["duplicates"] + data = { + "status": "imported", + "path": path, + "count": count, + "uploaded": result["uploaded"], + "duplicates": result["duplicates"], + } output(args, data, text=f"Imported {count} images from {path} to {args.project}") diff --git a/roboflow/core/version.py b/roboflow/core/version.py index a4633a3b..a6f80507 100644 --- a/roboflow/core/version.py +++ b/roboflow/core/version.py @@ -13,6 +13,7 @@ from tqdm import tqdm from roboflow.adapters import rfapi +from roboflow.adapters.rfapi import RoboflowError from roboflow.config import ( API_URL, APP_URL, @@ -662,6 +663,9 @@ def deploy(self, model_type: str, model_path: str, filename: str = "weights/best model_type (str): The type of the model to be deployed. model_path (str): File path to the model weights to be uploaded. filename (str, optional): The name of the weights file. Defaults to "weights/best.pt". + + Raises: + RoboflowError: If the upload URL cannot be obtained or the upload fails. """ bundle = package_custom_weights_interactive(model_type, model_path, filename, build_dir=model_path) @@ -676,41 +680,21 @@ def _upload_zip(self, model_type: str, model_path: str, model_file_name: str): f"{API_URL}/{self.workspace}/{self.project}/{self.version}" f"/uploadModel?api_key={self.__api_key}&modelType={model_type}&nocache=true" ) - try: - if res.status_code == 429: - raise RuntimeError( - "This version already has a trained model. Please generate and" - " train a new version in order to upload model to Roboflow." - ) - else: - res.raise_for_status() - except Exception as e: - print(f"An error occured when getting the model upload URL: {e}") - return - - res = requests.put( - res.json()["url"], - data=open(os.path.join(model_path, model_file_name), "rb"), - ) - try: - res.raise_for_status() - - if self.public: - print( - f"View the status of your deployment at: {APP_URL}/{self.workspace}/{self.project}/{self.version}" - ) - print( - "Share your model with the world at:" - f" {UNIVERSE_URL}/{self.workspace}/{self.project}/" - f"model/{self.version}" - ) - else: - print( - f"View the status of your deployment at: {APP_URL}/{self.workspace}/{self.project}/{self.version}" - ) - - except Exception as e: - print(f"An error occured when uploading the model: {e}") + if not res.ok: + raise RoboflowError(f"Failed to get the model upload URL (HTTP {res.status_code}): {res.text}") + + with open(os.path.join(model_path, model_file_name), "rb") as f: + res = requests.put(res.json()["url"], data=f) + if not res.ok: + raise RoboflowError(f"Failed to upload the model (HTTP {res.status_code}): {res.text}") + + print(f"View the status of your deployment at: {APP_URL}/{self.workspace}/{self.project}/{self.version}") + if self.public: + print( + "Share your model with the world at:" + f" {UNIVERSE_URL}/{self.workspace}/{self.project}/" + f"model/{self.version}" + ) def __download_zip(self, link, location, format): """ diff --git a/roboflow/core/workspace.py b/roboflow/core/workspace.py index 081e9ac6..aecd0412 100644 --- a/roboflow/core/workspace.py +++ b/roboflow/core/workspace.py @@ -521,7 +521,7 @@ def upload_dataset( wait: bool = True, poll_interval: float = 5.0, poll_timeout: float = 3600.0, - ) -> Optional[dict]: + ) -> dict: """ Upload a dataset to Roboflow. @@ -549,7 +549,9 @@ def upload_dataset( poll_timeout (float, optional): zip flow only — total seconds to wait before timing out. Returns: - dict | None: zip flow returns the final/pending status dict; per-image flow returns None. + dict: zip flow returns the final/pending status dict. Per-image flow returns a summary: + ``{"uploaded": int, "duplicates": int, "failed": [{"path", "stage", "error"}]}``, + where ``stage`` is ``"image"`` or ``"annotation"``. Failed images do not raise. """ # noqa: E501 // docs if dataset_format != "NOT_USED": print("Warning: parameter 'dataset_format' is deprecated and will be removed in a future release") @@ -703,22 +705,31 @@ def _upload(imagedesc): image_id = image["id"] annotation, annotation_time = _save_annotation(image_id, imagedesc) _log_img_upload(image_path, image, annotation, image_upload_time, image_retry_attempts, annotation_time) + return "duplicate" if image.get("duplicate") else "uploaded", None except ImageUploadError as e: retry_attempts = f" (with {e.retries} retries)" if e.retries > 0 else "" print(f"[ERR]{retry_attempts} {image_path} ({e.message})") + return "failed", {"path": image_path, "stage": "image", "error": e.message} except AnnotationSaveError as e: upload_time_str = f"[{image_upload_time:.1f}s]" retry_attempts = f" (with {image_retry_attempts} retries)" if image_retry_attempts > 0 else "" image_msg = f"[UPLOADED]{retry_attempts} {image_path} ({image_id}) {upload_time_str}" annotation_msg = f"annotations = ERR: {e.message}" print(f"{image_msg} / {annotation_msg}") + return "failed", {"path": image_path, "stage": "annotation", "error": e.message} except Exception as e: print(f"[ERR] {image_path} ({e})") + return "failed", {"path": image_path, "stage": "image", "error": str(e)} with concurrent.futures.ThreadPoolExecutor(max_workers=num_workers) as executor: - list(executor.map(_upload, images)) + results = list(executor.map(_upload, images)) - return None + statuses = [status for status, _ in results] + return { + "uploaded": statuses.count("uploaded"), + "duplicates": statuses.count("duplicate"), + "failed": [failure for _, failure in results if failure], + } def _get_or_create_project(self, project_id, license: str = "MIT", type: str = "object-detection"): try: @@ -907,6 +918,9 @@ def deploy_model( model_path (str): File path to the model weights to be uploaded. project_ids (list[str]): List of project IDs to deploy the model to. filename (str, optional): The name of the weights file. Defaults to "weights/best.pt". + + Raises: + RoboflowError: If the upload URL cannot be obtained or the upload fails. """ from roboflow.util.model_processor import ( @@ -943,34 +957,20 @@ def _upload_zip( res = requests.post( f"{API_URL}/{self.url}/models/prepareUpload?api_key={self.__api_key}&modelType={model_type}&modelName={model_name}&projectIds={','.join(project_ids)}&nocache=true" ) - try: - res.raise_for_status() - except Exception as e: - error_message = str(e) - status_code = str(res.status_code) - - print("\n\033[91m❌ ERROR\033[0m: Failed to get model deployment URL") - print("\033[93mDetails\033[0m:", error_message) - print("\033[93mStatus\033[0m:", status_code) - print(f"\033[93mResponse\033[0m:\n{res.text}\n") - return + if not res.ok: + raise RoboflowError(f"Failed to get the model upload URL (HTTP {res.status_code}): {res.text}") # Upload the model to the signed URL - res = requests.put( - res.json()["url"], - data=open(os.path.join(model_path, model_file_name), "rb"), - ) - try: - res.raise_for_status() - - for project_id in project_ids: - print( - f"View the status of your deployment for project {project_id} at:" - f" {APP_URL}/{self.url}/{project_id}/models" - ) + with open(os.path.join(model_path, model_file_name), "rb") as f: + res = requests.put(res.json()["url"], data=f) + if not res.ok: + raise RoboflowError(f"Failed to upload the model (HTTP {res.status_code}): {res.text}") - except Exception as e: - print(f"An error occured when uploading the model: {e}") + for project_id in project_ids: + print( + f"View the status of your deployment for project {project_id} at:" + f" {APP_URL}/{self.url}/{project_id}/models" + ) def search( self, diff --git a/tests/cli/test_image_handler.py b/tests/cli/test_image_handler.py index 33769444..b9d9ec86 100644 --- a/tests/cli/test_image_handler.py +++ b/tests/cli/test_image_handler.py @@ -3,15 +3,20 @@ import io import json import os +import shutil import sys import tempfile import types import unittest from unittest.mock import MagicMock, patch +import responses from typer.testing import CliRunner +from roboflow.adapters.rfapi import ImageUploadError from roboflow.cli import app +from roboflow.config import API_URL +from tests import PROJECT_NAME, ROBOFLOW_API_KEY, WORKSPACE_NAME, RoboflowTest runner = CliRunner() @@ -155,6 +160,7 @@ def test_upload_directory(self, mock_rf_cls): f.write("x") mock_ws = MagicMock() + mock_ws.upload_dataset.return_value = {"uploaded": 1, "duplicates": 1, "failed": []} mock_rf_cls.return_value.workspace.return_value = mock_ws args = _make_args( @@ -315,8 +321,7 @@ def test_zip_upload_flag_defaults_false(self, mock_rf_cls): with tempfile.TemporaryDirectory() as tmpdir: mock_ws = MagicMock() - # MagicMock return → not a dict → per-image output branch - mock_ws.upload_dataset.return_value = None + mock_ws.upload_dataset.return_value = {"uploaded": 0, "duplicates": 0, "failed": []} mock_rf_cls.return_value.workspace.return_value = mock_ws args = _make_args( @@ -800,3 +805,88 @@ def test_metadata_batch_over_limit(self): if __name__ == "__main__": unittest.main() + + +class TestImageUploadDirectoryFailures(RoboflowTest): + """Failed directory uploads exit non-zero with a failure count (#545).""" + + def setUp(self): + super().setUp() + rf_patch = patch("roboflow.Roboflow") + rf_patch.start().return_value.workspace.return_value = self.workspace + self.addCleanup(rf_patch.stop) + self.tmpdir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmpdir) + for name in ["a.jpg", "b.jpg", "c.jpg"]: + with open(os.path.join(self.tmpdir, name), "wb") as f: + f.write(b"x") + + def _invoke(self): + return runner.invoke( + app, + ["--json", "--api-key", ROBOFLOW_API_KEY, "image", "upload", self.tmpdir, "-p", PROJECT_NAME, "-c", "1"], + ) + + def test_every_image_fails(self): + with patch("roboflow.core.project.Project.upload_image", side_effect=ImageUploadError("Invalid image")): + result = self._invoke() + + self.assertEqual(result.exit_code, 1) + error = json.loads(result.stderr.strip().splitlines()[-1])["error"] + self.assertTrue(error["message"].startswith("3 of 3 images failed to upload")) + self.assertEqual(error["failed_count"], 3) + self.assertEqual(error["uploaded"], 0) + self.assertEqual({f["error"] for f in error["failed"]}, {"Invalid image"}) + self.assertIn("hint", error) + self.assertNotIn('"status"', result.stdout) + + def test_partial_failure_reports_count(self): + def upload_image(image_path, **kwargs): + if image_path.endswith("b.jpg"): + raise ImageUploadError("Invalid image") + return {"id": image_path, "success": True}, 0.1, 0 + + with patch("roboflow.core.project.Project.upload_image", side_effect=upload_image): + result = self._invoke() + + self.assertEqual(result.exit_code, 1) + error = json.loads(result.stderr.strip().splitlines()[-1])["error"] + self.assertEqual(error["failed_count"], 1) + self.assertEqual(error["uploaded"], 2) + self.assertTrue(error["failed"][0]["path"].endswith("b.jpg")) + + def test_all_uploaded_exits_zero(self): + image = ({"id": "img", "success": True}, 0.1, 0) + with patch("roboflow.core.project.Project.upload_image", return_value=image): + result = self._invoke() + + self.assertEqual(result.exit_code, 0, result.stderr) + # SDK progress lines precede the indented JSON document on stdout + data = json.loads(result.stdout[result.stdout.index("\n{") + 1 :]) + self.assertEqual(data["status"], "imported") + self.assertEqual(data["count"], 3) + self.assertEqual(data["uploaded"], 3) + + def test_zip_upload_failed_status_exits_nonzero(self): + zip_path = os.path.join(self.tmpdir, "dataset.zip") + with open(zip_path, "wb") as f: + f.write(b"PK") + responses.add( + responses.POST, + f"{API_URL}/{WORKSPACE_NAME}/{PROJECT_NAME}/upload/zip", + json={"taskId": "t1", "signedUrl": "https://storage.example.com/zip"}, + ) + responses.add(responses.PUT, "https://storage.example.com/zip") + responses.add( + responses.GET, + f"{API_URL}/{WORKSPACE_NAME}/upload/zip/t1", + json={"status": "failed", "error": "No images found in zip"}, + ) + + result = runner.invoke( + app, ["--json", "--api-key", ROBOFLOW_API_KEY, "image", "upload", zip_path, "-p", PROJECT_NAME] + ) + + self.assertEqual(result.exit_code, 1, result.stdout) + error = json.loads(result.stderr.strip().splitlines()[-1])["error"] + self.assertIn("No images found in zip", error["message"]) diff --git a/tests/cli/test_model_handler.py b/tests/cli/test_model_handler.py index f4ca2f2d..0472be1c 100644 --- a/tests/cli/test_model_handler.py +++ b/tests/cli/test_model_handler.py @@ -2,14 +2,20 @@ import io import json +import os +import shutil import sys +import tempfile import types import unittest from unittest.mock import MagicMock, patch +import responses from typer.testing import CliRunner from roboflow.cli import app +from roboflow.config import API_URL +from tests import PROJECT_NAME, PROJECT_VERSION, ROBOFLOW_API_KEY, WORKSPACE_NAME, RoboflowTest runner = CliRunner() @@ -398,3 +404,72 @@ def test_list_with_group_uses_public_endpoint(self, mock_list: MagicMock) -> Non if __name__ == "__main__": unittest.main() + + +class TestModelUploadFailures(RoboflowTest): + """`roboflow model upload` exits non-zero when the upload fails (#545).""" + + signed_url = "https://storage.example.com/signed-upload" + + def setUp(self): + super().setUp() + rf_patch = patch("roboflow.Roboflow") + rf_patch.start().return_value.workspace.return_value = self.workspace + self.addCleanup(rf_patch.stop) + self.model_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.model_dir) + with open(os.path.join(self.model_dir, "roboflow_deploy.zip"), "wb") as f: + f.write(b"weights") + bundle = types.SimpleNamespace( + model_type="yolov8n", archive_path=types.SimpleNamespace(name="roboflow_deploy.zip") + ) + for target in ( + "roboflow.core.version.package_custom_weights_interactive", + "roboflow.util.model_processor.package_custom_weights_interactive", + ): + packager = patch(target, return_value=bundle) + packager.start() + self.addCleanup(packager.stop) + + def _invoke(self, *extra: str): + return runner.invoke( + app, + ["--json", "--api-key", ROBOFLOW_API_KEY, "model", "upload", "-p", PROJECT_NAME] + + ["-t", "yolov8n", "-m", self.model_dir, *extra], + ) + + def test_version_upload_url_failure(self) -> None: + responses.add( + responses.GET, + f"{API_URL}/{WORKSPACE_NAME}/{PROJECT_NAME}/{PROJECT_VERSION}/uploadModel", + status=500, + body="internal error", + ) + result = self._invoke("-v", PROJECT_VERSION) + + self.assertEqual(result.exit_code, 1) + self.assertNotIn("uploaded", result.stdout) + error = json.loads(result.stderr)["error"] + self.assertIn("Failed to get the model upload URL (HTTP 500)", error["message"]) + + def test_workspace_signed_upload_failure(self) -> None: + responses.add(responses.POST, f"{API_URL}/{WORKSPACE_NAME}/models/prepareUpload", json={"url": self.signed_url}) + responses.add(responses.PUT, self.signed_url, status=403, body="AccessDenied") + result = self._invoke("-n", "my-model") + + self.assertEqual(result.exit_code, 1) + self.assertNotIn("uploaded", result.stdout) + error = json.loads(result.stderr)["error"] + self.assertIn("Failed to upload the model (HTTP 403): AccessDenied", error["message"]) + + def test_version_upload_success(self) -> None: + responses.add( + responses.GET, + f"{API_URL}/{WORKSPACE_NAME}/{PROJECT_NAME}/{PROJECT_VERSION}/uploadModel", + json={"url": self.signed_url}, + ) + responses.add(responses.PUT, self.signed_url) + result = self._invoke("-v", PROJECT_VERSION) + + self.assertEqual(result.exit_code, 0, result.stderr) + self.assertIn('"status": "uploaded"', result.stdout) diff --git a/tests/test_project.py b/tests/test_project.py index c8685d40..673bab5c 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -78,6 +78,45 @@ def _setup_upload_dataset_mocks( return mocks + def test_upload_dataset_returns_summary_with_failures(self): + dataset = self._create_test_dataset( + [ + {"file": "ok.jpg", "split": "train"}, + {"file": "dup.jpg", "split": "train"}, + {"file": "bad.jpg", "split": "train"}, + {"file": "bad-ann.jpg", "split": "train", "annotationfile": {"file": "bad-ann.xml"}}, + ] + ) + + def upload_image(image_path, **kwargs): + if image_path.endswith("/bad.jpg"): + raise ImageUploadError("Invalid image") + duplicate = image_path.endswith("/dup.jpg") + return {"id": image_path, "success": not duplicate, "duplicate": duplicate}, 0.1, 0 + + mocks = self._setup_upload_dataset_mocks( + test_dataset=dataset, + upload_image_side_effect=upload_image, + save_annotation_side_effect=AnnotationSaveError("Bad annotation"), + ) + for mock in mocks.values(): + mock.start() + try: + result = self.workspace.upload_dataset("/test/dataset", PROJECT_NAME, num_workers=1) + finally: + for mock in mocks.values(): + mock.stop() + + self.assertEqual(result["uploaded"], 1) + self.assertEqual(result["duplicates"], 1) + self.assertEqual( + result["failed"], + [ + {"path": "/test/location/bad.jpg", "stage": "image", "error": "Invalid image"}, + {"path": "/test/location/bad-ann.jpg", "stage": "annotation", "error": "Bad annotation"}, + ], + ) + def test_check_valid_image_with_accepted_formats(self): images_to_test = [ "rabbit.JPG", @@ -1104,7 +1143,7 @@ def test_directory_default_stays_on_per_image(self): started = {name: m.start() for name, m in {**rfapi_mocks, **per_image}.items()} try: result = self.workspace.upload_dataset(dataset_path=src_dir, project_name=PROJECT_NAME) - self.assertIsNone(result) + self.assertEqual(result, {"uploaded": 0, "duplicates": 0, "failed": []}) started["init"].assert_not_called() started["parser"].assert_called_once() finally: diff --git a/tests/test_version.py b/tests/test_version.py index d00ae4cc..c41e8281 100644 --- a/tests/test_version.py +++ b/tests/test_version.py @@ -1,5 +1,7 @@ import json import os +import shutil +import tempfile import unittest from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -20,6 +22,7 @@ from roboflow.core.training import Training from roboflow.core.version import Version, unwrap_version_id from roboflow.models.object_detection import ObjectDetectionModel +from tests import PROJECT_NAME, PROJECT_VERSION, WORKSPACE_NAME, RoboflowTest from tests.helpers import get_version @@ -537,3 +540,46 @@ def test_explicit_recipe_is_forwarded_without_template_fetch(self): self.assertEqual(body["modelType"], "cosmos3-edge") self.assertEqual(body["trainRecipe"], recipe) self.assertEqual(training.status, "queued") + + +class TestDeployUploadFailures(RoboflowTest): + """Version.deploy() raises when the model upload fails (#545).""" + + upload_url = f"{API_URL}/{WORKSPACE_NAME}/{PROJECT_NAME}/{PROJECT_VERSION}/uploadModel" + signed_url = "https://storage.example.com/signed-upload" + + def setUp(self): + super().setUp() + self.model_dir = tempfile.mkdtemp() + with open(os.path.join(self.model_dir, "roboflow_deploy.zip"), "wb") as f: + f.write(b"weights") + bundle = SimpleNamespace(model_type="yolov8n", archive_path=SimpleNamespace(name="roboflow_deploy.zip")) + packager = patch("roboflow.core.version.package_custom_weights_interactive", return_value=bundle) + packager.start() + self.addCleanup(packager.stop) + self.addCleanup(shutil.rmtree, self.model_dir) + + def _deploy(self): + return self.version.deploy("yolov8n", self.model_dir, "weights/best.pt") + + def test_raises_when_upload_url_request_fails(self): + responses.add(responses.GET, self.upload_url, status=500, body="internal error") + with self.assertRaisesRegex(rfapi.RoboflowError, r"model upload URL \(HTTP 500\): internal error"): + self._deploy() + + def test_raises_with_server_message_on_429(self): + responses.add(responses.GET, self.upload_url, status=429, json={"error": "already has a model"}) + with self.assertRaisesRegex(rfapi.RoboflowError, r"HTTP 429.*already has a model"): + self._deploy() + + def test_raises_when_signed_upload_fails(self): + responses.add(responses.GET, self.upload_url, json={"url": self.signed_url}) + responses.add(responses.PUT, self.signed_url, status=403, body="AccessDenied") + with self.assertRaisesRegex(rfapi.RoboflowError, r"upload the model \(HTTP 403\): AccessDenied"): + self._deploy() + + def test_returns_normally_on_success(self): + responses.add(responses.GET, self.upload_url, json={"url": self.signed_url}) + responses.add(responses.PUT, self.signed_url, status=200) + self.assertIsNone(self._deploy()) + self.assertEqual(responses.calls[-1].request.body, b"weights") diff --git a/tests/test_workspace.py b/tests/test_workspace.py index 5c33b5b9..0ac47508 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -1,12 +1,19 @@ """Unit tests for module-level helpers in roboflow.core.workspace.""" import os +import shutil import tempfile import unittest import zipfile +from types import SimpleNamespace from unittest.mock import patch +import responses + +from roboflow.adapters.rfapi import RoboflowError +from roboflow.config import API_URL from roboflow.core.workspace import Workspace, _zip_directory +from tests import PROJECT_NAME, WORKSPACE_NAME, RoboflowTest def _make_workspace(): @@ -178,3 +185,40 @@ def test_batch_update_wait_falls_back_to_task_id_poll(self, mock_batch, mock_get if __name__ == "__main__": unittest.main() + + +class TestDeployModelUploadFailures(RoboflowTest): + """Workspace.deploy_model() raises when the model upload fails (#545).""" + + prepare_url = f"{API_URL}/{WORKSPACE_NAME}/models/prepareUpload" + signed_url = "https://storage.example.com/signed-upload" + + def setUp(self): + super().setUp() + self.model_dir = tempfile.mkdtemp() + with open(os.path.join(self.model_dir, "roboflow_deploy.zip"), "wb") as f: + f.write(b"weights") + bundle = SimpleNamespace(model_type="yolov8n", archive_path=SimpleNamespace(name="roboflow_deploy.zip")) + packager = patch("roboflow.util.model_processor.package_custom_weights_interactive", return_value=bundle) + packager.start() + self.addCleanup(packager.stop) + self.addCleanup(shutil.rmtree, self.model_dir) + + def _deploy(self): + return self.workspace.deploy_model("yolov8n", self.model_dir, [PROJECT_NAME], "my-model") + + def test_raises_when_prepare_upload_fails(self): + responses.add(responses.POST, self.prepare_url, status=400, json={"error": "bad model type"}) + with self.assertRaisesRegex(RoboflowError, r"model upload URL \(HTTP 400\).*bad model type"): + self._deploy() + + def test_raises_when_signed_upload_fails(self): + responses.add(responses.POST, self.prepare_url, json={"url": self.signed_url}) + responses.add(responses.PUT, self.signed_url, status=500, body="boom") + with self.assertRaisesRegex(RoboflowError, r"upload the model \(HTTP 500\): boom"): + self._deploy() + + def test_returns_normally_on_success(self): + responses.add(responses.POST, self.prepare_url, json={"url": self.signed_url}) + responses.add(responses.PUT, self.signed_url, status=200) + self.assertIsNone(self._deploy()) From 0e715071a951bf7af58176361351cd02d2da38e0 Mon Sep 17 00:00:00 2001 From: Tony Lampada Date: Tue, 13 Oct 2026 22:21:45 -0300 Subject: [PATCH 2/2] Detect zip vs per-image upload result by status key; say 'import' for annotation failures Review round (jarbas-review): a zip status carrying a 'failed' field would have been read as the per-image summary and crashed. Co-Authored-By: Claude Opus 5.5 --- roboflow/cli/handlers/image.py | 6 +++--- tests/cli/test_image_handler.py | 27 ++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/roboflow/cli/handlers/image.py b/roboflow/cli/handlers/image.py index 2ca95817..a68fdeac 100644 --- a/roboflow/cli/handlers/image.py +++ b/roboflow/cli/handlers/image.py @@ -393,8 +393,8 @@ def _handle_upload_directory(args, api_key: str, path: str) -> None: # noqa: AN output_error(args, str(exc)) return - if "failed" not in result: - # Zip flow returns the server's task status + if "status" in result: + # Zip flow returns the server's task status; the per-image summary has no "status" status = result.get("status", "unknown") task_id = result.get("task_id") or result.get("taskId") if status == "failed": @@ -420,7 +420,7 @@ def _handle_upload_directory(args, api_key: str, path: str) -> None: # noqa: AN lines = [f" {f['path']} ({f['stage']}): {f['error']}" for f in failed[:10]] if len(failed) > 10: lines.append(f" ... and {len(failed) - 10} more") - message = f"{len(failed)} of {total} images failed to upload from {path} to {args.project}:\n" + "\n".join( + message = f"{len(failed)} of {total} images failed to import from {path} to {args.project}:\n" + "\n".join( lines ) error = { diff --git a/tests/cli/test_image_handler.py b/tests/cli/test_image_handler.py index b9d9ec86..6456f768 100644 --- a/tests/cli/test_image_handler.py +++ b/tests/cli/test_image_handler.py @@ -411,6 +411,7 @@ def test_upload_directory_omits_default_split_when_not_explicit(self, mock_rf_cl with tempfile.TemporaryDirectory() as tmpdir: mock_ws = MagicMock() + mock_ws.upload_dataset.return_value = {"uploaded": 0, "duplicates": 0, "failed": []} mock_rf_cls.return_value.workspace.return_value = mock_ws args = _make_args( @@ -445,6 +446,7 @@ def test_upload_directory_forwards_explicit_split(self, mock_rf_cls): with tempfile.TemporaryDirectory() as tmpdir: mock_ws = MagicMock() + mock_ws.upload_dataset.return_value = {"uploaded": 0, "duplicates": 0, "failed": []} mock_rf_cls.return_value.workspace.return_value = mock_ws args = _make_args( @@ -833,7 +835,7 @@ def test_every_image_fails(self): self.assertEqual(result.exit_code, 1) error = json.loads(result.stderr.strip().splitlines()[-1])["error"] - self.assertTrue(error["message"].startswith("3 of 3 images failed to upload")) + self.assertTrue(error["message"].startswith("3 of 3 images failed to import")) self.assertEqual(error["failed_count"], 3) self.assertEqual(error["uploaded"], 0) self.assertEqual({f["error"] for f in error["failed"]}, {"Invalid image"}) @@ -890,3 +892,26 @@ def test_zip_upload_failed_status_exits_nonzero(self): self.assertEqual(result.exit_code, 1, result.stdout) error = json.loads(result.stderr.strip().splitlines()[-1])["error"] self.assertIn("No images found in zip", error["message"]) + + def test_zip_status_with_failed_field_is_not_mistaken_for_per_image(self): + zip_path = os.path.join(self.tmpdir, "dataset.zip") + with open(zip_path, "wb") as f: + f.write(b"PK") + responses.add( + responses.POST, + f"{API_URL}/{WORKSPACE_NAME}/{PROJECT_NAME}/upload/zip", + json={"taskId": "t1", "signedUrl": "https://storage.example.com/zip"}, + ) + responses.add(responses.PUT, "https://storage.example.com/zip") + responses.add( + responses.GET, + f"{API_URL}/{WORKSPACE_NAME}/upload/zip/t1", + json={"status": "completed", "failed": 0}, + ) + + result = runner.invoke( + app, ["--json", "--api-key", ROBOFLOW_API_KEY, "image", "upload", zip_path, "-p", PROJECT_NAME] + ) + + self.assertEqual(result.exit_code, 0, result.stderr) + self.assertIn('"status": "completed"', result.stdout)