Fix daily layer-package split jobs failing on first artifact - #945
Merged
Merged
Conversation
write_package_artifact ran the upload hook before reading the artifact back for tensor count, byte size, and sha256. The production upload hook in split-model-job.sh uploads each artifact and then unlinks it locally to stay under the HF Jobs ephemeral storage limit, so the reopen failed on the very first artifact (shared/metadata.gguf) and every daily split job errored at ~2 minutes. Read all artifact metadata from disk before invoking the hook, matching the ordering copy_projector_artifact already used. Add a regression test that runs a hook which deletes the uploaded file and still succeeds.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change reorders ChangesArtifact packaging hook fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7 tasks
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
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.
What this fixes
The daily automated layer-package split jobs (Queue Unsloth layer packages, cron
23 17 * * *) have been failing every day for ~10 days. Every run — across different models — died after ~2 minutes on the very first artifact:After this change, split jobs run to completion again: no code or infra change is needed on the runner side, because each HF Job builds
skippy-model-packagefrommainat job start, so the next scheduled run picks up the fix automatically.Root cause
write_package_artifactinvoked the--after-artifact-commandupload hook before reopening the artifact to record its tensor count, byte size, and sha256. The production upload hook insplit-model-job.shuploads each artifact and thenunlinks it locally (by design, to stay under the HF Jobs ephemeral storage limit). So the reopen hit a file that had already been deleted, failing on the first artifact (shared/metadata.gguf).The neighboring
copy_projector_artifactalready did this correctly — read metadata first, run the hook last. This change makeswrite_package_artifactuse the same ordering.Validation
Reproduced and verified with the real
write-packagebinary against a real GGUF (unsloth/Qwen3.5-0.8B-GGUF:Q4_K_M), using a hook that mimics production (record upload, thenrm -fthe file):1(exact production error)0Added a regression test (
artifact_hook_tolerates_a_hook_that_deletes_the_uploaded_file) that runs a deleting hook and asserts success.cargo test -p skippy-model-package— 24 passedcargo test -p model-package— 9 passedcargo clippy -p skippy-model-package --all-targets -- -D warnings— cleancargo fmt --all --check— cleanSummary by CodeRabbit
Bug Fixes
Tests