Repository navigation
[api][plan][python][examples] Framework-managed LLM-as-judge routing (Strategies.llm) - #1042
Conversation
…(Strategies.llm) The follow-up promised in discussion apache#897 and the v1 javadoc: the engine — not the strategy — executes the judge call, on the same durable, metered, observable chat path as any model call. - Strategies.llm(judgeModel[, promptTemplate]): declarative built-in; the judge is any registered CHAT_MODEL; candidate describe(...) lines are its decision criteria. - LlmJudgeRoutingStrategy carries config plus two pure functions (prompt build, verdict parse); route() throws — it is never invoked. The verdict parser scans every "model" match for a candidate, so a chatty judge that quotes the format contract still parses. - ModelRoutingResolver runs the judge via the invoker under durable id "judge:<router>" (engine retries, trace events, token attribution to the judge model), derives the decision as a pure function of the verdict, and persists it — with its source and judge-inclusive decision_ms — under the standard "route:<router>" id; replayed decisions are guarded against candidate-set changes like the strategy path. - Failure policy: unparseable or non-candidate verdicts abstain to the default model; a judge that exhausts its retries honors the request's error-handling strategy (FAIL is loud, IGNORE degrades to the default with the cause recorded); interrupts propagate. - Shared retry-policy helpers on ChatModelInvoker (used by both the chat path and the judge path). - Python: add_resource(MODEL_ROUTER) now raises an explicit not-yet-supported error instead of dropping silently (per apache#964 review). - ModelRoutingJudgeExample mirrors ModelRoutingExample with a judge. Generated-by: Claude Code 2.1.239 (Claude Fable 5)
A typo'd judge model name would not fail the job: every judge call would fail and (under IGNORE) abstain to the default model, silently disabling routing. All resources are known at plan construction, so fail there instead — same fail-fast standard as the router/chat-model name-clash check. Validation instantiates the strategy exactly as the runtime does (instanceof dispatch + getJudgeModel()), so subclasses are judged by what they actually return; anything not instantiable at plan time is left to the runtime's own error. Null-tolerant for previously-legal constructor inputs. Generated-by: Claude Code 2.1.239 (Claude Fable 5)
1c16662 to
bb615cb
Compare
|
Thanks for opening the follow-up PR @purushah. Could you please create a corresponding github tracking issue and link the design discussion, #964, and this PR? The issue should capture the complete scope: what was delivered in V1, the planned V2 implementation, Python-side parity, and user documentation. Any additional follow-ups can then be linked to it so we can track the feature end to end. I’ll start reviewing this PR as soon as possible. |
|
Done — opened #1062 to track the feature end to end (design discussion #897, v1 #964, this PR, plus Python parity and user documentation as follow-ups). @wenjin272 |
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for the work on this—the implementation looks solid overall. I’ve left a few minor comments.
Since LLM-based routing has a special framework-managed execution model, it also prompted some broader thoughts about the current routing API design, particularly the separation between API-level strategy declarations and Plan-level execution. It would be good to align on this design before finalizing the PR.
Adopts the declaration/executor split from the PR apache#1042 review (wenjin272), with two amendments keeping apache#964's guarantees structural, and folds in the remaining review items from both reviewers. Architecture (review thread on LlmJudgeRoutingStrategy.java:84): - RoutingStrategy is now an immutable, serializable declaration: RoutingStrategyType (RULE_BASED | LLM_JUDGE | CUSTOM) + arguments (+ executorClass for CUSTOM). No route() exists at the API layer; the throwing UnsupportedOperationException class is gone. - Plan JSON encodes the strategy as a language-neutral tag (strategy_type: "llm_judge") + strategy_args — no Java FQCNs for built-ins, unblocking Python parity (apache#1062). Java-produced snapshot committed; the Python cross-language test now deserializes it verbatim. - Execution lives in Plan: rule matching in the resolver over patterns precompiled by the router; judge prompt/verdict logic in LlmJudgeRoutingExecutor (package-private pure helpers); dispatch by type. - Amendment 1: the resolver owns the durable route: boundary for every strategy type — no executor can skip persistence; replay never re-invokes the strategy or the judge. - Amendment 2: user-defined executors implement CustomRoutingExecutor with the data-only RoutingContext (v1's select-not-delegate fence, verbatim); the engine-facing signature stays plan-internal. Review fixes: - isCancellation() replaces containsInterrupt(): thread-state + explicit cancellation types only; a bare InterruptedIOException (Okio timeout shape) follows the normal FAIL/IGNORE policy. Regression test included. - Judge input is now full-fidelity: complete message list plus the rendered request (anchor candidate's bound prompt + args), with an opt-in Strategies.llm(...).withMaxContextChars(n) cap (SYSTEM + newest pinned, newest-first fill, judge_context_truncated flagged in metadata). - Judge setup constraints (no prompt/tools/skills) validate at plan construction; judgeSetupMisconfiguration() removed from the request path. - Strategy argument validation lives in the declaration constructor, so a bad-args judge fails Strategies.llm(...)/plan construction — the reflective instantiateIfLlmJudge() bypass is structurally gone. - decision_source is no longer duplicated into decision metadata (it contradicted the top-level key on judge+fallback). - FakeRunnerContext gained a seedable durable store; new tests cover the replay path (stored abstain -> current default; decision_ms survives) and judge token metadata (extraArgs-driven). Generated-by: Claude Code 2.1.239 (Claude Fable 5)
Fixes the 10 findings from the fresh review pass over the restructure commit (2 of which were holes the restructure itself opened): - Restore the runtime judge-plainness backstop: plan-time validation only sees descriptor-carried bindings, so instance-level (or non-Java) bound prompt/tools/skills on the judge are again caught at request time (FAIL loud, IGNORE abstain). Tests restored. - Judge promptArgs fallback now keys on missing REQUEST text (no non-empty USER/rendered message) instead of a fully-empty conversation — SYSTEM + empty USER + args no longer routes blind. Regression test added. - The context cap now pins messages generated by the anchor's request shaping (rendered template, skill prompt) via identity-tracked indices, honoring withMaxContextChars' documented 'rendered request always kept'. - Prompt application extracted to BaseChatModelSetup.prepareRequestMessages and shared by chat() and the judge input path — the judge's view can no longer drift from the chat path's (now also includes the skill-discovery prompt); reads getPrompt() so subclass overrides are honored. - Plan validation now fails loudly on a router descriptor without strategy_type instead of deferring to a per-record TaskManager error. - Rule validation/compilation consolidated to one path (compileRules), shared by build() and the constructor; identical diagnostics either way. - Dead disjunct removed in ChatModelAction's cancellation check; stale comment refreshed. - RoutingStrategy.validate() throws on unhandled future types instead of silently passing them (mirrors the executor dispatch). - selectWithinBudget early-returns when no cap is configured. Known, documented limitation (reviewed, deferred): the judge's rendered view anchors to the default candidate's bound prompt; candidates binding different prompts see their own rendering only at answer time. Generated-by: Claude Code 2.1.239 (Claude Fable 5)
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for working through the five earlier points. All five look closed to me. A few new questions inline on the restructure.
…, replay and multi-turn tests - Validate RULE_BASED declarations at plan construction via the shared ModelRouter.compileRules path (now public): invalid patterns, non-String values and rule keys naming non-candidates all fail with the builder's diagnostics instead of per record inside the durable call (descriptor-built plans previously skipped the builder check) - Restore the multi-turn rule test lost in the restructure: rules match the most recent user message, not the first - Add a judge-router replay test: both durable records (judge:<router>, route:<router>) replay and the judge is never re-invoked - Guard judge token metadata with the same both-or-neither instanceof-Number conjunction as the metrics reader, so metrics and routing metadata always agree - Rename validateLlmJudgeReferences to validateRoutingStrategies (it now dispatches all three strategy types) and add CANDIDATES_KEY - Fix ModelRouter javadoc: routers are cached per subtask, not per TaskManager - Python: extract the shared MODEL_ROUTER registration guard into check_registrable_from_python, parametrize the test over both entry points Generated-by: Claude Code 2.1.239 (Claude Fable 5)
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for working through these. All six from the last round check out. Three small things inline, all in the new code.
… guard, shape checks, comment fixes - validateRuleKeys compiles the rule map before the 'candidates' shape guard, so invalid patterns fail at plan construction even when the candidates argument is unusable (regression test included) - A non-List 'candidates' and a non-Map 'rules' now fail at plan construction with named diagnostics instead of a raw per-record ClassCastException / silently-empty rule set at request time - Trim the judge-token comment: the metrics reader has extra conditions (metric group, model_name, counts > 0), so type-guard parity is the only claim - Drop the stale compileRules javadoc sentence that contradicted the single-validation-path rewrite - Extract ruleBasedRouterArgs/ruleBasedProviders test helpers (four hand-rolled descriptor maps collapsed into one template) Generated-by: Claude Code 2.1.239 (Claude Fable 5)
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for the thoughtful updates and for addressing the previous comments. I’ve left a couple of follow-up thoughts about the execution-path abstraction: the declarations are now unified, while rule, custom, and LLM execution still follow different paths, with some execution details remaining in the API layer. A common Plan-side RoutingExecutor abstraction may provide a cleaner boundary. This is mainly something I think is worth discussing.
…or (round-4 review) Adopts wenjin272's execution abstraction, completing the W6 declaration/ executor split for the execution half: - New plan-side RoutingExecutor contract implemented by RuleBasedRoutingExecutor, LlmJudgeRoutingExecutor (a real executor now, orchestrating its own durable judge chat) and CustomRoutingExecutorAdapter; all type dispatch confined to the RoutingExecutors registry - ModelRoutingResolver reduced to executor lookup, the single durable route:<router> persistence boundary, normalization and events. One contract point remains: the durable substrate replays a flat, order-matched call sequence and cannot nest, so an executor with its own durable sub-calls (the judge chat) runs before the persistence call (issuesDurableCalls); pure executors run inside it and are never re-invoked on replay - ModelRouter slimmed to declaration data only: compiled-rule and custom-executor state moved to the plan-side executors (compilation cached per declaration, weakly keyed to match the per-subtask router cache); compileRules removed from the API surface - Rule shape/type/pattern validation moved into the RoutingStrategy constructor — the single declaration-validation path shared by the builder, the router constructor and plan-time validation - RoutingContext carries the router's default model (the judge's rendering anchor) so executors need no ModelRouter reference - Removed the per-request judgeSetupMisconfiguration backstop: plan validation covers descriptor-carried bindings, and a setup that binds a prompt/tools/skills dynamically without declaring them is outside the documented Strategies.llm judge contract (its verdicts stop parsing and requests abstain to the default, visible in events) Generated-by: Claude Code 2.1.239 (Claude Fable 5)
Fixes regressions the RoutingExecutor restructure introduced: - Custom-executor instantiation moved to a new RoutingExecutor.prepare() hook, invoked by the resolver OUTSIDE the persistence boundary: a transiently failing user constructor throws fresh per request instead of being persisted as the route:<router> record and replaying as a permanent poison (regression test asserts no decision record is attempted on constructor failure) - Executor caches (compiled rules, custom instances) moved from JVM-global synchronized WeakHashMaps to thread-confined state on the task mailbox thread: no global lock on the routing hot path, no user constructors running under a JVM-wide monitor, and a cancelled job's user classloader is released with the task thread instead of being pinned by static state - Strategies.rules copies the caller's map, so post-construction mutation cannot bypass declaration validation before the lazy plan-side compile - Rule patterns compile with one shared flag constant (RoutingStrategy.RULE_PATTERN_FLAGS) in both validation and execution, so the two sites cannot drift - Both route:<router> persistence shapes share one DurableCallable definition (routeDecisionCallable) - effectiveJudgeMessages uses orElseGet (eager orElse argument threw on an empty candidate list even with a default model set) - Cache/eviction javadocs corrected (ResourceCache has no eviction path); stale compileRules references removed from test docs Generated-by: Claude Code 2.1.239 (Claude Fable 5)
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments. Only two minor comments.
Could we add a dedicated routing package in the Plan module like API module and move the routing resolver, executor interface, registry, and implementations there? This would keep the routing logic better organized.
…eview) The new name states the contract directly: route() invokes durable execution internally, so it must run outside the resolver's outer route:<router> durable boundary. Generated-by: Claude Code 2.1.239 (Claude Fable 5)
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for working through these. All three from the last round are fixed, and I left two more comments inline.
… validation, plan-time hardening Review (weiqingy, wenjin272): - Move the plan-side routing classes (RoutingExecutor, RoutingExecutors, ModelRoutingResolver, ResolvedModelRoute, the rule/judge/custom executors) and LlmJudgeRoutingExecutorTest into a dedicated org.apache.flink.agents.plan.routing package. ChatModelAction reads the resolved route through public getters; ChatModelInvoker's retry helpers become public for the judge. - validateJudge reads the descriptor of Java- and Python-backed judge setups (descriptorOf), so a judge declared with pythonClazz can no longer bind prompt/tools/skills unnoticed now that the runtime backstop is gone. validateRoutingStrategies uses the same helper for routers. - Fix the stale javadoc reference to effectiveJudgeMessages. Self-review hardening: - 'candidates' shape guard applies to every strategy type, not only RULE_BASED. - A non-String strategy_type fails with a router-naming IllegalArgumentException instead of a raw ClassCastException. - Custom executors receive a mutable copy of their declared arguments. - max_context_chars is validated as a long within (0, Integer.MAX_VALUE].
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for working through these. All of my comments are resolved, nothing further from me.
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. LGTM
Adds docs/development/model_routing: a guide page covering router declaration, the rule-based, LLM-judge and custom strategies, default model and fallback with the error-handling policy table, durability and replay, and observability; plus a Reference subpage with the routing event and response fields, metrics, validation stages, and descriptor keys. Cross-links from Chat Models, Workflow Agent, and Monitoring (new rows for the routingDecisionLatencyMs histogram and the _model_routing_event type). Sibling page weights shift by one so the page sits after Chat Models; this also resolves the pre-existing MCP/Skills weight tie. Covers the doc-needed label on apache#964 and apache#1042.
What
Implements the
Strategies.llm(...)follow-up promised in discussion #897: the engine — not the strategy — executes the judge call, on the same durable, metered, observable chat path as any model call. Tracking: #1062.Revised after review (thanks @wenjin272, @weiqingy): the PR now adopts the declaration/executor architecture proposed in review — one serializable
RoutingStrategydeclaration at the API layer, executors in Plan, dispatch by a language-neutral strategy type — plus all eleven review items. The user-facing builder is unchanged:Architecture (revised per review)
RoutingStrategyis a declaration, not behavior:RoutingStrategyType(RULE_BASED|LLM_JUDGE|CUSTOM) + arguments (+ executor class forCUSTOM). There is noroute()at the API layer — the previousUnsupportedOperationException-throwing judge class is gone.LlmJudgeRoutingExecutor(package-private pure helpers). Noinstanceofon concrete classes.strategy_type: "llm_judge"+strategy_args— no Java class names for built-ins. A Java-emitted snapshot is committed and the Python cross-language test deserializes it verbatim, so Python parity ([Umbrella] Pluggable model routing (MODEL_ROUTER): delivery status, Python parity, documentation, and follow-ups #1062) needs only Python-side executors.route:<router>) for every strategy type — no executor can skip persistence; replay short-circuits execution, so neither the strategy nor the judge is re-invoked on recovery.CustomRoutingExecutorwith the data-onlyRoutingContext(v1's select-don't-delegate fence, verbatim): a custom executor cannot make hidden model calls. The engine-facing signature stays plan-internal.Judge semantics (unchanged from the original design, per #897)
"judge:<router>"(engine retries, trace events, token attribution); decision persisted with source and judge-inclusivedecision_msunder"route:<router>"; abstains persist as abstains, so replay after a candidate-set change resolves to the current default.FAILloud,IGNOREabstain-with-cause). Cancellation is determined from the thread's interrupt state plus explicit cancellation types (isCancellation); a bareInterruptedIOException(the shape Okio uses for ordinary HTTP timeouts) follows the normal failure policy — with a regression test.BaseChatModelSetup.prepareRequestMessagesthe chat path uses, so the judge's view cannot drift from what the selected model receives. Opt-in cost cap:withMaxContextChars(n)(rendered request + SYSTEM pinned, newest-first fill, truncation flagged asjudge_context_truncated).Strategies.llm(...)/plan construction — no reflective validation path to bypass). Plan construction verifies the judge model is registered and its descriptor binds no prompt/tools/skills; a slim runtime backstop catches instance-level bindings plan-time cannot see. Custom executor classes are checked (exists / implements / constructor) without instantiation.add_resource(..., MODEL_ROUTER)raises an explicit not-yet-supported error.Deliberate trade-offs (reviewed, kept)
route:in addition tojudge:) to keep the route-record schema uniform and preserve replayeddecision_ms.Claims → tests
llmJudgeVerdictRoutesToNamedCandidatellmJudgeUnparseableVerdictAbstainsToDefault,llmJudgeVerdictOutsideCandidatesAbstainsToDefault,parseVerdictAcceptsOnlyCandidatesstoredAbstainReplaysToCurrentDefaultWithoutRerunningStrategydecision_msreportedstoredDecisionReplaysWithOriginalDecisionMsInterruptedIOException) under IGNORE abstains, thread not left interruptedjudgeInterruptedIOTimeoutAbstainsToDefaultUnderIgnorejudgeSeesFullConversation,judgeSeesRenderedBoundPromptmaxContextCharsCapsJudgeInputAndFlagsTruncation,contextCapPinsRenderedTemplateMessagesjudgeMessagesFallBackToPromptArgsWithSystemAndEmptyUserjudgeTokenCountsLandInDecisionMetadatajudgeWithMissingJudgeModelFailsAtPlanConstruction,typoedJudgeModelFailsAtPlanConstruction,judgeWithBoundPrompt/ToolsFailsAtPlanConstructionllmJudgeRejectsInstanceLevelPromptBoundJudgeSetupUnderFail,llmJudgePromptBoundSetupAbstainsToDefaultUnderIgnorecustomExecutorIsNotInstantiatedDuringPlanConstructiontest_python_can_deserialize_java_plan_with_judge_routerCompatibility impact
CustomRoutingExecutor.route(strategy, context)instead ofRoutingStrategy.route(context);Strategies.of(...)is nowStrategies.custom(...); the plan wire format usesstrategy_type/strategy_args(+strategy_executor_classfor CUSTOM) instead ofstrategy_clazz.decision_sourcevaluellm_judgeand optional metadata keysjudge_model,judge_prompt_tokens,judge_completion_tokens,judge_context_truncated. (decision_sourceis no longer duplicated inside the metadata map — it contradicted the top-level key on the judge+fallback path.)Test evidence
flink runofModelRoutingJudgeExample, local Ollama): judge verdicts routed with full metadata; fallback composition exercised live.Generative AI was used for this change.
Generated-by: Claude Code 2.1.239 (Claude Fable 5)