Reuse an already-extracted checkpoint on the unrefactored branch too - #2
Conversation
* Reuse an already-extracted checkpoint instead of re-fetching it.
The loader re-checked the archive against its remote and re-extracted it
on every run. The 2018 model archives now answer 403, and the file left
behind is the error page, so extraction fails and no driver depending on
this loader can start, even though the extracted weights are present and
readable.
A present, non-empty checkpoint directory now short-circuits the size
check and the extraction. The download path is otherwise unchanged.
Two properties worth knowing. The guard trusts what is on disk without
verifying it, so a corrupted extraction is now used silently rather than
overwritten. And a run depends on prior state, so a result is only
interpretable together with what was already extracted.
* Derive the directory as the loader does, so the guard matches tarballs.
`rsplit('.', 1)` left a trailing `.tar` for the ALBERT archives, whose
suffix is `.tar.gz`, so the reuse guard never matched for them and every
run still paid a remote check. `load_bert_param` already splits at the
first dot to name the same directory; use that.
(cherry picked from commit 868ff06)
There was a problem hiding this comment.
🟡 Not ready to approve
The new reuse guard derives the extracted directory name in a way that likely won’t match ALBERT’s expected directory layout and can also short-circuit on incomplete extractions, breaking ALBERT startup and/or causing downstream FileNotFoundError.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the checkpoint loader to reuse an already-extracted BERT/ALBERT checkpoint directory, avoiding repeated remote size checks and re-extraction that can fail once upstream archives stop being served (e.g., returning 403 and leaving an invalid “archive” on disk).
Changes:
- Add an “already extracted” guard to short-circuit remote size checks and extraction when a checkpoint directory is present.
- Derive the checkpoint directory name from the archive filename to decide whether extraction is needed.
File summaries
| File | Description |
|---|---|
| nlpgnn/datas/checkpoint.py | Adds a reuse guard to skip re-downloading/re-extracting checkpoints when an extracted directory already exists. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| extracted = filename.split('.')[0] | ||
| if os.path.isdir(extracted) and os.listdir(extracted): | ||
| self.size = None | ||
| else: |
Companion to #1, which applied the same change to the other branch.
The guard belongs on both branches. Applying it to one alone would mean the two differ in whether their drivers can start at all, and any comparison between them would then be measuring that difference rather than the change under study.
The commit is a cherry-pick of the merged one, so the two branches carry identical loader behaviour.
What It Does
The loader re-checks the archive against its remote and re-extracts it on every run. The 2018 model archives now answer 403, and the file left behind is the error page, so extraction fails with
BadZipFileand no driver depending on this loader can start, even though the extracted weights are present and readable.A present, non-empty checkpoint directory now short-circuits the size check and the extraction. The download path is otherwise unchanged and behaves identically on a clean tree. The directory is derived with
filename.split('.')[0], matchingload_bert_param, so the two sites agree by construction and tarball suffixes are handled.Scope
The change decides only whether an archive is re-fetched and re-extracted. It touches no model construction, no tensor operation, no decorator, and no shape or dtype.