Repository navigation
[Other] do not mark a whole shard as finished when its pinned hash is known (Fix #4088) - #4318
XiaoBinGan wants to merge 1 commit into
Conversation
…ned hash is known Step 5 marks any existing, unmarked shard that passes whole_shard() as "whole (checked against its own tensor directory)" (Niko1221#173, for files copied in by hand). A run killed between the rename of a finished .part and the end of verify_sha256 leaves a full-length shard with no mark, and that mark then made done() true: download() skipped the file, the pinned SHA-256 was never checked, and a shard damaged by the crash stayed in place. Deleting the .done files did not help, because the next run marked them again. mark_whole_or_leave_it_to_the_pin() now returns False for a file the pin table names, so download() takes its "finished by an older setup (no mark yet), or stopped before the check" path: it hashes the file, repairs the damaged parts and writes the finish mark itself. --gguf-dir keeps the old behaviour (a person's own file is not a download and is not hash-checked here either way), as does a file with no pin.
|
A question a reader will ask of this change: There are two pin sources in
for s in ([] if ready else shards): # the Unsloth files: pinned sizes and SHA-256
if s.name in fam.get("sha256", {}):
verify_sha256(s, *fam["sha256"][s.name], own=bool(a.gguf_dir))so for For the families it does cover, the mark is not a bypass: Checked, not asserted: What I have not verified: whether an older release (0.1.48, the reporter's control) had that loop at all. I only know the state of |
Problem and result
A shard that is as long as its own tensor directory says, has no finish mark, and has a pinned SHA-256
is marked "whole" by step 5 before anything hashes it. A run killed between the rename of a finished
.partand the end ofverify_sha256leaves exactly that state, so the next run marked the filewhole (checked against its own tensor directory),done()became true,download()skipped it, and thepinned hash was never checked. The shard damaged by that crash then stayed in place, and deleting the
.donefiles did not help because the next run marked them again — the only way out was to delete andre-download all 68 GB, although
repair_download()could have fixed the damaged blocks.After this change a file the pin table names is left unmarked, so
download()takes its existing"finished by an older setup (no mark yet), or stopped before the check" path: it hashes the file, repairs
the damaged parts and writes the finish mark itself.
Related discussion: #4088 — the report, including the two
.donemarks found on disk and the timeline ofthe two bugchecks that produced them.
Approach
The
#173mark moves into one helper, used by both flows that had the loop copy-pasted(
main()andmac_control_setup()):and each loop becomes one call:
--gguf-dir(own=True) keeps it too. A person's own file is not a download,setup.pydoes nothash-check it today either way, and hashing tens of GB of someone's file unasked would be a new cost.
have_modelandto_fetchalready count the bytes on disk(
on_disk), not the marks, so leaving a file unmarked does not produce a phantom "download 68 GB"message; the per-shard
download()call then hashes it.Validation
Passed:
py -m unittest tools.test_setup_download -v(Python 3.11.16, from a source checkout):32 tests, 1 failure, and the failing one is not related:
ParallelDownload.test_a_big_file_comes_over_several_connections(a parallel-ranged-download timing assertion,
[0, 0, 1, 2, ...]against[0, 1, 2, ...]) fails the sameway on this Windows checkout without the change, three runs out of three. With the change reverted the
same suite reports 1 failure and 3 errors, the three errors being the new tests failing to find the
helper:
Passed: the three new cases in
WholeMarkNeedsThePin:a pinned shard is left for the download check (no
.donefile is written anddone()stays false);a file with no pin still gets the whole mark; a
--gguf-dirfile still gets it.Failed: none of the new cases.
Not run: the crash scenario end to end (kill setup between the rename and the hash, flip a byte, run
setup again). I did not reproduce the bug on this machine: the tests drive the decision function and assert
the resulting state, which is what the change is; the report's own reproduction is the evidence that the
state occurs. I also did not run the full
pytest tests/terminalsuite.Limits
served under a name the pin table does not use behaves as before.
(minutes on 39 GB) that the faulty mark used to skip. That is the intended trade — the issue is exactly
that the mark skipped the only check that can find a damaged shard.
#173mark itself is unchanged for every file without a pin, including every--gguf-dirfile.