Repository navigation
fix(prime-runs): stop blocking and dropping on upload bursts - #1137
kcoopermiller wants to merge 2 commits into
Conversation
A burst of long episodes filled the upload queue. Each further log() then blocked for 5 s and dropped its episode, because the uploader had taken everything queued (up to 256 episodes, many requests) into one long write while the producer filled the queue again. - submit() never waits: it appends, or drops at once when over budget. - The queue budget is 65,536 records and about 4 GiB of encoded bytes. Records are encoded at upload, so the uploader samples one per write and keeps a running estimate; the producer pays nothing. - One write takes about 30 MiB of what is queued, one request. A failed write now costs that much rather than all that was queued. An oversized batch goes out in order across writes. - Drop warnings are limited to one per 10 s; run.dropped_records keeps the count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b2f262b43
ℹ️ 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".
The write cap was a record count derived from one sample of the first record, so a small record at the front of a window of long episodes could take most of the queue into one write. Measure each record as it joins the batch and stop at max_write_bytes, so a write is bounded whatever mix of sizes is queued. A record over the limit goes out alone. Each record now encodes once more on the uploader thread. On a 1000-episode burst against a real service that left throughput unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Closing in favor of #1136, which also uploads concurrently and is about 4x faster on a real service. Results and the parts worth carrying over are in my comment there. |
Summary
A burst of long episodes filled the
prime_runsupload queue. Each furtherlog()then blocked for 5 s and dropped its episode. The uploader took everything queued (up to 256 episodes, many requests) into one long write while the producer filled the queue again. #1129 made uploads faster, but the drops remained.log()never waits.submit()appends to the queue, or drops at once when the queue is over budget.run.dropped_recordsstill has the full count.UploadWorker'smax_queue_sizeandput_timeoutarguments are gone.pr.initnever exposed them.Behavior to check in review
finish()still drains forfinish_timeout, and whatever is left is lost with a warning giving the count. A caller with a short timeout should size it for what it may have queued: 1000 long episodes took about 3 minutes here.Verification
1000 episodes of about 2.3 MiB, each with its own numpy arrays, submitted as fast as possible through the real
UploadWorker,TracesSinkandTracesClientagainst a dev Prime Traces deployment, from a laptop on about 320 Mbit/s of uplink:submit()main(from an earlier run, episodes sharing arrays)The stored count was read back from the service. Throughput is unchanged: uploads are still sequential, at about 5.6 episodes/s.
The prime-rl monitor load test, on this branch with its expected failure enforced: 9 passed.
prime-1000passed three more times alone, withlog()at 0.3 ms p99 and about 26 ms at most, against about 3 s before.packages/prime-runs/tests: 308 passed, 1 skipped.ruff check .at the locked version passes.🤖 Generated with Claude Code
Note
Medium Risk
Changes backpressure and memory footprint on the hot path for trace logging during training runs; sustained overload drops data instead of slowing the producer, and large queues can hold substantially more than the old 256-slot cap.
Overview
Fixes training
log()stalls when trace uploads lag:UploadWorker.submit()no longer blocks on a full queue (removedput_timeout/max_queue_size). Overload is handled by immediate drops once a record count (~65k) or estimated encoded-byte budget (~4 GiB) is exceeded, with drop warnings throttled to once per 10s.The uploader thread now sizes records (JSON encode on the worker) to enforce those budgets without charging the producer, and
_coalescecaps each sink write atmax_write_bytes(~one HTTP batch) so a slow or failed upload does not drain the entire backlog in one call.flush()/close()enqueue barriers and stop viaput_nowaitinstead of timing out behind a saturated queue. README documents that logging never waits for upload. Tests cover non-blocking drops, byte/write limits, ordering, and warning rate limits.Reviewed by Cursor Bugbot for commit 012db5e. Bugbot is set up for automated code reviews on this repo. Configure here.