feat(skippy): expose a model-bound tokenizer facade - #1214
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the ChangesTokenizer capability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant TokenizerCapability
participant StageZeroRuntime
participant StageModel
Consumer->>TokenizerCapability: Submit tokenization batch
TokenizerCapability->>TokenizerCapability: Validate identity and limits
TokenizerCapability->>StageZeroRuntime: Check runtime activity
StageZeroRuntime->>StageModel: Request bounded tokenization
StageModel-->>TokenizerCapability: Return token IDs or bounded miss
TokenizerCapability-->>Consumer: Return indexed responses or typed errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
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-server/src/tokenizer.rs`:
- Around line 107-119: Synchronize tokenizer operations with shutdown: in
crates/skippy-server/src/tokenizer.rs lines 107-119, recheck tokenizer_active
while holding the runtime mutex before calling tokenize_bounded; apply the same
guarded recheck in lines 122-140 before detokenize_bytes. In
crates/skippy-server/src/embedded.rs line 286, after clearing tokenizer_active,
acquire and release the runtime mutex before shutdown returns. Add a concurrent
regression test covering a tokenizer request queued behind the runtime mutex
during shutdown.
🪄 Autofix
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: 2f4681c0-8c37-44d4-8435-6dda7a042396
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/docker-precheck.ymlCargo.tomlcrates/skippy-protocol/Cargo.tomlcrates/skippy-protocol/src/lib.rscrates/skippy-protocol/src/tokenizer.rscrates/skippy-runtime/src/native.rscrates/skippy-server/Cargo.tomlcrates/skippy-server/README.mdcrates/skippy-server/src/embedded.rscrates/skippy-server/src/tokenizer.rscrates/skippy-tokenizer/Cargo.tomlcrates/skippy-tokenizer/README.mdcrates/skippy-tokenizer/src/lib.rsscripts/affected-crates.shscripts/plan-clippy-batches.shscripts/publish-crates.sh
💤 Files with no reviewable changes (1)
- crates/skippy-protocol/src/tokenizer.rs
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-server/src/tokenizer.rs`:
- Around line 558-589: The test
`queued_tokenization_rechecks_lifecycle_after_runtime_lock` must
deterministically pause the worker after its initial lifecycle check and before
locking `runtime`. Add a test-only synchronization signal at that point, have
the test wait for the signal before setting `active` to false, then release the
runtime lock and retain the assertion that tokenization returns
`RuntimeUnavailable`.
🪄 Autofix
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: f67b9af8-7e83-412a-ac89-be5501892b5c
📒 Files selected for processing (2)
crates/skippy-server/src/embedded.rscrates/skippy-server/src/tokenizer.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/skippy-server/src/embedded.rs
Closes #1213
What this enables
Skippy consumers can use a reusable, in-process tokenizer capability bound to the already-loaded stage-zero model.
skippy-tokenizercontract crate.Vec<i32>token IDs, optional token pieces, bounded batches, deterministic request indexes, and typed per-item errors./v1/tokenizeroute as a compatibility adapter, accepting bothspecial_tokensand legacyadd_specialrequests.skippy_protocol::tokenizernamespace through a compatibility re-export.Rust API example
In-process consumers use the tokenizer capability already bound to the loaded Skippy runtime:
Batch consumers receive deterministic request indexes and per-item results:
The capability uses the already-loaded stage-zero model; it does not perform an HTTP round trip or load a second model. The
/v1/tokenizeroute remains available as a compatibility adapter.Compatibility
No Skippy ABI or llama.cpp patch changes are included. This does not need to stack on #1194.
Legacy identities without the newly optional tokenizer-version or serving-profile fields continue to match the bound runtime identity.
Validation
cargo test -p skippy-tokenizer --libcargo test -p skippy-server --lib(387 passed)cargo test -p skippy-protocol --lib(45 passed)cargo check -p mesh-llmcargo clippy -p skippy-tokenizer --all-targets -- -D warningscargo clippy -p skippy-server --all-targets -- -D warningscargo run -p xtask -- repo-consistency ci-crate-listscargo run -p xtask -- repo-consistency publish-cratesjust buildThe repository-wide
cargo clippy -p mesh-llm --all-targets -- -D warningsremains blocked by 28 pre-existingunfulfilled_lint_expectationswarnings inmesh-llm-host-runtime, unrelated to this change.Summary by CodeRabbit
New Features
Documentation