Add direct quantized layer package workflow - #901
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR removes custom Changesskippy-quantize and skippy-model-package pipeline
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over CLI,preflight: quantize-layer-package flow
end
participant CLI as quantize-layer-package
participant quantize_layer_package
participant HookScript as quantize-package-artifact.sh
participant skippy_model_package as skippy-model-package write-package
participant preflight
CLI->>quantize_layer_package: QuantizeLayerPackageArgs (validate, lock manifest)
quantize_layer_package->>HookScript: write bash hook script to quant scratch dir
quantize_layer_package->>skippy_model_package: write-package --post-artifact-command hook
loop per artifact
skippy_model_package->>HookScript: invoke with ARTIFACT_PATH env
HookScript->>HookScript: skippy-quantize quantize (base quant + --tensor-type-file)
HookScript-->>skippy_model_package: move quantized result to artifact location
end
skippy_model_package-->>quantize_layer_package: package written
quantize_layer_package->>preflight: --verify-sha256 [--stages]
preflight-->>quantize_layer_package: verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-quantize/src/main.rs`:
- Around line 679-717: The quantize_layer_package function removes the
manifest.target directory without validating that it is distinct from the source
and package-dir paths, which could cause accidental deletion of source data or
the newly-written package. Add ensure! checks early in the function (alongside
the existing validation checks) to verify that manifest.target does not equal or
overlap with args.package_dir and the source path from args.init, then let the
cleanup proceed only if these paths are confirmed to be disjoint. Use
descriptive error messages that reference the specific paths to help users
understand path conflicts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32cee611-9569-4801-bbe2-c2cc361b97c4
📒 Files selected for processing (11)
crates/skippy-model-package/src/main.rscrates/skippy-quantize/README.mdcrates/skippy-quantize/src/direct_quantize.rscrates/skippy-quantize/src/gguf_template.rscrates/skippy-quantize/src/gguf_writer.rscrates/skippy-quantize/src/gguf_writer_tests.rscrates/skippy-quantize/src/main.rscrates/skippy-quantize/src/native_quantize.rscrates/skippy-quantize/src/type_catalog.rscrates/skippy-quantize/src/types.rscrates/skippy-quantize/tests/direct_quantize_cli.rs
Comment on lines
+679
to
+717
| fn quantize_layer_package(args: QuantizeLayerPackageArgs) -> Result<()> { | ||
| ensure!( | ||
| args.init.window_size == 1, | ||
| "quantize-layer-package currently requires --window-size 1" | ||
| ); | ||
| ensure!( | ||
| !args.runner.dry_run, | ||
| "quantize-layer-package does not support --dry-run; use quant-job --preflight-only first" | ||
| ); | ||
| ensure!( | ||
| !args.runner.print_only, | ||
| "quantize-layer-package does not support --print-only; use quant-job --preflight-only first" | ||
| ); | ||
| ensure!( | ||
| args.skippy_model_package_bin.is_file(), | ||
| "missing skippy-model-package binary {}; build it with `cargo build --release --locked -p skippy-model-package` or pass --skippy-model-package-bin", | ||
| args.skippy_model_package_bin.display() | ||
| ); | ||
| if args.package_dir.exists() { | ||
| ensure!( | ||
| args.replace_package, | ||
| "package dir already exists: {}; pass --replace-package to overwrite it", | ||
| args.package_dir.display() | ||
| ); | ||
| fs::remove_dir_all(&args.package_dir) | ||
| .with_context(|| format!("remove package dir {}", args.package_dir.display()))?; | ||
| } | ||
|
|
||
| let manifest = quant_manifest_from_args(&args.init)?; | ||
| let manifest_path = args.init.manifest.clone(); | ||
| let runner = prepare_quant_runner(args.runner.clone())?; | ||
| with_manifest_lock(&manifest_path, || { | ||
| ensure_manifest(&manifest_path, &manifest)?; | ||
| let hook = write_layer_package_quant_hook(&args, &runner)?; | ||
| write_and_preflight_layer_package(&args, &manifest, &hook)?; | ||
| if !args.keep_quant { | ||
| remove_dir_if_exists(&manifest.target)?; | ||
| print_path_event("🧹", "Cleaned quant scratch", &manifest.target); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard quant scratch cleanup from deleting source/package paths.
Line 714 removes manifest.target unconditionally (unless --keep-quant), but there is no guard that --target is disjoint from --source and --package-dir. A path overlap can wipe BF16 source data or the just-written package.
🛠️ Suggested fix
fn quantize_layer_package(args: QuantizeLayerPackageArgs) -> Result<()> {
+ ensure!(
+ args.init.target != args.init.source && !args.init.source.starts_with(&args.init.target),
+ "--target cannot equal or contain --source; quant scratch cleanup removes --target"
+ );
+ ensure!(
+ args.init.target != args.package_dir && !args.package_dir.starts_with(&args.init.target),
+ "--target cannot equal or contain --package-dir; quant scratch cleanup removes --target"
+ );
ensure!(
args.init.window_size == 1,
"quantize-layer-package currently requires --window-size 1"
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/skippy-quantize/src/main.rs` around lines 679 - 717, The
quantize_layer_package function removes the manifest.target directory without
validating that it is distinct from the source and package-dir paths, which
could cause accidental deletion of source data or the newly-written package. Add
ensure! checks early in the function (alongside the existing validation checks)
to verify that manifest.target does not equal or overlap with args.package_dir
and the source path from args.init, then let the cleanup proceed only if these
paths are confirmed to be disjoint. Use descriptive error messages that
reference the specific paths to help users understand path conflicts.
This was referenced Jul 14, 2026
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
1 participant
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.
Summary
Validation
Lab proof
Summary by CodeRabbit
Release Notes
New Features
quantize-layer-packageCLI subcommand for generating and quantizing layer packages from BF16 source files.Documentation
--target-prefix/--output-basename, not in the--quantflag.Bug Fixes
--tensor-type-file.