Skip to content

Raise / exit non-zero on failed model and directory uploads (#545) - #554

Closed
tonylampada wants to merge 2 commits into
mainfrom
bc/SLM-102
Closed

tonylampada wants to merge 2 commits into
mainfrom
bc/SLM-102

Conversation

@tonylampada

@tonylampada tonylampada commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Fixes #545: failed model and directory uploads were printed and swallowed, so scripts and agents driving the SDK/CLI saw success (exit 0, "status": "uploaded" / "imported") for uploads that never landed. Serves objective O8 (one canonical API shape on every surface): SDK and CLI now report the same failure the API returned.

  • Version.deploy() / Workspace.deploy_model() raise RoboflowError (HTTP status + server body) when the upload URL request or the signed PUT fails. The hardcoded 429 "already has a trained model" text is gone; the server's own message is surfaced (Feature: Multiple Models Per Version #497 made it possibly stale).
  • Workspace.upload_dataset() per-image flow returns {"uploaded", "duplicates", "failed": [{path, stage, error}]} instead of None. Failed images still don't raise.
  • roboflow model upload exits 1 with a structured error. roboflow image upload <dir> exits 1 when any image or annotation failed (failed_count + list in the JSON error); a zip upload whose final status is failed exits 1. count is now the real uploaded+duplicate count, not files on disk.

Type of change

  • Bug fix (behavior change: callers that relied on deploy() never raising now get an exception)

How has this change been tested?

Any specific deployment considerations

None; ships with the next SDK release. Changelog should note the new deploy() exception and upload_dataset() return value.

Next step: 1 of 1 for #545. Out of scope: SDK progress lines on stdout under --json (separate card).

PLAN
touches   SDK model upload (Version.deploy / Workspace.deploy_model) → raise RoboflowError instead of print+return
touches   SDK Workspace.upload_dataset per-image flow → summary dict instead of None (zip flow unchanged)
touches   CLI model upload → exit 1 + structured error
touches   CLI image upload <dir> → exit 1 w/ failed count; zip status failed → exit 1
tests     model upload URL 500/429/PUT failure raise · tests/test_version.py, tests/test_workspace.py
tests     upload_dataset summary with failures · tests/test_project.py
tests     CLI model upload failure exits 1 · tests/cli/test_model_handler.py
tests     CLI image upload dir partial/total failure, zip failed · tests/cli/test_image_handler.py
review    CLI agent experience: exit codes + {"error":{message,hint}} — CLI lines
review    Backwards compat: deploy() now raises, upload_dataset return value — SDK lines
verify    python -m unittest → 1245 OK; issue #545 reproducer → RoboflowError

🤖 Generated with Claude Code

…load (#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 <dir>` 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 <noreply@anthropic.com>
@tonylampada

tonylampada commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Deliver checks (dm-brain playbooks/deliver):

  • Own review + testing: diff re-read against plan block; every tests line has a passing run (gist green.txt, red on main in red.txt, full suite 1244 OK)
  • Evidence gist: https://gist.github.com/tonylampada/8f4c8aeb49b9798d81f44c18f6772535 (evidence block per criterion)
  • PR description: contract sections, <40 lines, objective O8 named
  • Review: no bot on this repo (lieutenant ruling); local jarbas-review round done, see comment below

Comment thread roboflow/core/version.py
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}")
Comment thread roboflow/core/version.py
Comment on lines +694 to +696
"Share your model with the world at:"
f" {UNIVERSE_URL}/{self.workspace}/{self.project}/"
f"model/{self.version}"
Comment on lines +971 to +972
f"View the status of your deployment for project {project_id} at:"
f" {APP_URL}/{self.url}/{project_id}/models"
@tonylampada

Copy link
Copy Markdown
Collaborator Author

CI status:

  • build (*) fails at make check_code_quality only: ruff 0.17 ICN001 in roboflow/core/project.py:1 and roboflow/cli/handlers/project.py:204, files this PR doesn't touch. Every branch hits it since the ruff release; fixed by fix: satisfy ruff 0.17 ICN001 for datetime imports #551. Tests pass locally (1244 OK).
  • CodeQL "clear-text logging" (version.py:691/694, workspace.py:971): these are the pre-existing "View the status of your deployment at …" success prints, moved by the refactor. They print workspace/project/version slugs, not the API key. False positive; same lines exist on main.

… 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 <noreply@anthropic.com>
@tonylampada

Copy link
Copy Markdown
Collaborator Author

Review round (local jarbas-review, one round):

  1. ⚠️ image.py: zip vs per-image result was told apart by the absence of a failed key, so a zip status carrying a failed field would crash. Fixed in the latest commit: check for status instead. New test test_zip_status_with_failed_field_is_not_mistaken_for_per_image.
  2. 🤨 image.py: "failed to upload" was misleading for annotation-stage failures. Fixed: now "failed to import", and stage is still reported per file.
  3. 🤨 Network errors (requests.ConnectionError) on the upload URL request/PUT aren't wrapped in RoboflowError: won't fix, out of scope: same as before this PR and consistent with the rest of the SDK; the CLI already catches them.
  4. 🤨 res.json()["url"] on a malformed 200 gives KeyError/ValueError: won't fix, out of scope: pre-existing, and Failed uploads are reported as success: Version.deploy() / Workspace.deploy_model() don't raise, and roboflow model upload / roboflow image upload <dir> exit 0 #545 is about non-2xx responses.
  5. 🤨 model upload errors carry no hint: won't fix, out of scope: the generic catch in the handler is pre-existing; the message now names the HTTP status and server reason, which is the actionable part.
  6. 🤨 An unrecognized image status (neither success nor duplicate) is counted as uploaded: won't fix, out of scope: that's the pre-existing [LOG ERROR] path; upload_image raises on real failures.

Full suite after fixes: 1245 OK (gist updated). CI red is the pre-existing ruff 0.17 ICN001 in untouched files (fixed by #551), not this PR.

@tonylampada

Copy link
Copy Markdown
Collaborator Author

Night-loop drill run; closed by Selma. Real fix for #545 is under review in #549.

@tonylampada
tonylampada deleted the bc/SLM-102 branch October 14, 2026 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants