Add Universe project fork CLI#473
Conversation
imbgar-roboflow
left a comment
There was a problem hiding this comment.
Review: Universe Project Fork CLI
Solid feature shape. The fork/poll split, the generic asynctasks namespace, and the SDK/CLI layering all look right. Flagging some runtime bugs and contract gaps before merge — most are small fixes.
Blockers
1. Falsy-zero progress bug — cli/handlers/asynctasks.py:227, cli/handlers/project.py:345
progress.get("current") or progress.get("completed") treats current=0 as falsy and silently falls through. A task starting at 0/100 skips the current key entirely. Use progress["current"] if "current" in progress else progress.get("completed").
2. Terminal status set too narrow — core/async_tasks.py:407
{"completed", "failed"} only. If the server ever returns cancelled/canceled/error/timed_out, the helper polls forever (and --timeout 0 disables the timeout). Either expand the set or invert: poll while status is in {pending, running, queued, created} and treat anything else as terminal.
3. progress non-dict crashes the wait loop — core/async_tasks.py, handlers
The dev-reference doc allows progress to be object | number | null. SDK does status.get("progress") or {} then .get(...), which throws AttributeError on a numeric value. Guard with isinstance(progress, dict).
4. URL path segments are not encoded — adapters/rfapi.py:62-63, 81-82
f"{API_URL}/{workspace_url}/asynctasks/{task_id}" is a raw f-string. A task_id containing /, .., ?, or # (or a copy-pasted one from a hostile source) silently mutates the request path with the user's API key attached. Wrap segments with urllib.parse.quote(value, safe="").
5. Polling URL is rebuilt locally instead of trusting the server — cli/handlers/project.py:335-339
The server returns {"taskId", "url"}, but the SDK ignores url and rebuilds polling from API_URL. In any env where API_URL ≠ the host the server returned (e.g. tests/manual/uselocal points to localapi.roboflow.one), polling silently hits the wrong host. Prefer the server-supplied url; fall back to local construction.
6. on_update not invoked on the terminal tick — core/async_tasks.py:407-409
Terminal-check returns before on_update, so the final progress event (typically current==total) is never delivered. A progress bar driven by the callback finishes at N-1/N. Move on_update(status) above the terminal check, or document the off-by-one.
Major
7. Both url and source_project_slug silently coexist — adapters/rfapi.py:807-815
No validation; both keys land in the payload if both kwargs are passed. Add an XOR guard: if (url is None) == (source_project_slug is None): raise ValueError(...).
8. 200 success path is fragile — adapters/rfapi.py:807
(200, 202) are both treated as success, but enqueued["taskId"] (project.py:335) raises KeyError if 200 means a synchronous result with no taskId. Either reject 200, or branch on if "taskId" in enqueued.
9. CLI cannot pass source_project_slug — cli/handlers/project.py:260-279
SDK exposes the kwarg; the CLI handler only forwards source as url=. Asymmetric surface — either drop the kwarg from Workspace.fork_project (let the server parse everything from url) or add a --source-slug flag.
10. --no-wait text mode drops the polling URL — cli/handlers/project.py:338
The most actionable field is enqueued["url"], but text output is just Fork enqueued: taskId=…. Append the URL.
11. Raw server text echoed to terminal — adapters/rfapi.py:68, 86
RoboflowError(response.text) flows into output_error(args, str(exc)) and prints server-controlled bytes to a TTY with no ANSI/CR stripping. A misbehaving server can inject escapes / clear-screen / fake prompts. Sanitize before printing (strip \x00-\x1f except \n\t).
Test gaps worth backfilling
progress.current=0(blocker #1) — uncovered.pendingstatus withprogress: None— fires but isn't asserted-silent.fork_projectwith both args / neither arg — uncovered.- HTTP
200success path — only202is tested. poll_until_terminal(timeout=0)— disabled-timeout branch unexercised.task_idURL-encoding — only substring-matched intests/adapters/test_rfapi_phase2.py:509.- The timeout test (
tests/cli/test_asynctasks_handler.py:671) is fragile —mock_monotonic.side_effect=[0.0, 0.5, 99999.0]silently breaks if anyone adds atime.monotonic()call. Counter-based callable would be sturdier. - No end-to-end Typer invocation — all handler tests call
_get_async_task/_fork_projectdirectly, bypassingctx_to_argsand the registration incli/__init__.py:209. Add arunner.invoke(app, ["asynctasks", "get", "task-1"], ...)withrfapimocked.
Nits
core/async_tasks.pysleepsintervalseconds after the deadline check passes, so elapsed time can exceed--timeoutby ~interval. Move the deadline check to the top or clamp the sleep to remaining time.cli/handlers/project.py:307empty-source exits with code1; sister branches (no workspace, no api key) use2. Standardize.CLI-COMMANDS.md:21has a double space:roboflow asynctasks get <task-id>.- Slug example mismatch with the docs PR: this PR's
CLI-COMMANDS.md:14usesleo-ueno-uduc7/license-plate-recognition;roboflow-dev-reference#11usesroboflow-jvuqo/football-players-detection-3zvbc. Pick one.
Happy to look again once these are addressed.
|
The points not addressed:
|
…ace poll URL - #6 poll_until_terminal: invoke on_update before terminal check so the final progress event (typically current==total) is delivered to callers driving a progress bar. - #8 fork_project: switch the success guard from an explicit (200, 202) list to response.ok so the SDK doesn't break if the contract ever broadens (e.g. 200 sync, 201 created). - #10 project fork --no-wait text: append the server-supplied polling URL so the user can poll later without rebuilding it. - Widen NON_TERMINAL_STATUSES to include pending/queued/in_progress to match the dev-reference enum (backend currently emits a subset).
Description
Adds SDK and CLI support for asynchronously forking public Universe projects into a workspace, plus generic async task inspection commands for polling fork status.
Fix: Adds
project fork,asynctasks get,asynctasks wait, SDK methods for project fork/task status, shared polling helper, docs, and tests.Type of change
How has this change been tested?
python -m unittest tests.cli.test_project_fork_handler tests.cli.test_asynctasks_handler tests.adapters.test_rfapi_phase2 tests.test_workspacesource tests/manual/uselocal && roboflow project fork https://universe.roboflow.one/leo-ueno-uduc7/license-plate-recognitionWill the change affect Universe?
No frontend Universe change. This adds SDK/CLI support for the backend fork API.
Any specific deployment considerations
None for the Python SDK.
Docs
CLI-COMMANDS.md