fix: updates versions and adds ahashmap to BPE #2072
Conversation
WalkthroughThe changes update dependency specifications in Changes
Estimated code review effort1 (~5 minutes) Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/llm/Cargo.toml (1)
104-108:tokenizersversion does not match the PR objective / likely still incompatibleThe PR description says we migrate to
tokenizers 0.22.2, yet the manifest only bumps to0.21.2.
API changes (e.g. removal ofget_normalizers) landed in 0.22, so this line probably leaves the original compile error unresolved.-tokenizers = { version = "0.21.2", default-features = false, features = [ +tokenizers = { version = "0.22.2", default-features = false, features = [Please adjust the version (and run
cargo update -p tokenizers) or amend the PR notes.
🧹 Nitpick comments (3)
lib/llm/Cargo.toml (1)
113-115: Minor: align sub-crates to a caret requirementUnless you need an exact patch, consider a caret requirement to allow bug-fixes automatically:
-toktrie = { version = "1.1.0" } -toktrie_hf_tokenizers = { version = "1.1.0" } +toktrie = "1.1" +toktrie_hf_tokenizers = "1.1"Nit, feel free to ignore if pinning is intentional.
lib/llm/src/gguf/gguf_tokenizer.rs (2)
238-244: Pre-reserve capacity forvocabto avoid re-hashingWe already know the final size (
p.tokens.len()), so we can avoid several reallocations:-let mut vocab: AHashMap<String, u32> = AHashMap::new(); +let mut vocab: AHashMap<String, u32> = AHashMap::with_capacity(p.tokens.len());Tiny but basically free.
269-272: Same reservation nit forspecial_toks-let mut special_toks: AHashMap<String, processors::template::SpecialToken> = - AHashMap::new(); +let mut special_toks: AHashMap<String, processors::template::SpecialToken> = + AHashMap::with_capacity(1);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
lib/engines/mistralrs/Cargo.toml(1 hunks)lib/llm/Cargo.toml(3 hunks)lib/llm/src/gguf/gguf_tokenizer.rs(3 hunks)
🧠 Learnings (1)
lib/engines/mistralrs/Cargo.toml (3)
Learnt from: kthui
PR: #1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
🧰 Additional context used
🧠 Learnings (1)
lib/engines/mistralrs/Cargo.toml (3)
Learnt from: kthui
PR: #1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (3)
lib/engines/mistralrs/Cargo.toml (1)
42-42: Pinning bothversionandrevcan lead to subtle mismatchesWhen a git source is specified Cargo ignores the
versionfield for resolution, but still checks that the crate inside the repo advertises at least that version.
If the commit8a4faf3no longer reportsversion = "0.6.0"in its ownCargo.toml, the build will fail with “package ID specification… does not match”.Verify the commit’s internal version or drop the
versionkey entirely:-mistralrs = { git = "https://github.com/EricLBuehler/mistral.rs.git", version = "0.6.0", rev = "8a4faf3" } +mistralrs = { git = "https://github.com/EricLBuehler/mistral.rs.git", rev = "8a4faf3" }(or update the version string if the crate evolved).
lib/llm/Cargo.toml (1)
130-130: Check for duplicateahashin the workspace
ahashis already a transitive dependency viatoktrie; adding an explicit top-level entry is fine, but you might want to enable the sameserde/stdfeature set everywhere to avoid duplicate crate instances.lib/llm/src/gguf/gguf_tokenizer.rs (1)
30-30: Explicit import ofAHashMaplooks goodThe replacement is clean and keeps the diff minimal.
Overview:
Fixes compile issue.
mistralrsv0.6.0 but rev 8a4faf3tokenizersfrom 0.21.1 to 0.21.2toktrieandtoktrie_hf_tokenizersfrom 0.6.28 to 1.1ahashtolib/llm/Cargo.tomlas we need to useahash::AHashMapinstead ofstd::collections::HashMapingguf_tokenizer.rsto satisfy new trait bounds inbpe_tokenizer(..)ureqas a feature forhuggingface-hubsince tests ingguf_tokenizer.rsuse itDetails:
Where should the reviewer start?
./lib/engines/mistralrs/Cargo.toml/lib/llm/Cargo.tomllib/llm/src/gguf/gguf_tokenizer.rsRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to:
get_normalizersEricLBuehler/mistral.rs#1523Summary by CodeRabbit