feat(libsy): stage router with handoff notes, per-tier prompts, and LLM fallback - #170
feat(libsy): stage router with handoff notes, per-tier prompts, and LLM fallback#170sabhatinas wants to merge 1 commit into
Conversation
WalkthroughAdds a public ChangesStage Routing
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
| } | ||
|
|
||
| #[async_trait] | ||
| impl Algorithm<SharedState> for StageRouter { |
There was a problem hiding this comment.
we should use this directly in config.rs
e1154ce to
72d8fe6
Compare
| /// | ||
| /// Errors if either threshold in `config` is outside `[0.0, 1.0]` or a tier is | ||
| /// missing from `targets`. | ||
| pub fn stage_router( |
There was a problem hiding this comment.
Do we want a factory function like this or a algorithm::Stage wrapper around FallThrough?
LlmTaskClassifier uses a wrapper. I'm guessing that is easier to configure on the server size?
@nachiketb-nvidia what are your thoughts?
There was a problem hiding this comment.
I prefer not have function,
something like this is better
struct StageRouter {
inner: FallThrough<...>
}
impl Algorithm for StageRouter {
... # use inner directly here
}
Using a function means we move Fallthrough from an internal implementation detail to public API (since it is crossing crates right now)
There was a problem hiding this comment.
Got it, I am pushing this change
e577cfe to
219f2ca
Compare
| .await?; | ||
| } | ||
|
|
||
| // 5. Offer the outbound request to the processors, paired with the decision that |
There was a problem hiding this comment.
@messiaen for clarity, I added this to append system prompts for instance. operations that do not need to go through routing algorithm and depend on routing decision can be here?
| .collect::<Vec<_>>(); | ||
| // Task-based routing judges the newest user message alone. A configured | ||
| // window widens that to the surrounding conversation. | ||
| let mut messages = match self.recent_turn_window { |
There was a problem hiding this comment.
@ayushag-nv please review this new method for recent window trimming.
| &self.efficient_target | ||
| } | ||
| _ => &self.capable_target, | ||
| // Judge output is untrusted, so only a complete, valid, non-abstained verdict that |
There was a problem hiding this comment.
@messiaen @ayushag-nv please should review this change as well. When an algorithm does not have a clear decision, there should be an ambigious decision so that cascades and fallbacks can move the needle.
219f2ca to
36706fd
Compare
| } | ||
|
|
||
| #[async_trait] | ||
| impl<S: Send> Classifier<S> for DefaultTarget { |
There was a problem hiding this comment.
No preference on how we are setting default target when all algorithms fail to provide a score. Please help review.
| // 5. Offer the outbound request to the processors, paired with the decision that | ||
| // routed it. This is the only hook that sees both, so it is where a | ||
| // decision-dependent rewrite belongs. | ||
| for processor in &self.processors { |
There was a problem hiding this comment.
@linj-glitch please grab this for your latching mechanism with escalation router. For confirmation count and session affinity, this will be the path for next request to skip all classifiers.
There was a problem hiding this comment.
A decision event contains a request.
And we want to process the decision also, no?
There was a problem hiding this comment.
Yeah we want to pass Decision event here, request event implies it's preprocessing I think
There was a problem hiding this comment.
I would imagine we should not touch decision here.
There was a problem hiding this comment.
If Im reading it right, decision is not mutable intentionally so.
There was a problem hiding this comment.
nvm, applying changes per your comment. passing in decision is enough.
1b09552 to
94b68ac
Compare
…apability-classifier fallback Signed-off-by: Sabhatina Selvam <sabhatinas@nvidia.com>
94b68ac to
f4f62df
Compare
messiaen
left a comment
There was a problem hiding this comment.
adding comments still reviewing. But looks good overal
| // 5. Offer the outbound request to the processors, paired with the decision that | ||
| // routed it. This is the only hook that sees both, so it is where a | ||
| // decision-dependent rewrite belongs. | ||
| for processor in &self.processors { |
There was a problem hiding this comment.
A decision event contains a request.
And we want to process the decision also, no?
| } | ||
|
|
||
| #[async_trait] | ||
| impl<S: Send> Classifier<S> for DefaultTarget { |
| // 3. Resolve the target and publish the decision. A tier is a fact about the | ||
| // target, not about which classifier happened to name it first, so any member | ||
| // of the cascade may answer — a terminal fallback that decides a turn would | ||
| // otherwise leave the call untiered. |
There was a problem hiding this comment.
don't need the paragraphs here.
| // 5. Offer the outbound request to the processors, paired with the decision that | ||
| // routed it. This is the only hook that sees both, so it is where a | ||
| // decision-dependent rewrite belongs. | ||
| for processor in &self.processors { |
There was a problem hiding this comment.
Yeah we want to pass Decision event here, request event implies it's preprocessing I think
| let kind = match event { | ||
| Event::Request(_) => "request", | ||
| Event::Decision { .. } => "decision", | ||
| Event::ModelRequest { .. } => "model_request", |
There was a problem hiding this comment.
Decision has the request already.
| &self.efficient_target | ||
| } | ||
| _ => &self.capable_target, | ||
| // Judge output is untrusted, so only a complete, valid, non-abstained verdict that |
| return Classification::Ambiguous(vec![Score { | ||
| target: self.capable_target.clone(), | ||
| confidence: 0.0, | ||
| }]); |
There was a problem hiding this comment.
should we just return an empty set of scores here?
There was a problem hiding this comment.
Is it different from returning confidence = 0?
A stage router cascade algorithm: tool signals decide most turns for free, and only the ones they can't fall through to the LLM classifier. If LLM classifier can not decide, default target is used as fall open.
The cascade
Each step decides or abstains, and the next one gets the turn. The last never abstains, so a turn is never left unrouted.
override/tests_passed/dimensionsllm-classifierfall_openA turn the signals settle never reaches the classifier, so it costs nothing.
StageRouterwraps the cascade and implementsAlgorithm, soFallThroughstays an internal detail.The two tiers are capable and efficient; their targets are whatever the deployment names them.
Changes to
llm_classabstain— instead of quietly forcing the capable tier. The composition picks the fallback, so a judge failure now errs cheap underefficient_firstrather than expensive.recent_turn_windowis now aTaskClassifierConfigknob.Nonekeeps today's newest-user-message-only judging;Some(n)widens to the client instructions, the opening task, and the lastnturns after it.trim_messagesselects by reference and clones only what survives, rather than copying the whole transcript to keep a window of it.Changes to tool signals
Changes to
llm_classabstain— instead of forcing the capable tier. The composition picks the fallback, so a judge failure errs cheap underefficient_first.recent_turn_windowis now aTaskClassifierConfigknob:Nonejudges the newest user message alone,Some(n)adds the instructions, opening task, and lastnturns.trim_messagesclones only what survives instead of copying the transcript.Changes to
fall_throughEvent::ModelRequestnow fires and carries the decision. Declared but never emitted before; it is the only hook that sees both, so a processor can act on the chosen target without keeping state between turns.DefaultTarget— a terminal classifier that always picks one target. Used by this router and the capability route to close their cascades now that classifiers can abstain.Configuration
Thresholds are validated at construction, and the fall-back target is resolved once from the picker mode rather than per turn.
Handoff notes
Tier system prompts
Follow-ups left as TODOs
llm_class.rs), the way the per-target system prompt does. Affinity is a fact about the decision, not a classification, so recording it there would let a pinned session skip the cascade outright on the next turn. It would also make the pin survive composition: a cascade that uses this classifier as one member — the stage router does — never runs the affinity wired inside it, because only the inner classifier is consulted. That is whysession_affinityin astage_router's[classifier]block parses today but has no effect.Testing
tests/stage_router.rs— multi-turn sessions through the assembled router: one note per signal-driven turn and none otherwise, no classifier call when the signals settled, a fresh classifier answer each turn, an unusable verdict landing on the picker default, the configured window reaching the judge, and the correct record of who decided.tests/prompts.rs— the request-text helpers on a plain cascade with no stage routing, which is what proves they are reusable: per-target prompts, one target's prompt not leaking onto another, the prompt following the target whichever classifier picked it, and a note reaching the model in the conversation.stage_routerroute with notes, tier prompts, and a classifier, and pin that a critical tool error escalates on the signals alone — the case that was silently broken.Refs SWITCH-1120, SWITCH-982, SWITCH-1110.