feat(CLI): add tg batches commands for the batch API - #525
Conversation
Broly Security ScanNote ✅ Clean scan Note Re-scan this PR anytime with
|
|
@cursoragent fix tests and update the PR description for changes I made |
|
Fixed the three CI failures and updated the PR description for your download/output changes.
PR: #525 — 43 CLI tests passed. |
| if output is not None: | ||
| saved: list[dict[str, str]] = [] | ||
| directory_output = _is_directory_output(output) | ||
| error_file_id = job.error_file_id | ||
|
|
||
| if job.output_file_id: | ||
| out_path = await download_file_content( | ||
| config.client, | ||
| job.output_file_id, | ||
| output=output, | ||
| loading_message="Downloading batch output...", | ||
| ) | ||
| assert isinstance(out_path, Path) | ||
| saved.append({"kind": "output", "id": job.output_file_id, "path": str(out_path)}) | ||
| elif error_file_id and not directory_output: | ||
| # No output file — write the error file to the exact path the user asked for. | ||
| err_path = await download_file_content( | ||
| config.client, | ||
| error_file_id, | ||
| output=output, | ||
| loading_message="Downloading batch errors...", | ||
| ) | ||
| assert isinstance(err_path, Path) | ||
| saved.append({"kind": "error", "id": error_file_id, "path": str(err_path)}) | ||
| error_file_id = None | ||
|
|
||
| if error_file_id: | ||
| err_dest = output if directory_output else _error_output_path(output) | ||
| err_path = await download_file_content( | ||
| config.client, | ||
| error_file_id, | ||
| output=err_dest, | ||
| loading_message="Downloading batch errors...", | ||
| ) | ||
| assert isinstance(err_path, Path) | ||
| saved.append({"kind": "error", "id": error_file_id, "path": str(err_path)}) |
There was a problem hiding this comment.
When --output is a directory and the job has both an output and an error file, both downloads go through download_file_content(output=
), which names each file from whatever the Files API returns for it. If those two filenames match, the second write clobbers the first — and we still print both "Output saved to …" and "Errors saved to …" as if two files landed. I mocked both files returning filename: "batch.jsonl" and ended up with a single batch.jsonl containing only the error content._error_output_path() already solves this for the concrete-file case; the directory case needs the same guarantee (suffix on collision, or force .errors into the error filename).
| console.print(raw.decode("utf-8")) | ||
| if job.error_file_id: | ||
| console.print(f"\n[dim]Error file also available: tg batches download {id} --output ./out[/dim]") |
There was a problem hiding this comment.
The default (no --output) path is console.print(raw.decode("utf-8")), so Rich does two things to the payload: it parses markup, and it hard-wraps at the console width. With a completion containing [bold]…[/bold] and a line over 80 chars, the tags get eaten and newlines get injected mid-JSON — > results.jsonl produces invalid JSONL. Model output containing an unbalanced tag like [/close] raises MarkupError and kills the command outright.
Since this is the default mode of the command, I think it has to be sys.stdout.buffer.write(raw) (or at minimum a markup=False, soft_wrap=True console). Two related things in the same block:
- No UnicodeDecodeError guard here, even though the --json branch right above has one — a non-UTF-8 byte traces back instead of falling back.
- The "Error file also available: …" hint on line 139 goes to stdout, so it becomes the last line of a redirected results.jsonl. Hints belong on stderr.
| if job.status in _INCOMPLETE_STATUSES and job.progress is not None: | ||
| console.print(f"{format_progress(job.progress)} {format_status(job.status)}") |
There was a problem hiding this comment.
print_batch_detail only prints a status line when status in _INCOMPLETE_STATUSES and progress is not None. So on a CANCELLED job you get created-at, the API, the model, and nothing else — no indication it was cancelled. Same for EXPIRED, for FAILED with no error field, and for VALIDATING before progress is populated. STATUS_COLORS and format_status() already cover all six states, so the four terminal ones are effectively dead code. Can we just always print the status line?
| if job is None or not job.id: | ||
| console.print("[red]x[/red] Batch job was not created") | ||
| if response.warning: | ||
| console.print(response.warning) | ||
| return |
There was a problem hiding this comment.
If the API comes back with {"job": null, "warning": …} we print x Batch job was not created and then return, so the shell sees success. tg batches submit … && next-step will happily keep going against a batch that doesn't exist. Needs sys.exit(1).
| def _is_directory_output(path: Path) -> bool: | ||
| return path.is_dir() or path.suffix == "" |
There was a problem hiding this comment.
--output ./results crashes when ./results already exists as a file: _is_directory_output() treats any suffix-less path as a directory, and since the validator here was loosened to file_okay=True (files download uses file_okay=False), that path now reaches output.mkdir(parents=True, exist_ok=True) inside download_file_content — and exist_ok does not tolerate an existing non-directory:
touch ./results && tg batches download --output ./results
→ Error: [Errno 17] File exists: '/…/results'
An explicit output.exists() and not output.is_dir() check before the mkdir would sort it, or restore file_okay=False.
| if not job.output_file_id: | ||
| console.print( | ||
| "[red]Batch job has no output file[/red]. " | ||
| "Use [primary]--output[/primary] to download the error file instead." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
All three early exits print Rich prose to stdout regardless of config.json, so tg batches download --json | jq fails on non-JSON input. I noticed download is the one command excluded from the new test_json_mode_pipeable_to_jq case (tests/cli/test_json_mode_pipeable_to_jq.py:100) — if that's why, I'd rather fix the output than skip the check.
| ) | ||
| sys.exit(1) | ||
|
|
||
| raw = await download_file_content( |
There was a problem hiding this comment.
response.read() pulls the entire batch output into RAM, and in --json mode we then build a second full copy as a string (a third, base64, on decode failure). Batch outputs can be very large by design, and download_file_content already has a streaming write_to_file path for --output. tg batches download | head shouldn't need the whole body resident.
| if job.error_file_id: | ||
| console.print(f" - [red]An error occurred[/red]") | ||
| console.print(f" - Error file ID {job.error_file_id}") | ||
|
|
||
| if job.error: | ||
| console.print(f" - [red]An error occurred[/red]") |
There was a problem hiding this comment.
nit: A job with both error_file_id and error set renders the header line once per block:
- An error occurred
- Error file ID file-err
- An error occurred
boom
| console.print(f"[green]√ Batch job submitted.[/green] [dim]({job.id})[/dim]") | ||
| if response.warning: | ||
| console.print(f"[yellow]{response.warning}[/yellow]") | ||
| print_model_dump(job, show_nulls=False) |
There was a problem hiding this comment.
BatchJob declares x_model_id = FieldInfo(alias="model_id"), and print_model_dump renders model_dump() (field names, not aliases), so submit shows a row literally labelled "X Model Id:". cancel is worse — the top-level parse keeps model_id as an extra field alongside x_model_id, so it prints both rows with the same value (I confirmed this against the real construct_type path, not just a plain model_validate). retrieve sidesteps the whole thing with its curated view; submit/cancel want either the same treatment or a by_alias dump.
| return | ||
|
|
||
| console.print("[green]√[/green] Cancelled batch job") | ||
| print_model_dump(response, show_nulls=False) |
There was a problem hiding this comment.
BatchJob declares x_model_id = FieldInfo(alias="model_id"), and print_model_dump renders model_dump() (field names, not aliases), so submit shows a row literally labelled "X Model Id:". cancel is worse — the top-level parse keeps model_id as an extra field alongside x_model_id, so it prints both rows with the same value (I confirmed this against the real construct_type path, not just a plain model_validate). retrieve sidesteps the whole thing with its curated view; submit/cancel want either the same treatment or a by_alias dump.
|
@cursoragent address all of Zain's feedback on this PR |
|
Addressed all of Zain’s review comments on #525.
53 CLI tests passed locally; batches JSON-mode jq check passed against the mock server. |
|
some places where the current design diverges from the batch inference API:
So you should take the model specification out of the CLI commands
|
Add submit, list/ls, get/retrieve, and cancel under `tg batches`, including local-file upload (like `ft create`), positional + `--api`/`--model`/`-M` flags, and JSON output. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
The UUID identifier overwrote the `batch*` prefix matcher, so `tg batches <batch-id>` stopped resolving to retrieve. Align retrieve assertions with the curated human output (no raw COMPLETED dump, progress bar uses In_progress). Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
Fail submit when the API returns a null job, escape API text in Rich output, use the curated batch view on submit/cancel, stream download stdout, treat existing suffix-less --output paths as files, and emit JSON on download errors. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
1024dea to
68d4958
Compare
| error_file_id = None | ||
|
|
||
| if error_file_id: | ||
| err_dest = output if directory_output else _error_output_path(output) |
There was a problem hiding this comment.
Still open from last round. When --output is a directory and the job has both files, err_dest = output, so both resolve to
/. With both mocked as batch.jsonl the output is silently clobbered — and we still print two "saved to" lines pointing at the same path, plus two --json files entries with identical path. The existing test only passes because it mocks two distinct filenames, which we don't control. Simplest fix is to disambiguate in the directory branch the way _error_output_path() already does for the file branch.| if job.error: | ||
| console.print(f" {escape_rich_markup(job.error)}") | ||
|
|
||
| if job.status in _INCOMPLETE_STATUSES and job.progress is not None: |
There was a problem hiding this comment.
The status line is gated on _INCOMPLETE_STATUSES and non-null progress, so a CANCELLED job prints created-at, endpoint, model and file IDs, and the word "cancelled" appears nowhere. Same for EXPIRED, and for FAILED when the server didn't populate error. Status is the one field people run get for — I think it should always print, with the progress bar as the extra that's only shown for in-flight jobs.
| ), | ||
| ) | ||
|
|
||
| if config.json: |
There was a problem hiding this comment.
--json with no --output downloads nothing and exits 0 - test freezing it in (test_download_stdout_json_is_metadata_only). tg batches download streams the results; adding --json makes it print {"batch_id":…,"output_file_id":…} and exit 0 having fetched nothing. A format flag shouldn't change what the command does. Either stream the content into a JSON field the way retrieve-content --stdout --json already does, or _fail() telling the user to pass --output. Downloading nothing while exiting 0 is the option that'll bite someone in a pipeline.
There was a problem hiding this comment.
Status was only shown for in-flight jobs with progress, so cancelled, expired, and failed jobs omitted the field people run get for. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
| from unittest.mock import AsyncMock, patch | ||
|
|
||
| import httpx | ||
| import pytest |
Directory --output used the server filename for both files, so identical names silently overwrote the results. Error files now get the same .errors suffix already used for file destinations. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
--json was printing file IDs and exiting 0 without fetching anything. Require --output so a format flag cannot silently skip the download. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
|
@cursoragent fix the CLI tests from |
Submit no longer takes a model argument (it's in the JSONL body). Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
* feat(CLI): add tg batches commands for the batch API Add submit, list/ls, get/retrieve, and cancel under `tg batches`, including local-file upload (like `ft create`), positional + `--api`/`--model`/`-M` flags, and JSON output. Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * add download and improve outputs * fix(CLI): restore batches implicit retrieve and match retrieve tests The UUID identifier overwrote the `batch*` prefix matcher, so `tg batches <batch-id>` stopped resolving to retrieve. Align retrieve assertions with the curated human output (no raw COMPLETED dump, progress bar uses In_progress). Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * fix(CLI): address batches review feedback Fail submit when the API returns a null job, escape API text in Rich output, use the curated batch view on submit/cancel, stream download stdout, treat existing suffix-less --output paths as files, and emit JSON on download errors. Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * test(CLI): avoid unused respx route in batches --json download test Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * test(CLI): keep batches --json jq checks when prism omits a job Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * prune unused cli parameters * fix(CLI): always print batch job status on retrieve Status was only shown for in-flight jobs with progress, so cancelled, expired, and failed jobs omitted the field people run get for. Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * fix(CLI): don't clobber batch output when error file shares a name Directory --output used the server filename for both files, so identical names silently overwrote the results. Error files now get the same .errors suffix already used for file destinations. Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * fix(CLI): reject batches download --json without --output --json was printing file IDs and exiting 0 without fetching anything. Require --output so a format flag cannot silently skip the download. Co-authored-by: Blaine Kasten <blainekasten@gmail.com> * test(CLI): drop model from batches submit tests Submit no longer takes a model argument (it's in the JSONL body). Co-authored-by: Blaine Kasten <blainekasten@gmail.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>


Fixes DX-918.
Adds CLI commands for the Batch API:
Submit
FILE_ID_OR_PATHbehaves likeft create: local paths are uploaded withpurpose=batch-api, otherwise treated as a file IDAPI_TYPEischat.completions|audio.transcriptions|audio.translations, positional or--apiX Model Iddump)--json)Retrieve
tg batches <batch-id>implicit retrieveerrorshare a single "An error occurred" header; API error text is escapedDownload
--output(file or directory)./resultsare treated as files, not directories<stem>.errors<suffix>so a shared server filename cannot clobber output--outputstreams the output file to stdout--jsonrequires--output(does not embed file contents); success JSON lists saved paths--jsonincluded) print{"error": "..."}then exit 1tg files downloadCancel
Also supports
--jsonandls/getaliases.Linear Issue: DX-918