Skip to content

[Other] do not mark a whole shard as finished when its pinned hash is known (Fix #4088) - #4318

Open
XiaoBinGan wants to merge 1 commit into
Niko1221:mainfrom
XiaoBinGan:fix/whole-mark-pin-check
Open

XiaoBinGan wants to merge 1 commit into
Niko1221:mainfrom
XiaoBinGan:fix/whole-mark-pin-check

Conversation

@XiaoBinGan

@XiaoBinGan XiaoBinGan commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

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
.part and the end of verify_sha256 leaves exactly that state, so the next run marked the file
whole (checked against its own tensor directory), done() became true, download() skipped it, and the
pinned hash was never checked. The shard damaged by that crash then stayed in place, and deleting the
.done files did not help because the next run marked them again — the only way out was to delete and
re-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 .done marks found on disk and the timeline of
the two bugchecks that produced them.

Approach

The #173 mark moves into one helper, used by both flows that had the loop copy-pasted
(main() and mac_control_setup()):

def mark_whole_or_leave_it_to_the_pin(s: Path, url: str, own: bool) -> bool:
    if not own and file_pin(url) is not None:
        return False
    try:
        mark(s, "whole (checked against its own tensor directory)")
    except OSError as e:                               # a read-only folder: the file is still whole
        warn(f"{s.name} is whole but its finish mark cannot be written ({e})")
    return True

and each loop becomes one call:

for s in ([] if ready else shards):
    if s.exists() and not done(s) and whole_shard(s):
        mark_whole_or_leave_it_to_the_pin(s, fam["hf"].format(q=model) + s.name, bool(a.gguf_dir))
  • A file with no pin keeps the old behaviour: hand-copied shards of an unlisted build still get the mark.
  • --gguf-dir (own=True) keeps it too. A person's own file is not a download, setup.py does not
    hash-check it today either way, and hashing tens of GB of someone's file unasked would be a new cost.
  • Nothing else in step 5 changes: have_model and to_fetch already 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 same
way 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:

AttributeError: module 'setup' has no attribute 'mark_whole_or_leave_it_to_the_pin'

Passed: the three new cases in WholeMarkNeedsThePin:
a pinned shard is left for the download check (no .done file is written and done() stays false);
a file with no pin still gets the whole mark; a --gguf-dir file 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/terminal suite.

Limits

  • The pin is looked up from the URL the loop already builds for that shard, so a model whose files are
    served under a name the pin table does not use behaves as before.
  • This changes what a finished state looks like after a crash: the next run will spend the hash time
    (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.
  • The #173 mark itself is unchanged for every file without a pin, including every --gguf-dir file.

…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.
@XiaoBinGan

Copy link
Copy Markdown
Contributor Author

A question a reader will ask of this change: main() has a pinned-verification loop after the download loop, so is the "whole" mark really the only check? On current main the answer is yes for the family the report names, and here is why — worth having in the thread rather than in a later reviewer round.

There are two pin sources in setup.py, and the pre-pass is only a problem for one of them:

family      fam['sha256'] entries    hf repo
qwen            0                   ISTA-DASLab/Qwen3.8-Flash-Next-GSQ-RCO-GGUF
swift           0                   ukisai/Swift-1.5-Qwen3.8-Flash-Next-GSQ-RCO-GGUF
coder           0                   ISTA-DASLab/Qwen3.8-Flash-Next-GSQ-RCO-Coder-GGUF
gyro            2                   agentionai/Qwen3.8-Flash-Next-Gyro-GGUF
unsloth        25                   unsloth/Qwen3.8-Flash-Next-GGUF

REPO_PINS (76 files) covers all of them, fam["sha256"] covers only gyro and unsloth. The loop after the download loop is

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 qwen, swift and coder — ISTA-DASLab's files, the default recommendation, and the family in #4088 (IQ2_XS) — that loop iterates and does nothing. The only hash check those files have is the one inside download(), and download() returns at its first line when done(dst) is true, which is exactly what the pre-pass made true.

For the families it does cover, the mark is not a bypass: verify_sha256() returns early only when the mark holds sha256 <hash>, and "whole (checked against its own tensor directory)" is not that, so it re-hashes anyway. That is why this change only has to touch the pre-pass, and why it is not a second verification pass bolted on.

Checked, not asserted: fam['sha256'] counts above come from setup.FAMILIES in this checkout of main, and the verify_sha256 early-return line is if m.exists() and f"sha256 {sha}" in m.read_text(...): return.

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 main at 4b9e05a, which is what this PR changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant