Repository navigation
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
the
dependencies
1aaf596 to
e23e6ec
Compare
6643d00 to
c98e733
Compare
rainj-me
changed the title
alexnails
left a comment
alexnails
left a comment
There was a problem hiding this comment.
This is most speculative review, will review again once egress and ingress PRs are in
| for &id in token_ids { | ||
| if let Some(chunk) = self | ||
| .stream | ||
| .step(id as u32) |
There was a problem hiding this comment.
add vocab_size to the shard?
| if stop.is_empty() { | ||
| return; | ||
| } | ||
| if let Some(pos) = text.find(stop) { |
There was a problem hiding this comment.
given my other comment from earlier PR about FinalMetadata for multiplexing we should also have this adopt maybe a better form of code that is more end aware
| /// Egress half of the lifecycle FSM. Lives here because the ingress | ||
| /// `Request` (and its FSM) was handed to the scheduler when queued; the | ||
| /// shard is the sole owner of the request's egress state, so no lock. | ||
| fsm: RequestState, |
There was a problem hiding this comment.
we can discard Result in quite a few places
|
|
||
| impl Runnable for DetokenizerWorker { | ||
| fn run(self) { | ||
| let mut table: HashMap<RidHash, DetokState> = HashMap::new(); |
There was a problem hiding this comment.
Part 3 of hashing comment lol
[Agent Verification]
The lookup on line 237 runs once per ChunkEvent, so at batch 4096 over 8 shards that's ~512 lookups per shard per decode step. Measured (--release, 512 keys, 2000 iters):
| per decode step | |
|---|---|
HashMap default (SipHash) — as written |
9.8 µs |
HashMap with a pass-through hasher |
2.1 µs |
4.7×, for a one-line type change:
type RidMap<V> = HashMap<RidHash, V, BuildHasherDefault<PassThroughHasher>>;RidHash::from_rid already ran SipHash-1-3 over the rid string, so the key is well distributed by construction — the second hash adds no avalanche, only latency. A pass-through Hasher whose write_u64 stores the value is enough. (RidHash derives Hash on a u64 field, so it goes through write_u64 and there's no multi-field path to worry about.)
Two caveats worth stating rather than glossing:
- HashDoS: pass-through hashing normally reopens that, but here the map key is already a keyed-hash digest, and sglang rust server tm egress #32249's
ids.rsfixes the SipHash keys anyway — so the collision resistance of this map is already whateverRidHashprovides, and re-hashing doesn't add any. Worth a comment saying so at the type alias. - Absolute size: ~8 µs per shard per step against a decode step of tens of ms is well under 1%. Same framing as my notes on the other two PRs — it's worth taking because it's free and this is the per-chunk path, not because it's a bottleneck today.
Related, while in run: this worker has no shutdown receiver and relies on rx closing, which per the comment requires the API runtime and tm-ingress and tm-egress to all release their Senders clones. But this worker holds tm: Sender<TmEvent> — a sender into the ingress inbox — and Ingress holds Senders (including detok), so the two keep each other's inboxes alive and neither can close by producer-drop. The chain only unwinds from Ingress's explicit shutdown signal outward. That works, but it's a four-party ordering whose only specification is this comment, spread across three PRs, and a single retained Senders clone anywhere hangs both pools forever with no timeout and no log. Worth either giving the pools the same shutdown receiver the TM stages have, or stating the invariant in one place.
|
|
||
| /// Resolve a model file from the tokenizer source: a dir → `dir/<file>`, a file → | ||
| /// its sibling, else an HF Hub repo id → the local cache. `None` if not found. | ||
| pub fn resolve_model_file(path: &str, revision: Option<&str>, filename: &str) -> Option<String> { |
| // The tokenizer pool only ever receives generate requests. Encode, | ||
| // then advance the FSM: `TokenizeDone` on success. | ||
| let event = { | ||
| let RequestKind::Generate(g) = &mut req.kind else { |
There was a problem hiding this comment.
copy Ingress::fail / ingress.rs behavior here?
8a3a9e8 to
f8a1aa9
Compare
c98e733 to
48413a4
Compare
2 participants
Motivation
Split PR from #29799
Depends on #32358
Modifications
Accuracy Tests
Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ✅ Run #30491901081
Latest PR Test (Extra): ❌ Run #30491900554