Add package writer progress output - #863
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new Changesskippy-model-package Progress Reporting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 |
ndizazzo
approved these changes
Jun 17, 2026
ndizazzo
left a comment
ndizazzo
left a comment
Collaborator
There was a problem hiding this comment.
Seems fine, but you might consider inline progress bars from Ratatui, since they're already in the project instead of rolling a bespoke one
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/skippy-model-package/src/main.rs (1)
481-659: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
write_packageprogress wiring out of this oversized module.This file now extends past 2,000 lines (Line 2130), and the new progress-specific logic/helper additions further increase it. Please move the
write_packageprogress orchestration (or at least the progress detail helpers and step-driving code) into a dedicated module to stay within the repository’s Rust file-size constraint.As per coding guidelines, "Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."
🤖 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-model-package/src/main.rs` around lines 481 - 659, The main.rs file exceeds the 2,000 line limit and needs to be refactored by extracting progress-related code into a dedicated module. Move the write_package function along with its helper functions artifact_progress_detail and projector_progress_detail into a new module file (e.g., package_progress.rs or package_writer.rs). Update the main.rs file to import and re-export these functions from the new module, ensuring all necessary dependencies and type imports are included in the new module. This will reduce the main.rs file size and separate concerns by consolidating progress orchestration logic into its own responsibility-focused module.Source: Coding guidelines
crates/skippy-server/src/frontend.rs (1)
172-195: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftKeep new batcher wiring out of the over-limit frontend module.
frontend.rsis already over the 2,000-line limit. Please move the batcher construction/field assembly behind a focused backend-construction helper or module, leaving this file as routing/orchestration only. As per coding guidelines, “Do not add Rust source files over 2,000 lines.”Also applies to: 522-544, 582-583
🤖 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-server/src/frontend.rs` around lines 172 - 195, The frontend.rs file exceeds the 2,000-line limit per coding guidelines, and the batcher construction and backend initialization logic (DecodeBatcher, DecodeFrameBatcher instantiation, and StageOpenAiBackend struct initialization) is adding to this violation. Extract the backend construction logic including the creation of decode_batcher, decode_frame_batcher, and the entire StageOpenAiBackend struct assembly into a separate focused helper function or dedicated module. This will keep frontend.rs as a routing/orchestration-only file and reduce its overall line count to comply with the guidelines. Apply the same refactoring principle to any other similar constructions mentioned at lines 522-544 and 582-583.Source: Coding guidelines
🧹 Nitpick comments (3)
crates/skippy-server/src/runtime_state.rs (1)
94-105: 💤 Low valueConsider adding
#[derive(Debug)]for diagnostic purposes.Other public structs in this file (e.g.,
RuntimeSessionLaneStats,RuntimeSessionDropStats) deriveDebug. Adding it here would aid troubleshooting batch decode issues without any runtime cost.♻️ Suggested change
+#[derive(Debug)] pub struct RuntimeDecodeBatchRequest<'a> { pub session_id: &'a str, pub token_id: i32, pub sampling: Option<&'a SamplingConfig>, } +#[derive(Debug)] pub struct RuntimeDecodeFrameBatchRequest<'a> {🤖 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-server/src/runtime_state.rs` around lines 94 - 105, The structs RuntimeDecodeBatchRequest and RuntimeDecodeFrameBatchRequest do not derive Debug, which is inconsistent with other public structs in the file like RuntimeSessionLaneStats and RuntimeSessionDropStats. Add the #[derive(Debug)] attribute to both struct definitions to enable diagnostic logging and debugging without any performance cost.crates/model-package/src/bin/queue-unsloth-layer-packages.rs (1)
444-446: 💤 Low valueConsider documenting
--forcebehavior more explicitly in help text.The
--forceflag bypasses all queue status checks (published, cataloged, queued), allowing re-queue of models regardless of their current state. A brief description would help users understand the implications.📝 Suggested help text expansion
--job-poll-seconds N\n\ --split-candidate-vram-gib GiB\n\ - --force\n\ + --force (skip queue status checks; re-queue regardless of existing state)\n\ --no-catalog-direct"Also applies to: 456-456
🤖 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/model-package/src/bin/queue-unsloth-layer-packages.rs` around lines 444 - 446, The help text for the `--force` flag does not adequately explain its behavior to users. Locate the help text definition for the `--force` flag in the file (referenced at lines 444-446 and 456) and add or expand the description to explicitly state that this flag bypasses all queue status checks (published, cataloged, queued state) and allows models to be re-queued regardless of their current state. This will help users understand the implications of using the flag before they invoke it.scripts/hf-jobs/checkpoint-quant-package-job.sh (1)
259-284: 💤 Low valueFragile string-based patching of llama.cpp conversion scripts.
The exact-string matching for patching
convert_hf_to_gguf.pyandutility.pywill break if llama.cpp reformats or modifies these lines. The error handling is good (exits if pattern not found), but this could cause unexpected job failures after llama.cpp updates.Consider documenting this fragility or adding a fallback mechanism. Alternatively, upstream the revision-pinning capability to llama.cpp's converter via a
--revisionargument.🤖 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 `@scripts/hf-jobs/checkpoint-quant-package-job.sh` around lines 259 - 284, The string-based patching logic for modifying convert_hf_to_gguf.py (replacing the old_snapshot pattern) and utility.py (replacing "/resolve/main/") is fragile and will break if llama.cpp makes formatting changes. Add documentation in a comment block above the patching section explaining the brittle nature of these exact-string matches and what could cause failures. Additionally, enhance the error handling by capturing the state before patching and adding validation after each file modification to confirm the expected changes were applied successfully, rather than relying solely on initial pattern detection.
🤖 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-runtime/src/lib.rs`:
- Around line 3028-3142: The function decode_step_frame_batch_sampled_raw
contains multiple distinct responsibilities including pointer preparation,
buffer allocation, FFI retry handling, token-count updates, and output assembly.
Extract these phases into separate helper functions such as one for preparing
input pointers and buffers, another for handling the FFI call with retry logic,
and another for assembling the output. Replace the verbose inline code in
decode_step_frame_batch_sampled_raw with calls to these focused helpers to
reduce the function length and improve maintainability while staying within code
style guidelines.
In `@crates/skippy-server/src/binary_transport.rs`:
- Around line 884-898: The batched decode candidate check and execution logic in
handle_binary_connection is contributing to binary_transport.rs exceeding 3,000
lines. Extract the is_decode_frame_batch_candidate predicate and the entire
batched decode execution block (including the decode_batcher.decode call and the
assignment of runtime_lock_wait_ms, runtime_lock_hold_ms, runtime_lock_acquires,
decode_batch_size, and decode_batch_wait_ms) into a focused helper method in the
decode_batcher module or a new binary-decode helper module. Replace the inline
conditional block with a single call to this extracted method, allowing
handle_binary_connection to only orchestrate the batched decode path rather than
containing the full implementation details.
In `@crates/skippy-server/src/binary_transport/decode_batcher.rs`:
- Around line 130-145: The wait_for_batch function currently drains frames by
count without validating session uniqueness, which can result in multiple frames
for the same session_id being in a single batch and causing validation failures.
Modify the draining logic in wait_for_batch to track which session_ids have
already been included in the current batch using a set, then iterate through the
pending frames and only add frames whose session_id hasn't been seen yet,
stopping when the batch reaches max_batch_size or when no more unique sessions
are available, leaving frames with duplicate session_ids in the queue for the
next batch.
- Around line 191-201: In the batch processing block where
batch.into_iter().zip(outputs) is called, add validation to ensure the number of
outputs matches the number of pending requests before zipping them together.
Check that outputs.len() equals batch.len(), and if they don't match, send an
error response to all pending items in the batch rather than proceeding with the
zip operation, which would silently drop mismatched items and cause callers to
receive misleading "batcher stopped" errors instead of the actual integration
failure.
In `@crates/skippy-server/src/frontend/decode_batcher.rs`:
- Around line 184-193: The `zip(predicted)` operation in the batch processing
loop silently ignores count mismatches between batch and predicted results,
potentially leaving callers without responses. Before the for loop that iterates
through batch.into_iter().zip(predicted), add an explicit validation check to
ensure predicted.len() equals batch.len(). If the counts don't match, send error
responses to all pending callers via pending.reply.send with an appropriate
error indicating the mismatch, rather than allowing the zip operation to
silently truncate or drop results.
In `@crates/skippy-server/src/frontend/local_generation.rs`:
- Around line 415-445: Extract the batched decode logic from the
generate_local_tokens method into a separate helper function. Create a new
private helper method that encapsulates the decode step starting from the
decode_batcher.decode() call through the signal and runtime session statistics
gathering. This helper should accept the session_id, current token, and request
sampling parameters, then return a struct containing the predicted token, all
collected metrics (batch_size, batch_wait_ms, runtime_lock timings, decode_ms),
and the signal information. Replace the inline decode block in
generate_local_tokens with a call to this new helper method to reduce the
complexity and line count of the main generation loop.
In `@third_party/llama.cpp/patches/0083-Add-skippy-batched-decode-ABI.patch`:
- Around line 192-208: The validation loop that processes each session to build
the batch does not check for duplicate sessions. Before or at the beginning of
the loop that starts with `for (int32_t i = 0; i < n_tokens; ++i)`, add a
validation check to detect if the same skippy_session pointer appears more than
once in the sessions array. If duplicates are found, call llama_batch_free on
the batch, use skippy_set_error to report the error with an appropriate status
code, and return early to prevent corrupting session state by processing the
same session multiple times in a single decode operation.
In
`@third_party/llama.cpp/patches/0084-Add-skippy-batched-activation-decode-ABI.patch`:
- Around line 104-137: The batched activation decode loop starting with the
iteration over n_tokens does not check for duplicate sessions in the sessions
array. Add validation to reject cases where the same session object appears
multiple times in the batch, which would cause it to be processed twice at the
same position. Implement this by comparing each session pointer against all
previously processed sessions in the loop and call skippy_set_error with
SKIPPY_STATUS_INVALID_ARGUMENT and an appropriate message if a duplicate is
found, then return the error status.
- Around line 141-154: Add a guard condition in the activation embedding block
to check if n_embd_inp is less than n_embd and return SKIPPY_STATUS_UNSUPPORTED
before attempting any memory operations. This check should be placed after the
existing SKIPPY_ACTIVATION_FLAG_GEMMA3N_ALTUP flag check and before the
conditional logic that handles the memcpy and memset operations, since the
current else clause assumes n_embd_inp >= n_embd and will cause buffer overflow
on the memcpy call and integer underflow on the memset size calculation when
this assumption is violated.
---
Outside diff comments:
In `@crates/skippy-model-package/src/main.rs`:
- Around line 481-659: The main.rs file exceeds the 2,000 line limit and needs
to be refactored by extracting progress-related code into a dedicated module.
Move the write_package function along with its helper functions
artifact_progress_detail and projector_progress_detail into a new module file
(e.g., package_progress.rs or package_writer.rs). Update the main.rs file to
import and re-export these functions from the new module, ensuring all necessary
dependencies and type imports are included in the new module. This will reduce
the main.rs file size and separate concerns by consolidating progress
orchestration logic into its own responsibility-focused module.
In `@crates/skippy-server/src/frontend.rs`:
- Around line 172-195: The frontend.rs file exceeds the 2,000-line limit per
coding guidelines, and the batcher construction and backend initialization logic
(DecodeBatcher, DecodeFrameBatcher instantiation, and StageOpenAiBackend struct
initialization) is adding to this violation. Extract the backend construction
logic including the creation of decode_batcher, decode_frame_batcher, and the
entire StageOpenAiBackend struct assembly into a separate focused helper
function or dedicated module. This will keep frontend.rs as a
routing/orchestration-only file and reduce its overall line count to comply with
the guidelines. Apply the same refactoring principle to any other similar
constructions mentioned at lines 522-544 and 582-583.
---
Nitpick comments:
In `@crates/model-package/src/bin/queue-unsloth-layer-packages.rs`:
- Around line 444-446: The help text for the `--force` flag does not adequately
explain its behavior to users. Locate the help text definition for the `--force`
flag in the file (referenced at lines 444-446 and 456) and add or expand the
description to explicitly state that this flag bypasses all queue status checks
(published, cataloged, queued state) and allows models to be re-queued
regardless of their current state. This will help users understand the
implications of using the flag before they invoke it.
In `@crates/skippy-server/src/runtime_state.rs`:
- Around line 94-105: The structs RuntimeDecodeBatchRequest and
RuntimeDecodeFrameBatchRequest do not derive Debug, which is inconsistent with
other public structs in the file like RuntimeSessionLaneStats and
RuntimeSessionDropStats. Add the #[derive(Debug)] attribute to both struct
definitions to enable diagnostic logging and debugging without any performance
cost.
In `@scripts/hf-jobs/checkpoint-quant-package-job.sh`:
- Around line 259-284: The string-based patching logic for modifying
convert_hf_to_gguf.py (replacing the old_snapshot pattern) and utility.py
(replacing "/resolve/main/") is fragile and will break if llama.cpp makes
formatting changes. Add documentation in a comment block above the patching
section explaining the brittle nature of these exact-string matches and what
could cause failures. Additionally, enhance the error handling by capturing the
state before patching and adding validation after each file modification to
confirm the expected changes were applied successfully, rather than relying
solely on initial pattern detection.
🪄 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: 59167f13-7030-431c-9895-d862521b0f26
📒 Files selected for processing (18)
crates/model-package/src/bin/queue-unsloth-layer-packages.rscrates/skippy-ffi/src/lib.rscrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/progress.rscrates/skippy-runtime/src/lib.rscrates/skippy-server/src/binary_transport.rscrates/skippy-server/src/binary_transport/decode_batcher.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/decode_batcher.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/tests.rscrates/skippy-server/src/runtime_state.rsscripts/hf-jobs/checkpoint-quant-package-job.shscripts/hf-jobs/launch-jianyang-phase1.shthird_party/llama.cpp/patches/0083-Add-skippy-batched-decode-ABI.patchthird_party/llama.cpp/patches/0084-Add-skippy-batched-activation-decode-ABI.patch
Comment on lines
+884
to
+898
| let result = if is_decode_frame_batch_candidate(&message, executable_token_ids) { | ||
| let token_id = executable_token_ids | ||
| .first() | ||
| .copied() | ||
| .unwrap_or(message.state.current_token); | ||
| let sampling = runtime_sampling_config(message.sampling.as_ref()); | ||
| let outcome = decode_frame_batcher | ||
| .decode(&session_key, token_id, sampling.as_ref(), input) | ||
| .context("execute batched binary decode frame")?; | ||
| runtime_lock_wait_ms = outcome.runtime_lock_wait_ms; | ||
| runtime_lock_hold_ms = outcome.runtime_lock_hold_ms; | ||
| runtime_lock_acquires = 1; | ||
| decode_batch_size = outcome.batch_size; | ||
| decode_batch_wait_ms = outcome.batch_wait_ms; | ||
| (outcome.predicted, Vec::new(), outcome.output) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move the batch-candidate and execution glue out of this oversized module.
This file is already over 3,000 lines, and the new decode batching branch/helper adds more logic to handle_binary_connection. Please move the candidate predicate plus batched decode execution into decode_batcher or a focused binary-decode helper so this file only orchestrates the call. As per coding guidelines, “Do not add Rust source files over 2,000 lines” and “Do not add Rust methods or functions over the configured Clippy line-count limit.”
Also applies to: 3305-3315
🤖 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-server/src/binary_transport.rs` around lines 884 - 898, The
batched decode candidate check and execution logic in handle_binary_connection
is contributing to binary_transport.rs exceeding 3,000 lines. Extract the
is_decode_frame_batch_candidate predicate and the entire batched decode
execution block (including the decode_batcher.decode call and the assignment of
runtime_lock_wait_ms, runtime_lock_hold_ms, runtime_lock_acquires,
decode_batch_size, and decode_batch_wait_ms) into a focused helper method in the
decode_batcher module or a new binary-decode helper module. Replace the inline
conditional block with a single call to this extracted method, allowing
handle_binary_connection to only orchestrate the batched decode path rather than
containing the full implementation details.
Source: Coding guidelines
Comment on lines
+130
to
+145
| fn wait_for_batch(&self) -> Option<Vec<PendingDecodeFrame>> { | ||
| let mut state = self | ||
| .state | ||
| .lock() | ||
| .expect("decode frame batcher lock poisoned"); | ||
| while state.pending.is_empty() && !state.stopping { | ||
| state = self | ||
| .ready | ||
| .wait(state) | ||
| .expect("decode frame batcher lock poisoned"); | ||
| } | ||
| if state.pending.is_empty() && state.stopping { | ||
| return None; | ||
| } | ||
| let batch_size = self.max_batch_size.min(state.pending.len()); | ||
| Some(state.pending.drain(..batch_size).collect()) |
Contributor
There was a problem hiding this comment.
Keep duplicate sessions out of a single runtime batch.
RuntimeState batch decode validates unique sessions; draining by count can batch two frames for the same wire session_id, causing the whole batch—including unrelated requests—to fail. Leave later requests for an already-selected session in the queue for the next batch.
Suggested direction
use std::{
- collections::VecDeque,
+ collections::{HashSet, VecDeque},
sync::{- let batch_size = self.max_batch_size.min(state.pending.len());
- Some(state.pending.drain(..batch_size).collect())
+ let mut seen_sessions = HashSet::new();
+ let mut batch = Vec::with_capacity(self.max_batch_size.min(state.pending.len()));
+ let mut deferred = VecDeque::new();
+
+ while let Some(pending) = state.pending.pop_front() {
+ if batch.len() < self.max_batch_size && seen_sessions.insert(pending.session_id.clone())
+ {
+ batch.push(pending);
+ if batch.len() == self.max_batch_size {
+ break;
+ }
+ } else {
+ deferred.push_back(pending);
+ }
+ }
+
+ deferred.append(&mut state.pending);
+ state.pending = deferred;
+ Some(batch)🤖 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-server/src/binary_transport/decode_batcher.rs` around lines 130
- 145, The wait_for_batch function currently drains frames by count without
validating session uniqueness, which can result in multiple frames for the same
session_id being in a single batch and causing validation failures. Modify the
draining logic in wait_for_batch to track which session_ids have already been
included in the current batch using a set, then iterate through the pending
frames and only add frames whose session_id hasn't been seen yet, stopping when
the batch reaches max_batch_size or when no more unique sessions are available,
leaving frames with duplicate session_ids in the queue for the next batch.
Comment on lines
+192
to
+208
| + for (int32_t i = 0; i < n_tokens; ++i) { | ||
| + skippy_session * session = sessions[i]; | ||
| + if (session == nullptr || session->ctx != first->ctx || session->stage_model != first->stage_model) { | ||
| + llama_batch_free(batch); | ||
| + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "all sessions must belong to the same stage model"); | ||
| + return SKIPPY_STATUS_INVALID_ARGUMENT; | ||
| + } | ||
| + if (skippy_is_filtered(session) && session->stage_model->config.layer_start > 0) { | ||
| + llama_batch_free(batch); | ||
| + skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched token decode requires the first runtime slice or a full model"); | ||
| + return SKIPPY_STATUS_UNSUPPORTED; | ||
| + } | ||
| + batch.token[i] = token_ids[i]; | ||
| + batch.pos[i] = session->n_past; | ||
| + batch.n_seq_id[i] = 1; | ||
| + batch.seq_id[i][0] = session->seq_id; | ||
| + batch.logits[i] = 1; |
Contributor
There was a problem hiding this comment.
Reject duplicate sessions before building the native batch.
A caller can pass the same skippy_session* more than once. That batches duplicate seq_id/n_past entries and then advances the same session multiple times after one llama_decode, corrupting the session/KV state.
Proposed guard
for (int32_t i = 0; i < n_tokens; ++i) {
skippy_session * session = sessions[i];
if (session == nullptr || session->ctx != first->ctx || session->stage_model != first->stage_model) {
llama_batch_free(batch);
skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "all sessions must belong to the same stage model");
return SKIPPY_STATUS_INVALID_ARGUMENT;
}
+ for (int32_t j = 0; j < i; ++j) {
+ if (sessions[j] == session) {
+ llama_batch_free(batch);
+ skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "batched decode sessions must be unique");
+ return SKIPPY_STATUS_INVALID_ARGUMENT;
+ }
+ }
if (skippy_is_filtered(session) && session->stage_model->config.layer_start > 0) {
llama_batch_free(batch);
skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched token decode requires the first runtime slice or a full model");
return SKIPPY_STATUS_UNSUPPORTED;
}🤖 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 `@third_party/llama.cpp/patches/0083-Add-skippy-batched-decode-ABI.patch`
around lines 192 - 208, The validation loop that processes each session to build
the batch does not check for duplicate sessions. Before or at the beginning of
the loop that starts with `for (int32_t i = 0; i < n_tokens; ++i)`, add a
validation check to detect if the same skippy_session pointer appears more than
once in the sessions array. If duplicates are found, call llama_batch_free on
the batch, use skippy_set_error to report the error with an appropriate status
code, and return early to prevent corrupting session state by processing the
same session multiple times in a single decode operation.
Comment on lines
+104
to
+137
| + const size_t hidden_bytes_per_request = skippy_activation_hidden_bytes(first, 1); | ||
| + for (int32_t i = 0; i < n_tokens; ++i) { | ||
| + skippy_session * session = sessions[i]; | ||
| + if (session == nullptr || session->ctx != first->ctx || session->stage_model != first->stage_model) { | ||
| + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "all sessions must belong to the same stage model"); | ||
| + return SKIPPY_STATUS_INVALID_ARGUMENT; | ||
| + } | ||
| + const skippy_activation_desc * input_desc = input_descs != nullptr ? input_descs[i] : nullptr; | ||
| + const void * input_payload = input_payloads != nullptr ? input_payloads[i] : nullptr; | ||
| + enum skippy_status status = skippy_validate_frame_input(session, input_desc, input_payload, 1, out_error); | ||
| + if (status != SKIPPY_STATUS_OK) { | ||
| + return status; | ||
| + } | ||
| + if (input_desc != nullptr && input_desc->flags != 0) { | ||
| + skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched activation decode does not support activation sidebands yet"); | ||
| + return SKIPPY_STATUS_UNSUPPORTED; | ||
| + } | ||
| + if (skippy_output_activation_flags(session, input_desc) != 0) { | ||
| + skippy_set_error(out_error, SKIPPY_STATUS_UNSUPPORTED, "batched activation decode does not support output activation sidebands yet"); | ||
| + return SKIPPY_STATUS_UNSUPPORTED; | ||
| + } | ||
| + status = skippy_prepare_output_activation_frame( | ||
| + session, | ||
| + 1, | ||
| + output_payloads != nullptr ? output_payloads[i] : nullptr, | ||
| + output_payload_capacities != nullptr ? output_payload_capacities[i] : 0, | ||
| + out_output_payload_bytes != nullptr ? &out_output_payload_bytes[i] : nullptr, | ||
| + output_descs != nullptr ? &output_descs[i] : nullptr, | ||
| + input_desc, | ||
| + out_error); | ||
| + if (status != SKIPPY_STATUS_OK) { | ||
| + return status; | ||
| + } | ||
| + } |
Contributor
There was a problem hiding this comment.
Reject duplicate sessions in frame batches too.
This entrypoint has the same duplicate-session hazard: the same session can be decoded twice at the same position in one llama_batch, then advanced twice after a single native decode.
Proposed guard
for (int32_t i = 0; i < n_tokens; ++i) {
skippy_session * session = sessions[i];
if (session == nullptr || session->ctx != first->ctx || session->stage_model != first->stage_model) {
skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "all sessions must belong to the same stage model");
return SKIPPY_STATUS_INVALID_ARGUMENT;
}
+ for (int32_t j = 0; j < i; ++j) {
+ if (sessions[j] == session) {
+ skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "batched frame decode sessions must be unique");
+ return SKIPPY_STATUS_INVALID_ARGUMENT;
+ }
+ }
const skippy_activation_desc * input_desc = input_descs != nullptr ? input_descs[i] : nullptr;
const void * input_payload = input_payloads != nullptr ? input_payloads[i] : nullptr;🤖 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
`@third_party/llama.cpp/patches/0084-Add-skippy-batched-activation-decode-ABI.patch`
around lines 104 - 137, The batched activation decode loop starting with the
iteration over n_tokens does not check for duplicate sessions in the sessions
array. Add validation to reject cases where the same session object appears
multiple times in the batch, which would cause it to be processed twice at the
same position. Implement this by comparing each session pointer against all
previously processed sessions in the loop and call skippy_set_error with
SKIPPY_STATUS_INVALID_ARGUMENT and an appropriate message if a duplicate is
found, then return the error status.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/skippy-model-package/src/main.rs (1)
505-659: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
write_packageprogress orchestration out ofmain.rsto meet the file-size constraint.This adds more logic to a file that is already over 2,000 lines. Please move the
write_packageprogress wiring (including related detail helpers) into an owning module so new behavior is added outside this oversized file.As per coding guidelines, "Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."
🤖 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-model-package/src/main.rs` around lines 505 - 659, The write_package function and its helper functions artifact_progress_detail and projector_progress_detail are contributing to main.rs exceeding the 2,000 line limit. Extract the entire progress orchestration logic (the block starting with let mut progress = PackageProgress::new through the final println and Ok(())) along with both helper functions into a new dedicated module file, then update main.rs to import and call this extracted function instead. Ensure the extracted function maintains the same signature and return type as before.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/skippy-model-package/src/main.rs`:
- Around line 505-659: The write_package function and its helper functions
artifact_progress_detail and projector_progress_detail are contributing to
main.rs exceeding the 2,000 line limit. Extract the entire progress
orchestration logic (the block starting with let mut progress =
PackageProgress::new through the final println and Ok(())) along with both
helper functions into a new dedicated module file, then update main.rs to import
and call this extracted function instead. Ensure the extracted function
maintains the same signature and return type as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a66f2767-5084-4843-b938-23f8f7dc9041
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/skippy-model-package/Cargo.tomlcrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/progress.rs
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
2 participants
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
skippy-model-package write-packagenow reports deterministic package-writing progress while it builds layer package repos. The progress advances at artifact boundaries: shared metadata, embeddings, output, each layer GGUF, projector artifacts, and the finalmodel-package.json.The interactive renderer uses Ratatui's inline
LineGaugeinstead of a bespoke progress bar, so this stays aligned with the terminal UI dependency already used by the project.Output Preview
Interactive terminals redraw one stderr line while work is active. These examples are simulated from the actual Ratatui
LineGaugerenderer configured by this PR:For the GLM-5.1 Q3_K_M-plus layer package, the expected step count is:
A representative full progression looks like:
Non-interactive logs emit one line per completed step instead of terminal redraws:
Progress output can be disabled with:
Validation
cargo check -p skippy-model-package cargo test -p skippy-model-package --bin skippy-model-package cargo clippy -p skippy-model-package --bin skippy-model-package -- -D warnings