review-cli-cloud-tooling

# 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.py` - `inference_cli/lib/cloud_adapter.py` — SkyPilot cloud-deploy backing the `cloud` app. - `inference_cli/lib/benchmark/**`, `inference_cli/lib/benchmark_adapter.py` - `inference_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 under `docs/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: - **`cloud` app** (`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-cloud` app** (`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-cloud` API op that does NOT go through `get_workspace()` + `handle_response_errors(operation_name=...)`, is not decorated `@backoff.on_exception(backoff.constant, RetryError, max_tries=3, interval=1)`, does not pass `timeout=REQUEST_TIMEOUT`, or does not re-raise connectivity/`Timeout` as `RetryError`. Raising a bare `Exception` where retry logic expects `RetryError` breaks retries. - Wire schema: a request/response field whose `serialization_alias` (outbound camelCase) or `alias` (inbound) does not match the backend contract, or a casual rename of an existing alias. - Breaking CLI contract: renaming/removing a `typer.Option` long or short flag, a command name, or narrowing a default. A colliding short flag within one command silently shadows (`-o` reuse, #2038). - Benchmark integrity: fabricating metrics on the error path (`execution_time=0` counted a failure as a zero-latency success, #1150). **FLAG** — reviewer should raise: - New network command missing `api_key` resolution from `ROBOFLOW_API_KEY`, a `--debug-mode/--no-debug-mode` flag, or the standard `rf-cloud` error envelope (KeyboardInterrupt → exit 2; else debug re-raise / echo + exit 1). - New request capability not threaded through all three layers: `Optional[...] = None` entity field → `api_operations` param → `core.py` command option (#2399). - Closed choice set typed as a free-form `str` instead of a `str, Enum` (#2062). - Magic number hardcoded in `api_operations` instead of an `os.getenv`-backed constant in `config.py`. - Filesystem path handling: manual `"/"` split (#1051), glob missing the `*` wildcard (#1157), or non-deterministic/unlabeled aggregation (#891). - `isinstance` vs `issubclass` misuse 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.py` bump — 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 `cloud` app (SkyPilot) deliberately uses `str` options, single-char shorts, and a bare `except Exception` → exit-1 envelope with no `debug_mode`. Do NOT demand the `rf-cloud` conventions on `cloud.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.py` bump is required (release-time concern, see Not blocking). Ignore `inference_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 under `tests/inference_cli/integration_tests/`. (#891 updated both.) - **CI deps** — new runtime imports must be covered by the unit-test CI install list; `inference` is an optional dep, so new deps typically belong in `requirements/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 `## Unreleased` in `inference_models/docs/changelog.md`; apply `review-inference-models-pkg` to any `pyproject.toml` dependency changes, but do not ask the contributor for a version bump. ## Key files & entry points - `inference_cli/main.py` — root Typer `app` + `--version` callback; mounts `server`/`cloud`/`benchmark`/`workflows`/`rf-cloud`/`enterprise` sub-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-cloud` commands + 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](https://github.com/roboflow/inference/pull/2399) — feature: Asset Library metadata threaded entity→api_op→core; CI deps. - [#2062](https://github.com/roboflow/inference/pull/2062) — feature: selectable `InferenceBackend` enum for batch processing (+ swagger). - [#2143](https://github.com/roboflow/inference/pull/2143) — feature: add job name to batch jobs (+ swagger). - [#1950](https://github.com/roboflow/inference/pull/1950) — feature: env-configurable `ROBOFLOW_API_REQUEST_TIMEOUT` in `config.py`. - [#2186](https://github.com/roboflow/inference/pull/2186) / [#1679](https://github.com/roboflow/inference/pull/1679) — feature: enterprise inference-compiler TRT extension (LICENSE, guarded deps, inference_models changelog). - [#1516](https://github.com/roboflow/inference/pull/1516) — bugfix: thread missing `page_size` through list-ingest-details. - [#1157](https://github.com/roboflow/inference/pull/1157) — bugfix: glob wildcard for local benchmark images. - [#1051](https://github.com/roboflow/inference/pull/1051) — bugfix: Windows-safe path split via `os.path`. - [#891](https://github.com/roboflow/inference/pull/891) — bugfix: deterministic aggregation + `image` provenance key (unit+integration tests). - [#1150](https://github.com/roboflow/inference/pull/1150) — bugfix: stop recording `execution_time=0` on benchmark error. - [#354](https://github.com/roboflow/inference/pull/354) — bugfix: `isinstance` for exception check + formatted error status codes. - [#1099](https://github.com/roboflow/inference/pull/1099) — feature: surface video-processing errors to the caller (`ErrorsInterceptor`). - [#1211](https://github.com/roboflow/inference/pull/1211) — chore: remove stale data-staging export. - [#2038](https://github.com/roboflow/inference/pull/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-fallback` - `review-topic-backward-compat-and-versioning` - `review-topic-test-hygiene`