Make Harbor artifact transfer budgets configurable - #2567
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes the production artifact-transfer pipeline by allowing substantially larger archives and introducing a new Prime VM streaming path across both normal and deferred grading. The runtime cleanup contract for that path remains insufficiently established, so human review is warranted. You can add or adjust custom eligibility rules. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c05595bf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| class HarborConfig(TasksetConfig): | ||
| artifact_max_bytes: int = Field(MAX_ARTIFACT_BYTES, gt=0) |
There was a problem hiding this comment.
When this limit is raised for the advertised trained-checkpoint or disk-image use case, collect() leaves the entire tar archive as bytes in trace.state.artifacts; neither IsolatedVerifierEnv.stage_verifier (verifiers/v1/envs/isolated_verifier/env.py:114-119) nor Harbor finalization clears it, and write_episode() (verifiers/v1/cli/output.py:147-153) subsequently applies Pydantic JSON serialization to the whole episode. Pydantic's default bytes serializer requires valid UTF-8, so typical binary archives make an otherwise successful rollout fail while saving its result; even UTF-8-compatible payloads produce multi-gigabyte trace records. Exclude or clear these transient payloads after restoration, or persist them separately, before allowing multi-gigabyte budgets.
Useful? React with 👍 / 👎.
| existence.extend(output.splitlines()) | ||
| collected: dict[str, bytes | None] = {} | ||
| budget = MAX_ARTIFACT_BYTES | ||
| budget = max_bytes |
There was a problem hiding this comment.
With a multi-gigabyte budget on the default Prime VM runtime, _tar_out() passes this remaining value to Runtime.read, whose VM implementation (verifiers/v1/runtimes/prime.py:372-387) returns the complete archive through base64-encoded execute_command stdout before decoding it. A 4 GiB archive therefore holds more than 5.3 GiB of stdout plus the decoded 4 GiB payload simultaneously and can exhaust the host or command-output transport; the uncapped path immediately below already uses download_file specifically to avoid output limits and base64 overhead. Large configured budgets need a bounded streaming/download path rather than the command-output read path.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0b6e2d8. Configure here.
| stderr = (stderr + chunk)[-500:] | ||
| if await process.wait(): | ||
| raise SandboxError(stderr.decode(errors="replace").strip()) | ||
| return data.getvalue() |
There was a problem hiding this comment.
VM read cleanup calls missing aclose
High Severity
The new Prime VM bounded-read path wraps the SDK process in contextlib.aclosing, which always calls aclose() when leaving the block, including after a successful return. Elsewhere this process is used through wait, terminate, kill, and write_stdin; PrimeProcess never forwards aclose. If that method is absent, every successful Prime VM artifact read fails, so Harbor collection cannot transfer the larger checkpoints this change is meant to allow.
Reviewed by Cursor Bugbot for commit 0b6e2d8. Configure here.
1 participant
Harbor tasks that produce trained checkpoints or VM disk files can exceed the fixed 32 MiB artifact-transfer cap and fail before separate grading. Add a per-task artifact budget so environments can provide enough space for their declared outputs.
HarborConfig.artifact_max_bytesis copied intoTaskDataand passed to the shared collector from both Harbor finalization and deferred rollout collection. The collector applies one total budget across all declared roots and the implicit/logs/artifacts/directory, while retaining 32 MiB as the default.On Prime VMs, bounded reads stream binary chunks into a capped buffer and close the process on success, failure, or cancellation. Artifact payloads remain transient and excluded from persisted traces.
This provides the artifact-budget support required by the Terminal-Bench 4 environment in prime-envs #800 and PTB Max #4, whose default budget is 4 GiB.
Note
Medium Risk
Raises configurable artifact memory on the host and changes Prime VM read semantics for bounded reads; defaults are unchanged but misconfigured large budgets could increase rollout memory use.
Overview
Adds a configurable total artifact archive budget (default still 32 MiB) so Harbor and generic rollouts can collect larger solver outputs—checkpoints, VM disks—without hitting a hard cap.
HarborConfig/TaskDataexposeartifact_max_bytes, copied into each Harbor task and passed intocollect()from rollout finalize and Harbor finalize; docs describe--env.taskset.artifact-max-bytes.Prime VM bounded reads no longer pipe files through
base64+execute_command; they stream raw bytes viaopen_processandhead -c, with independent limits on the sandbox read and host buffer—better suited to artifact collection at higher budgets.Tests assert runtime
state(including in-memory artifacts) stays off persisted traces when writing episodes; harbor docs note the same.Reviewed by Cursor Bugbot for commit 0b6e2d8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Make Harbor artifact transfer byte budget configurable via
artifact_max_bytesartifact_max_bytesfield toTaskDataandHarborConfig, defaulting to 32 MiB (MAX_ARTIFACT_BYTES), and propagates it through Harbor task parsing and non-deferred/rollout fallback artifact collection calls.collectutility in artifacts.py to accept a keyword-onlymax_bytesbudget and subtract each archive's length before collecting the next entry.PrimeRuntime._readin prime.py to stream raw binary viahead -cinto aBytesIObuffer instead of base64-decoding command output, raisingSandboxErrorwhen the stream exceeds the limit or the process exits non-zero.max_bytesretain the 32 MiB default.PrimeRuntime._readno longer uses base64 or an effectively-unbounded timeout.Macroscope summarized 0b6e2d8.