Repository navigation
feat(get): --workers streams to S3 across compute nodes (0.25.0) - #186
Merged
Merged
Conversation
`dt get` had no distributed mode. `dt push --workers` had solved multi-node transfer already, and the machinery was never push-specific, so this reuses it rather than rewriting it (#172). Scoped to `s3://` destinations, which is a decision rather than an unfinished edge: -o s3://... bound by the network -> a second node adds bandwidth -o some/dir/ bound by the shared FS -> a second node adds contention A local destination on Lustre is limited by the filesystem, not the node, so `--workers` is a hard usage error there pointing at `-j`. On NCI there is a second reason to like the S3 case: `copyq` is the queue with outbound network access, and it is already hpc.DEFAULT_TRANSFER_QUEUE. Reuse, per the issue: - `hpc.partition_by_size()` is the LPT bin-packer lifted out of `push.partition_manifest`, which was pure bin-packing with nothing push- specific in it. What stays behind in push is the part that genuinely is: a file is named by its hash, so its size has to be read from the local cache. The docstring says where the boundary is and why LPT belongs to qxub workers and not to a ThreadPoolExecutor -- separate processes cannot steal work, a shared queue self-corrects, and freezing an assignment the queue would fix is strictly worse. - `hpc.save_manifest()` takes a `metadata` dict. Everything a worker needs beyond its own partition travels that way, because it is a fresh process on another machine: push needs a remote name, get also needs a destination, credentials config and the flags the run was invoked with. - `submit_workers` / `monitor_jobs` / `load_worker_partition` unchanged. The design questions #172 raised, settled: *Reachability.* Resolve once on the submitting node and pass paths. The submitter has to resolve every row anyway -- LPT needs byte sizes and `list_source_files` is where sizes come from -- so having paid for that it ships the resolved md5, size and destination key per file. **No worker ever runs `dvc list`**, which is what keeps N compute nodes off the shared clone's SQLite state db; `_run_dvc_list`'s retry loop exists because that lock is real. A worker opens the clone once, read-only, for one thing: the source remote's filesystem and credentials out of its `.dvc/config`. Link types and the destination identity are likewise resolved on the submitter and passed through, not re-derived per worker. `resolve_link_types()` reads `dvc config` from the cwd and `preflight()` resolves a credential chain; both could answer differently on a compute node, and a transfer that silently changes its mind halfway is worse than one that fails. *Partition unit.* Per file, not per row -- the issue framed this as a trade against per-row reporting, but it is not one. Each task carries its row, so reporting stays per-row and in CSV order even when a sample directory spans nodes, and per-file balances properly in the two cases per-row cannot: a single row of thousands of files, and rows differing by an order of magnitude, which fastq samples reliably do. *Reporting.* Workers write `result_<n>.json`; the submitter collates by row tag. A row whose files do not all come back is failed even when every result that did arrive succeeded -- a job killed on walltime would otherwise read as a clean run of a short row. Each worker preflights for itself, so `--dest-account-id` is enforced on the node that actually writes. The manifest carries the profile *name*, endpoint and region; no key material. Also, from the issue's note about the ThreadPoolExecutor idiom being copy-pasted five times: this adds a sixth use, so per "factor it then" the two in `get.py` are now behind `_place_all` and `_upload_all`, and `_resolve_rows` is shared by the local and S3 row-resolve paths. That makes `upload_to_s3` resolve rows concurrently, which it did not before. A cross-module `utils.run_parallel()` is still not worth it. Not verified here, and it needs a real submission: whether more nodes actually move more bytes. #172 asks for that measurement and it remains open -- if the *source* remote is on /g/data the read side is a shared filesystem again and the same contention argument applies to it. docs/get.md says so and says to time a single-node `-j 8` run first. Tested: 30 new unit tests. The central one replaces `hpc.submit_workers` with a stub that runs the worker in-process, so the manifest is exercised as a real interface rather than asserted against its spelling. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #172.
dt gethad no distributed mode.dt push --workershad solved multi-node transfer already and the machinery was never push-specific, so this reuses it rather than rewriting it.Scoped to
s3://, deliberately-os3://…some/dir/A local destination on Lustre is limited by the filesystem, not the node, so
--workersis a hard usage error there, pointing at-j:On NCI there is a second reason to like the S3 case:
copyqis the queue with outbound network access, and it is alreadyhpc.DEFAULT_TRANSFER_QUEUE.Reuse, as the issue asked
hpc.partition_by_size()— the LPT bin-packer, lifted out ofpush.partition_manifest. What stays behind inpushis the part that genuinely is push-specific: a file is named by its hash, so its size has to be read from the local cache. Splitting exactly there seemed better than a blind move.hpc.save_manifest(..., metadata=...)— everything a worker needs beyond its own partition travels this way, because it is a fresh process on another machine.pushneeds a remote name;getalso needs a destination, credentials config, and the flags the run was invoked with.submit_workers/monitor_jobs/load_worker_partition— unchanged. The only accommodation needed was makingdt get'sREPOSITORYpositional optional, since a worker reads everything from the manifest.The docstring on
partition_by_sizerecords why LPT belongs to qxub workers and not to aThreadPoolExecutor, so the next person doesn't apply it to a thread pool: separate processes cannot steal work, a shared queue self-corrects, and freezing an assignment the queue would fix is strictly worse.The design questions, settled
Reachability — resolve once on the submitter and pass paths. The submitter has to resolve every row anyway (LPT needs byte sizes;
list_source_filesis where sizes come from), so having paid for that it ships the resolved md5, size and destination key per file. No worker ever runsdvc list— which is what keeps N compute nodes off the shared clone's SQLite state db._run_dvc_list's retry loop exists because that lock is real. A worker opens the clone once, read-only, for one thing: the source remote's filesystem and credentials out of its.dvc/config.Link types and the destination identity are resolved on the submitter too, not re-derived per worker.
resolve_link_types()readsdvc configfrom the cwd andpreflight()resolves a credential chain; both could answer differently on a compute node, and a transfer that silently changes its mind halfway is worse than one that fails.Partition unit — per file, not per row. The issue framed this as a trade against per-row reporting, but it isn't one: each task carries its row tag, so reporting stays per-row and in CSV order even when a sample directory spans nodes. Per-file also balances properly in the two cases per-row cannot — a single row of thousands of files, and rows differing by an order of magnitude, which fastq samples reliably do.
Reporting — workers write
result_<n>.json; the submitter collates by row tag. A row whose files do not all come back is reported failed even when every result that did arrive succeeded, because a job killed on walltime would otherwise read as a clean run of a short row.Observed end-to-end
Real CLI, real partitioning, real manifest round-trip; fake S3 and fake qxub. Note row
AF013-Awas split across workers 0 and 1 and still reports as one row:Security
Each worker preflights for itself, so
--dest-account-idis enforced on the node that actually writes — a credential chain can resolve differently on a compute node. The manifest carries the profile name, endpoint and region; no key material. There's a test asserting that.Also
The issue noted the
ThreadPoolExecutoridiom copy-pasted five times, with "if a sixth copy is about to appear, factor it then". This adds a sixth use, soget.py's two are now behind_place_alland_upload_all, and_resolve_rowsis shared by the local and S3 row-resolve paths. Side effect:upload_to_s3now resolves rows concurrently, which it did not before. A cross-moduleutils.run_parallel()is still not worth it.Still open — please read before merging
Whether more nodes actually move more bytes is not verified here, and needs a real submission. #172 asks for that measurement and I could not do it: it needs real data, a queue slot and time. It also isn't only about Lustre on the write side — if the source remote is on
/g/datarather than object storage, the read side is a shared filesystem again and the same contention argument applies to it.docs/get.mdsays so, and says to time a single-node-j 8run and watch for saturation before scaling up.So: this ships the tool to do the measurement with, not the conclusion.
Tests
30 new unit tests. The central one replaces
hpc.submit_workerswith a stub that runs the worker in-process, so the manifest is exercised as a real interface rather than asserted against its spelling. Full unit suite: 2381 passed, 2 skipped, 0 failed — run on a compute node viaqxub exec --env dt -- pytest tests/unit -q(2:03).For the record, since an earlier version of this description said otherwise: a run on the login node reported 1 failure in
test_auth_credentials_aws.py(FileNotFoundError: 'dvc'). That was an artefact of my test invocation, not a pre-existing failure and not this change — I had bypassedconda run, which leaves the env'sbinoff PATH, so anything shelling out todvcdies. It passes in the real env.🤖 Generated with Claude Code