diff --git a/tests/parity/hf_snapshot.h b/tests/parity/hf_snapshot.h index c5478bc73..f0e2ff311 100644 --- a/tests/parity/hf_snapshot.h +++ b/tests/parity/hf_snapshot.h @@ -20,8 +20,10 @@ // pass. Publishers re-quantize in place; a correctness gate must name the // revision its golden belongs to. +#include #include #include +#include #include #include @@ -72,6 +74,106 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, return snap.string(); } +// GATE-SNAPSHOT-CONTENT-PIN (issue #569). The revision the bytes of +// `/` were actually downloaded as, or "" when nothing records +// it. +// +// A `hf download --local-dir` tree keeps one sidecar per file at +// `/.cache/huggingface/download/.metadata`, three lines: +// commit hash, etag, timestamp. That layout is the library's own -- see the +// `huggingface_hub/_local_folder.py` module docstring, and +// `read_download_metadata`, which parses the three back in that order into +// `LocalDownloadFileMetadata.commit_hash`. +// +// Line 1 is the record this header needs, and the reason it is trustworthy is +// that it is rewritten IN PLACE on every download: +// `huggingface_hub/file_download.py` calls +// `write_download_metadata(..., commit_hash=commit_hash, ...)` on all four +// outcomes -- fresh download, copy out of the shared cache, LFS hash matched, +// and the `local_metadata.etag == etag` early return that downloads nothing at +// all. So after re-fetching a repo at a new revision, every file's sidecar names +// the new one, including files whose bytes did not change. +inline std::string HfLocalDownloadCommit(const std::filesystem::path& local_dir, + const std::string& name) { + std::ifstream in(local_dir / ".cache/huggingface/download" / + (name + ".metadata")); + std::string line; + if (!std::getline(in, line)) return ""; + while (!line.empty() && (line.back() == '\r' || line.back() == '\n')) { + line.pop_back(); + } + return line; +} + +// True when EVERY file staged directly in `local_dir` was downloaded at +// `revision`. `why`, when non-null, receives the first thing that was wrong, so +// the caller's skip banner can name it. +// +// WHY EVERY FILE AND NOT JUST `config.json`. The cheap version of this check +// reads one sidecar, and one sidecar answers a question nobody asked: the +// goldens depend on the WEIGHTS. `hf download config.json --revision B` +// into a tree fetched at A leaves a config from B beside 52 shards from A, and +// the reverse -- shards re-fetched at B under a config still recording A -- is +// the substitution that matters and the one a config-only check cannot see. The +// sweep costs 69 sidecar reads of ~130 bytes each against a 20.1 GiB checkpoint +// the gate is about to load, so there is nothing to trade off. +// +// It is symmetric on purpose. A file with NO sidecar refuses just as a file with +// the wrong one does, because otherwise substituting a shard would cost one `rm` +// of its sidecar, and a foreign file dropped into the tree would carry no +// provenance at all. Directories are skipped -- `.cache` is the bookkeeping +// itself -- and a tree with no staged files refuses rather than passing +// vacuously. +inline bool HfLocalDirIsRevision(const std::filesystem::path& local_dir, + const std::string& revision, + std::string* why) { + namespace fs = std::filesystem; + std::error_code ec; + const fs::directory_iterator end; + fs::directory_iterator it(local_dir, ec); + if (ec) { + if (why != nullptr) { + *why = "cannot list " + local_dir.string() + ": " + ec.message(); + } + return false; + } + std::size_t checked = 0; + for (; it != end; it.increment(ec)) { + if (ec) { + if (why != nullptr) { + *why = "cannot list " + local_dir.string() + ": " + ec.message(); + } + return false; + } + std::error_code kind; + if (!it->is_regular_file(kind) || kind) continue; + const std::string name = it->path().filename().string(); + const std::string got = HfLocalDownloadCommit(local_dir, name); + if (got.empty()) { + if (why != nullptr) { + *why = name + " has no .cache/huggingface/download/" + name + + ".metadata, so nothing records which revision its bytes are"; + } + return false; + } + if (got != revision) { + if (why != nullptr) { + *why = name + " was downloaded at revision " + got + ", not " + + revision; + } + return false; + } + ++checked; + } + if (checked == 0) { + if (why != nullptr) { + *why = "no files staged in " + local_dir.string(); + } + return false; + } + return true; +} + // The Nemotron-3.5-Lightning gate model (#517), and the ONE resolver for it. // // Unlike the Qwen pins above, this one is NOT in the HF cache: it is staged on @@ -86,9 +188,36 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, // an env override is deliberately not revision-checked. So the pin named the // revision the goldens belong to and could not refuse a different one, which is // the exact failure `kQwen27NvfP4Revision` exists because of. Both spellings now -// resolve HERE, and the `local_dir` layout does record its revision, just not in -// the path: `hf download --local-dir` writes a per-revision file manifest at -// `/.cache/huggingface/trees/.json`. That is what is checked. +// resolve HERE. +// +// GATE-SNAPSHOT-CONTENT-PIN (issue #569). LOW-3 first gated this arm on the +// EXISTENCE of `/.cache/huggingface/trees/.json`, the manifest +// `hf download --local-dir` writes per revision. That records "this revision was +// downloaded into this directory once". It does not record "these bytes are that +// revision", and DEMONSTRATED during review: a directory holding a different +// model's `config.json`, whose own +// `.cache/huggingface/download/config.json.metadata` named a different revision, +// still resolved -- with the manifest an empty `touch`ed file and a decoy +// manifest beside it changing nothing. +// +// The two records come apart, and huggingface_hub is explicit about why. +// `_tree_cache.py` (1.23.0 / 1.24.0, the versions on the gate host that wrote +// this tree; 1.7.2 here has no `trees` code at all) opens with "Because a commit +// hash is immutable, its tree listing never changes and can be cached forever +// WITHOUT ANY INVALIDATION LOGIC". `write_tree_cache` only ever `os.replace`s +// `.json`, and no code path in the library removes one -- the string +// `trees` appears exactly five times in it, all reads and path joins. So +// manifests ACCUMULATE: re-download at a later revision and the pinned +// revision's manifest is still sitting there, vouching for bytes that are no +// longer it. The existence check could not have been salvaged; it is not a +// weaker version of the right check, it is a check of a different fact. +// +// The per-file sidecar is the right fact and it does not accumulate -- see +// `HfLocalDownloadCommit` above for the four call sites that rewrite it. The +// manifest check is therefore REPLACED rather than kept as an extra term: as an +// AND-term it can no longer refuse anything the sidecar sweep does not already +// refuse, while it CAN refuse a good tree fetched by a huggingface_hub older +// than 1.23, which writes sidecars and no manifest at all. // // Resolution order, in the order the code checks it, and why: // @@ -97,11 +226,12 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, // that a set-but-wrong override refuses rather than falling back. First, so // that setting it OVERRIDES `CHECKPOINT_ROOT` rather than racing it. // 2. Otherwise `CHECKPOINT_ROOT` -> `/`, -// and the revision manifest MUST be present. This is the DEFAULT path every -// gate takes, so it is the one that has to carry the pin: a re-download of -// the same repo name lands a different revision under the identical path, -// and a gate that cannot tell would substitute it silently. Missing -// manifest => "" => the caller's loud skip, never a substitution. +// and EVERY file staged there must record the pinned revision as the one it +// was downloaded at. This is the DEFAULT path every gate takes, so it is the +// one that has to carry the pin: a re-download of the same repo name lands a +// different revision under the identical path, and a gate that cannot tell +// would substitute it silently. Any mismatch => "" plus a reason in `why` +// => the caller's loud skip, never a substitution. // 3. Otherwise the ordinary HF cache layout, for a host that fetched it that // way. // @@ -111,23 +241,41 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, // // Absent both env vars => "" => the caller emits its loud SKIP, which is the // intended behavior off the gate host. -inline std::string Nemotron35LightningSnapshot() { +// +// `why`, when non-null, receives the reason for a "" -- an empty string when the +// answer is non-empty. A token-exact gate that silently loaded the wrong +// checkpoint is the failure this header exists to prevent, and a skip that says +// only "not staged" gets re-run rather than investigated. +inline std::string Nemotron35LightningSnapshot(std::string* why = nullptr) { namespace fs = std::filesystem; std::error_code ec; + if (why != nullptr) why->clear(); const char* over = std::getenv("VT_NEMOTRON35_SNAPSHOT"); const char* root = std::getenv("CHECKPOINT_ROOT"); if ((over == nullptr || *over == '\0') && root != nullptr && *root != '\0') { const fs::path dir = fs::path(root) / kNemotron35LightningLocalDirName; - const fs::path tree = dir / ".cache/huggingface/trees" / - (std::string(kNemotron35LightningNvfP4Revision) + - ".json"); - if (!fs::exists(dir / "config.json", ec)) return ""; - if (!fs::exists(tree, ec)) return ""; + if (!fs::exists(dir / "config.json", ec)) { + if (why != nullptr) *why = "no config.json under " + dir.string(); + return ""; + } + // Bound to a local rather than passed inline: tests/scripts/ + // test_check_snapshot_pins.py asserts each `k*Revision` constant is followed + // by a comma EXACTLY once in this header, so that a pin cannot exist without + // an accessor resolving through it. That single use is the HfSnapshot call + // below, and it stays the single use. + const std::string want = kNemotron35LightningNvfP4Revision; + if (!HfLocalDirIsRevision(dir, want, why)) return ""; return dir.string(); } - return HfSnapshot("models--nvidia--NVIDIA-Nemotron-3.5-Lightning-30B-A3B-NVFP4", - kNemotron35LightningNvfP4Revision, - "VT_NEMOTRON35_SNAPSHOT"); + const std::string resolved = HfSnapshot( + "models--nvidia--NVIDIA-Nemotron-3.5-Lightning-30B-A3B-NVFP4", + kNemotron35LightningNvfP4Revision, "VT_NEMOTRON35_SNAPSHOT"); + if (resolved.empty() && why != nullptr) { + *why = std::string("neither VT_NEMOTRON35_SNAPSHOT nor CHECKPOINT_ROOT " + "resolves a checkpoint at revision ") + + kNemotron35LightningNvfP4Revision; + } + return resolved; } // The 27B NVFP4 gate model, pinned to the goldens' revision. diff --git a/tests/parity/test_hf_snapshot_pinning.cpp b/tests/parity/test_hf_snapshot_pinning.cpp index b15bac4e1..e04ed85a6 100644 --- a/tests/parity/test_hf_snapshot_pinning.cpp +++ b/tests/parity/test_hf_snapshot_pinning.cpp @@ -58,6 +58,31 @@ class ScopedHome { bool had_prev_ = false; }; +// A variable forced UNSET for the duration. The local_dir arm +// below is only reached when `VT_NEMOTRON35_SNAPSHOT` is unset, and a developer +// who exports it in their own shell would otherwise silently take a DIFFERENT +// branch than the one the case is about -- which reads as a pass. +class ScopedUnsetEnv { + public: + explicit ScopedUnsetEnv(const char* name) : name_(name) { + const char* prev = std::getenv(name); + had_prev_ = prev != nullptr; + if (had_prev_) prev_ = prev; + unsetenv(name_); + } + ~ScopedUnsetEnv() { + if (had_prev_) setenv(name_, prev_.c_str(), /*overwrite=*/1); + } + + ScopedUnsetEnv(const ScopedUnsetEnv&) = delete; + ScopedUnsetEnv& operator=(const ScopedUnsetEnv&) = delete; + + private: + const char* name_; + std::string prev_; + bool had_prev_ = false; +}; + // Same for an env override, so an unset variable stays unset afterwards. class ScopedEnv { public: @@ -109,6 +134,59 @@ constexpr const char* kUnsloth27bRepo = "models--unsloth--Qwen3.6-27B-NVFP4"; constexpr const char* kUnsloth27bDecoyRevision = "ccdaab7e68af2409599b8949a8f2685703c9bae5"; +// ------------------------------------------------------------------------- // +// GATE-SNAPSHOT-CONTENT-PIN (issue #569): the `hf download --local-dir` arm. +// ------------------------------------------------------------------------- // + +// One `.cache/huggingface/download/.metadata` sidecar, in the layout +// huggingface_hub actually writes: commit hash, etag, timestamp, one per line +// (huggingface_hub/_local_folder.py module docstring, and +// `LocalDownloadFileMetadata` / `read_download_metadata` which parse it back in +// that order). +void PlantSidecar(const fs::path& dir, const std::string& file, + const std::string& commit) { + const fs::path meta = dir / ".cache/huggingface/download"; + fs::create_directories(meta); + std::ofstream(meta / (file + ".metadata")) + << commit << "\n" + << std::string(64, 'e') << "\n" + << "1786554554.7491977\n"; +} + +// The per-revision tree listing `hf download --local-dir` leaves behind. The +// body is deliberately a bare `{}`: the DEMONSTRATED failure this row closes had +// an empty `touch`ed manifest, because nothing ever read the contents. +void PlantTreeManifest(const fs::path& dir, const std::string& revision) { + const fs::path trees = dir / ".cache/huggingface/trees"; + fs::create_directories(trees); + std::ofstream(trees / (revision + ".json")) << "{}\n"; +} + +// A staged `hf download --local-dir` tree: the payload files a gate loads, the +// tree manifest for `manifest_revision`, and one sidecar per payload file naming +// `sidecar_revision`. Passing two different revisions is the substitution the +// old existence check could not see. +void PlantLocalDir(const fs::path& dir, const std::string& manifest_revision, + const std::string& sidecar_revision) { + fs::create_directories(dir); + for (const char* file : {"config.json", "hf_quant_config.json", + "model-00001-of-00001.safetensors"}) { + std::ofstream(dir / file) << "{}\n"; + PlantSidecar(dir, file, sidecar_revision); + } + PlantTreeManifest(dir, manifest_revision); +} + +// `$CHECKPOINT_ROOT/`, which is the ONE path the default arm +// resolves. +fs::path StagedCheckpoint(const fs::path& root) { + return root / parity::kNemotron35LightningLocalDirName; +} + +// A revision that is not the pin: the shape of "the publisher re-quantized and +// the same `hf download` command landed different bytes under the same path". +const std::string kOtherRevision(40, 'b'); + } // namespace TEST_CASE("hf_snapshot: the pinned revision is selected out of a two-revision cache") { @@ -224,3 +302,208 @@ TEST_CASE("hf_snapshot: the env override is the ONLY way to gate another checkpo std::string::npos); } } + +// --------------------------------------------------------------------------- // +// GATE-SNAPSHOT-CONTENT-PIN (issue #569). +// +// The Nemotron-3.5-Lightning gate model is staged as a `hf download --local-dir` +// tree, so the revision is not in the PATH. The pin used to be the EXISTENCE of +// `.cache/huggingface/trees/.json`, which records "this revision was +// downloaded into this directory once" and never "these bytes are that +// revision". The two come apart, and huggingface_hub says so itself: +// +// * `_tree_cache.py` (1.23.0/1.24.0, the versions that wrote the manifest on +// the gate host) opens with "Because a commit hash is immutable, its tree +// listing never changes and can be cached forever WITHOUT ANY INVALIDATION +// LOGIC", and `write_tree_cache` only ever `os.replace`s `.json`. +// Nothing in the library removes one. Manifests ACCUMULATE, so after a +// re-download at a new revision the OLD manifest is still sitting there +// vouching for the NEW bytes. +// * The per-file sidecar does not accumulate. `file_download.py` calls +// `write_download_metadata(..., commit_hash=...)` on EVERY outcome -- +// fresh download, cache copy, and the etag-already-matches early return -- +// so line 1 of `.cache/huggingface/download/.metadata` always names +// the revision the bytes on disk were fetched as. +// +// So the manifest is the wrong record and the sidecar is the right one. These +// cases pin that, and they are the reason W6 of `.agents/specs/nemotron-h-model.md` +// (#517) may be believed: a token-exact gate is precisely the instrument that +// CANNOT see a substituted checkpoint. +// --------------------------------------------------------------------------- // + +TEST_CASE("hf_snapshot: a local_dir whose sidecars name ANOTHER revision refuses") { + const fs::path root = ScratchRoot("nemotron_substituted"); + ScopedHome guard(root); // also cleans `root` up on scope exit + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // The DEMONSTRATED failure, staged exactly: the pinned revision's manifest is + // present, and every file in the tree was actually downloaded at a different + // one. An existence check on the manifest resolves this directory. + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + kOtherRevision); + + std::string why; + const std::string got = parity::Nemotron35LightningSnapshot(&why); + CHECK(got.empty()); + // The refusal has to SAY what was wrong, or the exit-77 banner the callers + // print degenerates into "not staged" and the operator re-runs it. WHICH file + // is named is readdir order and deliberately not asserted; that it names the + // revision actually on disk is the part an operator needs. + CHECK(why.find(kOtherRevision) != std::string::npos); + CHECK(why.find("not " + + std::string(parity::kNemotron35LightningNvfP4Revision)) != + std::string::npos); +} + +TEST_CASE("hf_snapshot: a RIGHT config beside WRONG weights refuses") { + const fs::path root = ScratchRoot("nemotron_weights_only"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // The case that decides config-only versus per-file, staged as the difference + // between them: `config.json` is genuinely the pinned revision, the SHARD the + // goldens actually depend on is not. `hf download --revision B` + // into a tree fetched at A produces exactly this, and a resolver that read one + // sidecar would hand the gate a mixed checkpoint and call it pinned. + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + PlantSidecar(staged, "model-00001-of-00001.safetensors", kOtherRevision); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("model-00001-of-00001.safetensors") != std::string::npos); + CHECK(why.find(kOtherRevision) != std::string::npos); +} + +TEST_CASE("hf_snapshot: a local_dir whose sidecars name the PIN resolves") { + const fs::path root = ScratchRoot("nemotron_matching"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why) == staged.string()); + CHECK(why.empty()); + // The no-argument spelling every existing caller uses stays identical. + CHECK(parity::Nemotron35LightningSnapshot() == staged.string()); +} + +TEST_CASE("hf_snapshot: an ACCUMULATED manifest does not vouch for newer bytes") { + const fs::path root = ScratchRoot("nemotron_accumulated"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // What `hf download --local-dir` leaves behind after fetching the pin and then + // re-fetching the same repo at a later revision: BOTH manifests, and sidecars + // rewritten in place to the later one. Nothing here is hypothetical -- it is + // what the writer's own "cached forever without any invalidation logic" means. + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, kOtherRevision, kOtherRevision); + PlantTreeManifest(staged, parity::kNemotron35LightningNvfP4Revision); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find(kOtherRevision) != std::string::npos); +} + +TEST_CASE("hf_snapshot: a local_dir file with NO sidecar refuses") { + const fs::path root = ScratchRoot("nemotron_unrecorded"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + + SUBCASE("a shard dropped in by hand carries no provenance at all") { + // The check is symmetric on purpose. If only the files that HAVE a sidecar + // were checked, substituting a checkpoint would cost one `rm` of the + // sidecar -- and the file whose bytes the goldens depend on is exactly the + // one an attacker or an accident replaces. + std::ofstream(staged / "model-00002-of-00002.safetensors") << "{}\n"; + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("model-00002-of-00002.safetensors") != std::string::npos); + // The MESSAGE is asserted, not only the refusal. Falling through to the + // wrong-revision branch also refuses -- "" never equals the pin -- so + // without this the no-sidecar branch could be deleted outright and every + // case here would still pass while the banner told the operator the file + // "was downloaded at revision , not 29f2d174...", which is not what + // happened and sends them looking for the wrong thing. + CHECK(why.find("has no .cache/huggingface/download/") != std::string::npos); + } + SUBCASE("deleting a sidecar is not a way to pass") { + fs::remove(staged / ".cache/huggingface/download/config.json.metadata"); + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("config.json") != std::string::npos); + CHECK(why.find("has no .cache/huggingface/download/") != std::string::npos); + } +} + +TEST_CASE("hf_snapshot: a tree with no staged FILE refuses rather than passing") { + const fs::path root = ScratchRoot("nemotron_vacuous"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // `config.json` as a DIRECTORY. `fs::exists` says yes, so the arm gets past + // the staging probe, and the sweep then has zero regular files to check. A + // sweep with nothing to check must not report success: "every file matches" + // is vacuously true of no files, and that is the one way a content pin can go + // quiet without anyone deleting a line of it. + const fs::path staged = StagedCheckpoint(root); + fs::create_directories(staged / "config.json"); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("no files staged") != std::string::npos); +} + +TEST_CASE("hf_snapshot: the local_dir arm keeps the behaviors it already had") { + const fs::path root = ScratchRoot("nemotron_preserved"); + ScopedHome guard(root); + + SUBCASE("no CHECKPOINT_ROOT and no override -> refuse") { + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedUnsetEnv no_root("CHECKPOINT_ROOT"); + CHECK(parity::Nemotron35LightningSnapshot().empty()); + } + SUBCASE("CHECKPOINT_ROOT set but nothing staged under it -> refuse") { + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + CHECK(parity::Nemotron35LightningSnapshot().empty()); + } + SUBCASE("the override names a directory outright and wins over the root") { + // Unchanged semantics: naming ONE directory is the deliberate + // different-checkpoint run, and it is not revision-gated. Naming a ROOT is + // not, which is why only the root arm carries the content pin. + const fs::path elsewhere = root / "deliberate-checkpoint"; + fs::create_directories(elsewhere); + std::ofstream(elsewhere / "config.json") << "{}\n"; + PlantLocalDir(StagedCheckpoint(root), + parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + ScopedEnv over("VT_NEMOTRON35_SNAPSHOT", elsewhere.string()); + CHECK(parity::Nemotron35LightningSnapshot() == elsewhere.string()); + } + SUBCASE("a set-but-wrong override refuses rather than falling back") { + PlantLocalDir(StagedCheckpoint(root), + parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + ScopedEnv over("VT_NEMOTRON35_SNAPSHOT", (root / "nonexistent").string()); + CHECK(parity::Nemotron35LightningSnapshot().empty()); + } +} diff --git a/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp b/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp index 1f6709ef1..70e92209a 100644 --- a/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp +++ b/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp @@ -51,10 +51,18 @@ namespace { // staging directory name by hand, which meant two env vars — // `VT_NEMOTRON35_SNAPSHOT` there, `CHECKPOINT_ROOT` here — reached one // checkpoint and neither refused a revision the goldens were not captured -// against. Both spellings now go through that resolver, which gates the staged -// `local_dir` on its own `.cache/huggingface/trees/.json` manifest, so -// a re-download landing a different revision under the identical path SKIPS -// here rather than being silently substituted. +// against. Both spellings now go through that resolver. +// +// GATE-SNAPSHOT-CONTENT-PIN (#569). That resolver gates the staged `local_dir` +// on the per-file `.cache/huggingface/download/.metadata` sidecars, whose +// first line is the revision the bytes were downloaded at, so a re-download +// landing a different revision under the identical path SKIPS here rather than +// being silently substituted. It used to gate on the presence of +// `.cache/huggingface/trees/.json`, which records only that the +// revision was fetched here once and is never deleted when a later one lands. +// The resolver now hands back the reason it refused, and the banner below prints +// it verbatim -- a skip that does not say WHICH file named WHICH revision gets +// re-run instead of investigated. // // HOW TO MAKE THIS ARM RUN. `CHECKPOINT_ROOT` is a `.env` key, and `.env` is not // exported into a login shell by anything: `.env.example:8` documents the loader @@ -65,12 +73,13 @@ namespace { // has not sourced it SKIPS this arm, which is correct behavior and not a pass — // the banner below names the exact export. std::string CheckpointFile(const char* filename) { - const std::string snapshot = parity::Nemotron35LightningSnapshot(); + std::string why; + const std::string snapshot = parity::Nemotron35LightningSnapshot(&why); if (snapshot.empty()) { SkipGate( std::string("no pinned Nemotron-3.5-Lightning snapshot, so ") + - filename + - " cannot be located.\n" + filename + " cannot be located.\n*** REFUSED BECAUSE: " + why + + "\n" "*** To RUN this arm, load the repo env first — `.env.example:8`\n" "*** documents exactly this:\n" "*** set -a; . ./.env; set +a\n" @@ -81,10 +90,11 @@ std::string CheckpointFile(const char* filename) { "*** $CHECKPOINT_ROOT/" + parity::kNemotron35LightningLocalDirName + "/{config,hf_quant_config}.json — no weights, no GPU.\n" - "*** A staged directory that IS present skips too unless it carries\n" - "*** .cache/huggingface/trees/" + + "*** A staged directory that IS present skips too unless EVERY file\n" + "*** in it records revision " + parity::kNemotron35LightningNvfP4Revision + - ".json,\n" + "\n" + "*** on line 1 of its .cache/huggingface/download/.metadata —\n" "*** the revision the committed goldens were captured against."); } const std::filesystem::path p = std::filesystem::path(snapshot) / filename;