Reviewing cli-cloud-tooling changes
Scope
Triggers on changes under inference_cli/ except inference_cli/server.py, inference_cli/lib/container_adapter.py, tunnel_adapter.py, infer_adapter.py (server/container/tunnel lifecycle — different skill). In scope:
inference_cli/benchmark.py,inference_cli/workflows.py,inference_cli/cloud.py,inference_cli/main.pyinference_cli/lib/cloud_adapter.py— SkyPilot cloud-deploy backing thecloudapp.inference_cli/lib/benchmark/**,inference_cli/lib/benchmark_adapter.pyinference_cli/lib/workflows/**(core, common, entities, local/remote image + video adapters)inference_cli/lib/roboflow_cloud/**(common.py,config.py,errors.py,core.py,data_staging/**,batch_processing/**)inference_cli/lib/enterprise/**(inference-compiler TRT extension)- Companion tests under
tests/inference_cli/**and CLI docs underdocs/inference_helpers/cli_commands/**.
OUT of scope: the four lifecycle adapters above; the Workflows Execution Engine internals and blocks (separate skill); inference_sdk.
Two distinct cloud surfaces live here — do not conflate them:
cloudapp (inference_cli/cloud.py→inference_cli/lib/cloud_adapter.py): SkyPilot VM deploy (deploy/status/start/stop/undeploy). No Roboflow HTTP calls; older option/error style (see below).rf-cloudapp (inference_cli/lib/roboflow_cloud/**): data-staging + batch-processing against the Roboflow API. This is where the HTTP/retry/pydantic-alias contracts apply.
Review checklist
BLOCK — must be fixed before merge:
- HTTP: a new
rf-cloudAPI op that does NOT go throughget_workspace()+handle_response_errors(operation_name=...), is not decorated@backoff.on_exception(backoff.constant, RetryError, max_tries=3, interval=1), does not passtimeout=REQUEST_TIMEOUT, or does not re-raise connectivity/TimeoutasRetryError. Raising a bareExceptionwhere retry logic expectsRetryErrorbreaks retries. - Wire schema: a request/response field whose
serialization_alias(outbound camelCase) oralias(inbound) does not match the backend contract, or a casual rename of an existing alias. - Breaking CLI contract: renaming/removing a
typer.Optionlong or short flag, a command name, or narrowing a default. A colliding short flag within one command silently shadows (-oreuse, #2038). - Benchmark integrity: fabricating metrics on the error path (
execution_time=0counted a failure as a zero-latency success, #1150).
FLAG — reviewer should raise:
- New network command missing
api_keyresolution fromROBOFLOW_API_KEY, a--debug-mode/--no-debug-modeflag, or the standardrf-clouderror envelope (KeyboardInterrupt → exit 2; else debug re-raise / echo + exit 1). - New request capability not threaded through all three layers:
Optional[...] = Noneentity field →api_operationsparam →core.pycommand option (#2399). - Closed choice set typed as a free-form
strinstead of astr, Enum(#2062). - Magic number hardcoded in
api_operationsinstead of anos.getenv-backed constant inconfig.py. - Filesystem path handling: manual
"/"split (#1051), glob missing the*wildcard (#1157), or non-deterministic/unlabeled aggregation (#891). isinstancevsissubclassmisuse in exception-type checks (#354).- Missing companions: tests, CI deps, docs, swagger (see Standards).
- Removal that leaves a dead export/command behind (#1211).
NIT:
- Accidental trailing-comma tuple (
x = (foo,)) in option/arg construction. - Missing
help=string on a new option.
Not blocking
- Do NOT ask for an
inference/core/version.pybump — inference releases are versioned separately from feature/bugfix PRs; the bump is coordinated at release time. inference_cli/version.py__version__is auto-adjusted by the PyPI-build CI action — do NOT ask for a manual bump there.- The
cloudapp (SkyPilot) deliberately usesstroptions, single-char shorts, and a bareexcept Exception→ exit-1 envelope with nodebug_mode. Do NOT demand therf-cloudconventions oncloud.py/cloud_adapter.py— it is a separate, older surface. - Speed/latency evidence for benchmark changes is verifiable via the PR description, not the diff.
Standards
CLI signature = public contract. Option long/short names, defaults, and command names are user-facing. Removing/renaming an option or short flag is breaking; additions must default to None/existing behavior. New params: Annotated[Optional[T], typer.Option("--long", "-short", help="...")] = None, snake_case Python name, kebab-case flag. Short flags must be unique within a command — a duplicate silently shadows (benchmark -o reuse renamed to -mpi, #2038). (inference_cli/lib/roboflow_cloud/data_staging/core.py, #1516, #2143.)
RF-cloud HTTP contract. All rf-cloud API calls go through get_workspace() + handle_response_errors(response, operation_name=...) in roboflow_cloud/common.py, decorated @backoff.on_exception(backoff.constant, RetryError, max_tries=3, interval=1), passing timeout=REQUEST_TIMEOUT. Connectivity/Timeout is re-raised as RetryError. In handle_response_errors: codes in HTTP_CODES_TO_RETRY = {408,429,502,503,504} → RetryError; 401 → UnauthorizedRequestError; any other >= 400 → RFAPICallError. (roboflow_cloud/common.py, config.py; timeout made env-configurable in #1950.)
Error taxonomy. roboflow_cloud/errors.py: RoboflowCloudCommandError is the base; RetryError and RFAPICallError subclass it; UnauthorizedRequestError subclasses RFAPICallError. Retry keys off RetryError type — do not raise a bare Exception where a RetryError is expected.
Command error envelope (rf-cloud). Command bodies resolve api_key (falling back to ROBOFLOW_API_KEY), then wrap the delegate call in try/except KeyboardInterrupt → print("Command interrupted.") + raise typer.Exit(code=2), and except Exception as error: → if debug_mode: raise error else typer.echo("Command failed. Cause: {error}") + raise typer.Exit(code=1). Non-zero exit codes are the scripting contract. (roboflow_cloud/data_staging/core.py.)
Pydantic wire schema. Entities in */entities.py map snake_case ⇄ camelCase JSON. Outbound bodies use serialization_alias (e.g. batchId, partName, machineType); inbound responses use alias (e.g. displayName, shardId). JSON keys are the server API contract and must match the backend. (roboflow_cloud/{data_staging,batch_processing}/entities.py.)
New request field is optional + threaded end-to-end. A new capability adds an Optional[...] = None field (with serialization_alias) on the entity, a matching param on the api_operations trigger fn, and the core.py command option — all three layers. (#2399, #2143, #2062.)
Enums for closed choice sets. Machine/backend/format options are str, Enum in */entities.py (MachineType, MachineSize, InferenceBackend, AggregationFormat, CompilationDevice), so Typer validates the choice. (batch_processing/entities.py; InferenceBackend added in #2062.)
Tunables live in config.py, env-overridable. REQUEST_TIMEOUT, MAX_SHARDS_UPLOAD_PROCESSES, MAX_DOWNLOAD_THREADS, MAX_IMAGE_REFERENCES_IN_INGEST_REQUEST use int(os.getenv("NAME", default)). Do not hardcode magic numbers in api_operations. (roboflow_cloud/config.py; #1950.)
Filesystem & aggregation robustness. Use os.path for path decomposition (no manual "/" split — #1051 broke on Windows); local-image globs must include the * wildcard because IMAGE_EXTENSIONS are bare extensions (#1157); batch-result aggregation must be deterministic (sorted(...)) and stamp source provenance (the image key — #891 produced unordered/unlabeled rows).
Benchmark error paths. Do not inject fake metrics on failure (execution_time=0 polluted latency stats, #1150), and use isinstance (not issubclass on an instance) for exception-type checks (#354).
SkyPilot cloud deploy. cloud_adapter.py lazily imports sky via check_sky_installed() (prompting pip install inference[cloud-deploy]) rather than importing at module top, so the base CLI imports without the heavy optional dep. Keep new cloud/SkyPilot code behind that guard; the YAML_DEFS templates (gcp_cpu/gcp_gpu/aws_cpu/aws_gpu) carry the container images and ports — treat changes to them as deployment-contract changes.
Enterprise extension isolation. inference_cli/lib/enterprise/** ships its own LICENSE.txt, is mounted as the enterprise typer app, and its heavy/optional TRT deps (under enterprise/inference_compiler/) must be import-guarded so the base CLI still imports. (#2186, #1679.)
Required companions
- Versioning — no
inference/core/version.pybump is required (release-time concern, see Not blocking). Ignoreinference_cli/version.py(CI-managed). - Tests — logic changes need unit tests under
tests/inference_cli/unit_tests/lib/...mirroring the source tree; user-visible changes update integration tests undertests/inference_cli/integration_tests/. (#891 updated both.) - CI deps — new runtime imports must be covered by the unit-test CI install list;
inferenceis an optional dep, so new deps typically belong inrequirements/requirements.cli.txt. - Docs — user-facing command/option changes update
docs/inference_helpers/cli_commands/{benchmark,cloud,workflows,infer,server}.md. Batch-processing API swagger is exposed externally - tag @Erol444. - Enterprise / inference_models — inference-compiler PRs (#2186) also touch weights-provider code and must add user-facing changes under
## Unreleasedininference_models/docs/changelog.md; applyreview-inference-models-pkgto anypyproject.tomldependency changes, but do not ask the contributor for a version bump.
Key files & entry points
inference_cli/main.py— root Typerapp+--versioncallback; mountsserver/cloud/benchmark/workflows/rf-cloud/enterprisesub-apps.inference_cli/cloud.py+inference_cli/lib/cloud_adapter.py— SkyPilot VM deploy.inference_cli/benchmark.py/inference_cli/lib/benchmark_adapter.py+inference_cli/lib/benchmark/**— benchmarking.inference_cli/workflows.py/inference_cli/lib/workflows/**— workflow image/video processing & result aggregation.inference_cli/lib/roboflow_cloud/{common,config,errors,core}.py— shared HTTP/retry/config/error taxonomy +rf_cloud_app.inference_cli/lib/roboflow_cloud/{data_staging,batch_processing}/{core,api_operations,entities}.py—rf-cloudcommands + wire schemas.inference_cli/lib/enterprise/inference_compiler/**— TRT compiler extension (own LICENSE).- Tests:
tests/inference_cli/{unit_tests,integration_tests}/**. Docs:docs/inference_helpers/cli_commands/**.
Reference PRs
- #2399 — feature: Asset Library metadata threaded entity→api_op→core; CI deps.
- #2062 — feature: selectable
InferenceBackendenum for batch processing (+ swagger). - #2143 — feature: add job name to batch jobs (+ swagger).
- #1950 — feature: env-configurable
ROBOFLOW_API_REQUEST_TIMEOUTinconfig.py. - #2186 / #1679 — feature: enterprise inference-compiler TRT extension (LICENSE, guarded deps, inference_models changelog).
- #1516 — bugfix: thread missing
page_sizethrough list-ingest-details. - #1157 — bugfix: glob wildcard for local benchmark images.
- #1051 — bugfix: Windows-safe path split via
os.path. - #891 — bugfix: deterministic aggregation +
imageprovenance key (unit+integration tests). - #1150 — bugfix: stop recording
execution_time=0on benchmark error. - #354 — bugfix:
isinstancefor exception check + formatted error status codes. - #1099 — feature: surface video-processing errors to the caller (
ErrorsInterceptor). - #1211 — chore: remove stale data-staging export.
- #2038 — bugfix: rename colliding benchmark short flag
-o→-mpi.
Related topic skills
When the PR also exhibits these cross-cutting concerns, load the matching topic skill too (see each skill's description for the trigger):
review-topic-external-contract-and-silent-fallbackreview-topic-backward-compat-and-versioningreview-topic-test-hygiene