refactor fastcp/fastmd5's direct_io and file system walking code to a shared location so they can be used by other utilities. - #43
Open
matthew-frank wants to merge 8 commits into
Open
Conversation
fastcp and fastmd5 each carried their own copy of the aligned-buffer I/O and the source-tree walk, so a fix to one did not reach the other. Move both into mlperf_common.fileio, where anything else that needs them can import them too. The package is deliberately stdlib-only -- no torch, no numpy -- so that the single-node client scripts can use it without pulling in a training stack. copyplan.plan_copy_operations reports every unreadable entry rather than dying on the first, and refuses a tree it cannot fully list. os.walk's default is to swallow that and carry on, which makes an unlistable subtree indistinguishable from an empty one: a partial copy that exits 0, and then a checksum run that skips the same files and agrees with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both scripts now import direct_io and copyplan from mlperf_common.fileio instead of carrying private copies, so the two agree by construction about which files a copy covers -- which matters because fastmd5 is what verifies a tree fastcp wrote. Checksumming a different set of files than was copied would look exactly like success. client/direct_io.py becomes a compatibility shim re-exporting the package module, so `import direct_io` keeps working for anything that still expects a module sitting next to fastcp. The sys.path dance in each script covers both ways these are run: a pip install that puts the package and the scripts together, and a source tree with mlperf_common/ next to client/, which is the "deploy the repo to a shared filesystem and run in place" pattern. Copying a single script out of client/ on its own does not work; preserve that when touching the imports. fastmd5's output is byte-identical to the pre-refactor version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_copyplan.py checks the src->dst mapping against what GNU cp actually does, and checks that an unlistable directory makes planning fail loudly instead of quietly yielding a short file list. copyplan is stdlib-only, so the test imports it straight out of the checkout; there is nothing to stub. run_tests.py still runs each file in its own interpreter, so nothing one test leaves in sys.modules can reach another. direct_io has no test of its own. Its retry loops and block-size padding are exercised only indirectly, by running fastcp and fastmd5 against a real filesystem. tests/README.md says so rather than leaving the gap implied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
plan_copy_operations reported only files, so a directory with nothing under it was never created: nothing in the file list implied it. `fastcp -r src newdir` where src holds an empty subdir produced a destination quietly missing it, and a wholly empty source produced no jobs at all, so newdir was never created and fastcp still exited 0. cp -r creates it in both cases. This was a regression the fileio extraction introduced. Before it, the same input failed loudly -- the old planner tried to copy the directory as a file and died with "Failed to open source file ... Invalid argument", exit 1 -- so a caller checking the exit status noticed. Exiting 0 with an incomplete tree is strictly worse, and it is the shape this repo cares about most: fastmd5 emits no line for a directory either, so a checksum comparison of the two trees also reports them identical. plan_copy_operations now returns a CopyPlan(files, directories). The directories come from the same os.walk that already yields dirnames and threw them away, so finding them costs no extra metadata work on the shared filesystem -- walking a second time would have doubled it. They are sorted shallowest first, so creating them in order never needs an absent parent, and they include each file's parent, which lets fastcp create the tree up front instead of calling makedirs once per file. list_relative_files keeps its signature and is now a wrapper over list_relative_entries; fastmd5 wants only the files and is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three were introduced or preserved by trimming CLAUDE.md for this branch, and all three are checkable and were wrong. BUFFER_ALIGN was described as "the shared alignment constant across fastcp and fastmd5". It does not exist here: it was defined in datastage.py, which this branch does not carry, and grep matched nothing but the sentence asserting it. 2 MiB is hardcoded in four places across the two scripts. Say that instead, and note that direct_io is where it belongs. CLAUDE.md listed fastmd5 and dropcache among the scripts setup.py installs. It installs neither. That is worth stating rather than quietly correcting, because this branch makes fastmd5 hard-depend on the package while its own ImportError text advises "install mlperf-common" -- a remedy that cannot produce a fastmd5. copyplan's docstring still justified its sort order by "every rank of a collective copy", which has no consumer on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three, all reproduced, all absent from main's pre-refactor fastcp: the friendly "cannot import mlperf_common" advisory is dead code because the unguarded import above it fails first; a leftover standalone direct_io.py in an install's bin/ outranks the new shim, which matters because setup.py installs that file and an upgrade leaves the old one behind; and test_copyplan's refusal rows assert only that some CopyArgumentError escaped, not which. Deliberately excludes the longer list of defects inherited from main -- direct_io.py is byte-identical there, and fastcp's destructive cases (same-file -f, src/.., -n 0) reproduce identically against main. Those predate this branch and are tracked elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ten of them, severity-ordered, each with a runnable reproduction against `git show main:client/fastcp` rather than against this branch, because none is caused by the fileio extraction -- they all predate it, and direct_io.py is byte-identical across the two. Two lose data silently and exit 0: `fastcp -f a.bin .` truncates the source it was asked to copy, and `fastcp -r -f src/.. dst` writes every destination back outside DEST onto the sources themselves. A third pair compounds -- `-n 0` produces a full-size all-zero destination while `fastmd5 -n 0` produces no output, so the checksum comparison that would have caught it reports the two trees identical. This goes in the tree rather than a bug tracker because the repo has GitHub issues disabled. It is the record until there is somewhere better; the header says to move it and delete this file when that happens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
That commit message says the bug "was a regression the fileio extraction introduced". Only one of its two cases was. With DEST already a directory, main silently omits an empty subdirectory and exits 0, and the extraction carried that across unchanged -- a pre-existing defect, now recorded in MAIN-BUGS.md alongside the others main has. Only the wholly-empty-source case regressed, where main exited 1 and the extraction turned that into exit 0 with the destination never created. The fix in 01f4634 is correct for both; only its description was too broad. Correcting it here rather than force-pushing a reworded commit onto an open PR. Co-Authored-By: Claude Opus 5 <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
1 participant
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.
No description provided.