Upload all skill files concurrently - #2540
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 changes every skill-supporting harness setup from ordered per-file writes to unbounded concurrent writes while buffering the entire skill tree, affecting runtime resource usage and overwrite ordering. Unresolved comments identify concrete memory/process pressure and nondeterministic collision risks that require human review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
xeophon
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| # `write` moves bytes, not modes; restore the execute bits scripts need. | ||
| await runtime.run(["chmod", "+x", *executables], {}) | ||
| # Settle every upload before returning or surfacing an error. | ||
| results = await asyncio.gather( |
There was a problem hiding this comment.
🟠 High v1/harness.py:119
When two configured skills have the same basename (or the same folder is listed twice), duplicate destinations are written concurrently, so the loaded skill can contain a nondeterministic mix of the skills rather than the later configured skill deterministically overwriting the earlier one. Replace the concurrent asyncio.gather with ordered writes so duplicate targets are resolved consistently.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/harness.py around line 119:
When two configured skills have the same basename (or the same folder is listed twice), duplicate destinations are written concurrently, so the loaded skill can contain a nondeterministic mix of the skills rather than the later configured skill deterministically overwriting the earlier one. Replace the concurrent `asyncio.gather` with ordered writes so duplicate targets are resolved consistently.
There was a problem hiding this comment.
The ordering change is intentional. This PR makes file and skill upload order unspecified, including writes from different configured folders that share a destination. Skills are expected to use distinct destination paths; preserving the previous last-configured-wins behavior for collisions is outside the intended contract of this change. The PR description explicitly documents this, and we are keeping concurrent uploads.
| """Upload each `config.skills` folder into `runtime` at `dest/<folder name>` — | ||
| the program's fixed skill discovery location, which a supporting harness's | ||
| `setup` passes.""" | ||
| uploads = [] |
There was a problem hiding this comment.
🟠 High v1/harness.py:103
install_skills loads every skill file into memory and starts a concurrent write task for each file, so a large skill tree can exhaust runner memory or Docker exec process resources during setup. Write each file before reading or scheduling the next one, as the previous sequential loop did.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/harness.py around line 103:
`install_skills` loads every skill file into memory and starts a concurrent write task for each file, so a large skill tree can exhaust runner memory or Docker `exec` process resources during setup. Write each file before reading or scheduling the next one, as the previous sequential loop did.
There was a problem hiding this comment.
Agreed that this increases peak resource usage: all selected file contents remain in memory, and Docker can start one exec process per write. This is an accepted tradeoff of the all-concurrent design. The SDK connection pool limits connections, not buffered bytes or Docker processes. The PR description documents the buffering; the measurements cover one Prime VM with up to 128 concurrent uploads, not large trees across many rollouts or Docker. We are keeping the all-concurrent implementation with those limits on the performance claim.
![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: b34cf2a337
ℹ️ 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".
| results = await asyncio.gather( | ||
| *(runtime.write(target, data) for target, data in uploads), | ||
| return_exceptions=True, |
There was a problem hiding this comment.
When a skill contains many files, especially while multiple rollouts are being set up, this creates one in-flight runtime.write per file with no concurrency limit, after retaining every file's bytes in uploads. For example, DockerRuntime.write starts a separate docker exec process for every call, while remote runtimes issue an RPC per call, so a large tree multiplied by eval concurrency can exhaust processes, connections, provider limits, or host memory and fail setup. Upload in bounded batches or through a semaphore instead of gathering the entire tree at once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a real limitation of the chosen all-concurrent design. Peak buffered memory scales with the total selected file bytes, and concurrent rollout setup can multiply the upload/process pressure. The SDK connection pool does not provide a general resource bound. We explicitly accept that tradeoff for this PR and document the buffering and the single-VM measurement scope. The benchmark does not establish safety for arbitrarily large skill trees or many simultaneous rollouts; the implementation will remain all-concurrent.
| results = await asyncio.gather( | ||
| *(runtime.write(target, data) for target, data in uploads), |
There was a problem hiding this comment.
When config.skills contains directories with the same basename and relative filename, both uploads target the same destination, and the config currently permits this. gather preserves result ordering but not write completion ordering, so whichever remote upload finishes last now wins nondeterministically; the previous sequential awaits made the later configured skill deterministically overwrite the earlier one. Reject duplicate targets or serialize writes that collide.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct: gather preserves result ordering, not write completion ordering. This PR intentionally leaves completion order unspecified for both files and skills, including duplicate destination paths. Distinct destinations are the expected configuration; there is no last-source-wins guarantee for collisions in the proposed behavior. The PR description documents that decision, so we are not adding collision serialization or validation here.
![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 b34cf2a. Configure here.
| raise result | ||
| if executables: | ||
| # `write` moves bytes, not modes; restore the execute bits scripts need. | ||
| await runtime.run(["chmod", "+x", *executables], {}) |
There was a problem hiding this comment.
Skill uploads lose archive batching
High Severity
install_skills now issues one runtime.write per skill file through an unbounded asyncio.gather, instead of the ordered tar batches this change set out to use. That restores the per-file remote-call cost measured on Prime and can fan out well past the previous 32-file and 1 MiB caps, so large skills may overload the runtime or hang a cancelled write with no extract process to stop.
Reviewed by Cursor Bugbot for commit b34cf2a. Configure here.
There was a problem hiding this comment.
The final design intentionally uses ordinary concurrent uploads; retaining the archive batches and their caps is no longer a requirement. There is still one write per file, but overlapping transfers reduced the measured 32-file installation median from 6.099 s on main to 1.009 s. The increased memory and upload fan-out are accepted tradeoffs, documented in the PR description.
There is no extraction process in this path. Cancellation uses the existing runtime.write implementations. In the Prime probe, cancelling 128 uploads left no active client upload tasks; this does not establish Docker cancellation behavior or guarantee rollback of bytes already accepted by the service. The absence of an extraction process alone does not demonstrate a cancellation failure.
1 participant
Upload all configured skill files concurrently through the existing runtime write API, then restore executable bits in one chmod call. Harness setup waits for every upload to settle before returning or propagating an upload error.
File and skill upload order is unspecified. Configurations with colliding destination paths must not depend on which write wins. File contents are buffered in memory for the upload phase; the runtime client's connection pool manages network connections.
Warm Prime VM measurements, including the runtime upload-directory improvement already merged in #2539:
One temporary
vm=TruePrime VM (python:3.11-slim, 1 CPU, 2 GiB), one warmup round and six interleaved measured rounds per workload. All variants use the full Prime runtime from main at284bcfdfb9fa. Provisioning and initial program downloads are excluded; the setup measurement includes the full existing warm Codex setup path.This comparison measures setup latency on one VM, including up to 128 concurrent uploads. It does not measure model inference or throughput across many simultaneous rollouts.
Observed all-concurrent install/multi range: 0.950–1.061 s.
Observed all-concurrent install/large range: 1.614–1.846 s.
Observed all-concurrent setup/multi range: 2.498–4.926 s.
Note
Medium Risk
Concurrent runtime writes can change failure timing and load on the sandbox API compared to sequential uploads, though paths and post-upload chmod behavior stay the same.
Overview
Harness.install_skillsno longer awaits each skill file upload one at a time. It first collects every(target, bytes)pair and executable path, then runs allruntime.writecalls concurrently withasyncio.gather, failing only after all uploads settle (re-raising the first exception from the batch).Executable
chmodnow runs once for the full rollout after uploads complete, instead of per skill folder. File discovery dropssorted()onrglob, so iteration order is no longer fixed but destinations are unchanged.Reviewed by Cursor Bugbot for commit b34cf2a. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Batch skill uploads in
Harness.install_skillsvia tar archives_EXTRACT_SKILLShelper script to extract uploaded archives in the runtime and remove the temporary archive path on exit.SandboxError, attempting shielded process termination and temporary archive removal.📊 Macroscope summarized b34cf2a. 1 file reviewed, 2 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues