upload-to-s3: read the source once instead of three times#77
Conversation
upload-to-s3 read the uncompressed source three separate times before the upload: once for the sha256sum stored as object metadata, once for `wc -l` (the recordcount metadata), and once to compress it. For large inputs those three reads -- at disk speed -- dominate the run, not the S3 transfer itself. Read the source once instead: tee the single read to the hasher and the line counter (via fifos) while compressing to a temp file, then upload that file. Uploading a real file rather than piping to `aws s3 cp -` also lets the AWS CLI parallelize the multipart transfer instead of a single slow stdin stream. Measured in nextstrain/ncov-ingest (GenBank, ~262 GB uncompressed sequences): per-upload wall dropped ~3x on the large read-bound files (e.g. sequences.fasta.zst 8131s -> 1739s), ~3.15x summed across all upload rules, and ~72 min off the end-to-end run (the big uploads are on the critical path). The uploaded object is unchanged: same body, same sha256sum/recordcount metadata (the hash is still of the uncompressed source), same content-type. Temp artifacts are removed on any exit via a trap. Behaviour note: the sha256 is now computed while compressing, so the "files are identical, skipping upload" short-circuit can no longer avoid the read+compress -- it only skips the upload (the head-object check still runs first, so nothing is transferred when unchanged). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`wait a b` returns only b's exit status, so a failure of the sha256 process was masked from `set -e`: the script could proceed and upload with an empty or wrong sha256sum in the object metadata. Wait for each pid on its own line so either failure aborts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
If $src can't be read, `tee < "$src"` fails and set -e aborts -- but the background sha256/wc readers, already blocked opening their fifos, never get a writer and leak (hang). Guard with `[[ -r "$src" ]]` before launching them, which also gives a clear error message. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
joverlee521
left a comment
joverlee521
left a comment
There was a problem hiding this comment.
Thanks for tackling a long standing issue Trevor!
| wait "$hash_pid" | ||
| wait "$count_pid" |
There was a problem hiding this comment.
With this spinning out 2 processes in the background, do we have to worry about oversubscribing CPUs when Snakemake schedules multiple upload jobs?
I think we're fine based on the improved runtimes documented in the PR description, but something to keep in mind in the future.
There was a problem hiding this comment.
My (partial) understanding here:
-
The existing
zstd -T0was grabbing all cores https://github.com/nextstrain/shared/blob/main/scripts/upload-to-s3#L41. Though maybe not actually an issue in terms of oversubscription, because this will be disk bound? -
The synchronous
wcandsha256sumprocesses should be I/O bound rather than CPU bound. And by having less disk access per upload job, the competition between jobs is ameliorated.
|
I'll follow up with all of the downstream PRs to update the vendored scripts tomorrow. Notably, the ncov-ingest update will require some path update. |
|
I have branch https://github.com/nextstrain/ncov-ingest/tree/migrate-shared-vendored that does this more extensively and should make future updates easy. I was waiting to rebase it however until the |
|
Merged all downstream PRs created by https://github.com/nextstrain/shared/actions/runs/28630822214 except ncov-ingest and hbv (not actively maintained). |
3 participants
Via Claude
Summary
scripts/upload-to-s3read the uncompressed source three separate times before uploading:sha256sum— for thesha256sumobject metadata (used to skip re-uploads of unchanged files)wc -l— for therecordcountmetadatazstd/xz/gzip) — to produce the upload bodyThis reads it once: a single read is
tee'd to the hasher and the line counter (via fifos) while being compressed to a temp file, which is then uploaded. Uploading a real file rather than piping intoaws s3 cp -also lets the AWS CLI parallelize the multipart transfer instead of a single serialized stdin stream.The uploaded object is unchanged — same body, same
sha256sum/recordcountmetadata (the hash is still of the uncompressed source), same content-type.download-from-s3's hash comparison and all consumers are unaffected.Why we expect this to be faster
For large inputs the three reads — not the S3 transfer — dominate. Phase-timing a 24 GB chunk of
aligned.fastaon an AWS Batch box showed every pass running at the same ~240 MB/s disk-read floor:cat(raw read)sha256sum(this repo's python impl)wc -lzstd -T0They are all disk-I/O-bound, so three passes ≈ three full reads of the source. Collapsing to one read is ~3× less work. An isolated A/B on the same 24 GB confirmed 3.15× faster (302s → 96s), producing an identical sha256, identical line count, and a valid
.zst.Measured impact in nextstrain/ncov-ingest
Full-corpus GenBank trial (actions run), ~262 GB uncompressed
sequences.fasta, per-rulebenchmark:wall time vs the prior version:genbank.ndjson.zstsequences.fasta.zstaligned.fasta.zsttranslation_ORF1a.fasta.zstupload_singlerules (sum)The big uploads sit on the pipeline's critical path, so reconstructing the DAG timeline from the Snakemake log the change cut ~72 min off the end-to-end run (~6.6 h → ~5.4 h). Tiny files (
*.json, small tsvs) stayed at ~1× — a nice control, since their reads are negligible — and peak RSS was unchanged (the fifo/tee streaming adds no memory).Behaviour note
The sha256 is now computed while compressing, so the "files are identical, skipping upload" short-circuit can no longer avoid the read+compress — it only skips the upload itself (the
head-objectcheck still runs first, so nothing is transferred when unchanged).Test plan
aws s3 cpinvocation — only the source arg differs:-→ temp file)*.json, direct upload); temp cleaned up on every exitbash -nclean🤖 Generated with Claude Code