mirror of
https://github.com/BerriAI/litellm.git
synced 2026-11-11 08:38:24 +00:00
fix(ui): mirror backend precedence when resolving a complexity router default
The edit modal only recognized a pin stored in complexity_router_config.default_model, so a router whose default lived solely in litellm_params.complexity_router_default_model lost it on the next save. That field cannot be trusted outright either: before this PR every save wrote a tier-derived value into it, so treating any value as a pin would freeze legacy routers away from their tiers. Hydration now takes the config marker as authoritative and falls back to litellm_params only when it diverges from what the tiers alone derive, which is only reachable through an external API or config write. Test Connection had the mirror-image bug: it fell back to complexity_router_config.default_model, a UI-only marker init_complexity_router_deployment never reads, so it could probe a model the router would never call. It now follows router.py exactly: litellm_params, else pure tier-derivation. Also reword a tooltip that hardcoded the Default Model select's position on the page, and document the dual write and the create-vs-edit validation asymmetry.
This commit is contained in:
parent
de2c59addc
commit
a7abe57123
7 changed files with 177 additions and 14 deletions
|
|
@ -230,7 +230,7 @@ const ClassificationMethodConfig: React.FC<ClassificationMethodConfigProps> = ({
|
|||
<Tooltip
|
||||
title={
|
||||
hasDefaultModel
|
||||
? "Change it in the Default Model row above the advanced sections."
|
||||
? "Change it from the Default Model select."
|
||||
: "Set a default model on this router to use this option"
|
||||
}
|
||||
>
|
||||
|
|
|
|||
|
|
@ -365,6 +365,10 @@ const AddAutoRouterTab: React.FC<AddAutoRouterTabProps> = ({
|
|||
form
|
||||
.validateFields(requiresTeamScope ? ["auto_router_name", "team_id"] : ["auto_router_name"])
|
||||
.then((values) => {
|
||||
// auto_router_default_model (-> litellm_params, read by the backend at init) and
|
||||
// complexity_router_config.default_model (-> the pin marker read back on edit, see
|
||||
// hydratePinnedDefaultModel in edit_auto_router_modal.tsx) must both come from the same
|
||||
// `defaultModel`, or the two fields diverge and hydration's divergence check misfires.
|
||||
const submitValues = {
|
||||
...values,
|
||||
auto_router_name: name,
|
||||
|
|
|
|||
|
|
@ -106,6 +106,12 @@ export const getTierLabelsError = (tierLabels: ComplexityTierLabels | undefined)
|
|||
return null;
|
||||
};
|
||||
|
||||
// Requires all 4 tiers non-empty, so the create form can never reach the
|
||||
// resolveComplexityDefaultModel(tiers, ...) === undefined case — MEDIUM (or SIMPLE) is always
|
||||
// populated. The edit modal has no equivalent of this check (it allows saving with only some
|
||||
// tiers filled), which is why it needs its own explicit `!defaultModel` guard after deriving —
|
||||
// see edit_auto_router_modal.tsx's save handler. A future contributor copying this form's submit
|
||||
// handler elsewhere should not assume the same guarantee holds without this check.
|
||||
export const getMissingTiersError = (tiers: ComplexityTiers): string | null => {
|
||||
const missing = TIER_KEYS.filter((tier) => tiers[tier].length === 0);
|
||||
if (missing.length === 0) return null;
|
||||
|
|
|
|||
|
|
@ -577,6 +577,27 @@ describe("EditAutoRouterModal default model", () => {
|
|||
/>,
|
||||
);
|
||||
|
||||
// No config blob marker — only litellm_params.complexity_router_default_model, as an untouched
|
||||
// router looked before this PR's marker existed, or one an external API call wrote directly to.
|
||||
const renderWithLitellmParamsDefaultOnly = (complexityRouterDefaultModel: string) =>
|
||||
renderWithProviders(
|
||||
<EditAutoRouterModal
|
||||
isVisible
|
||||
onCancel={vi.fn()}
|
||||
onSuccess={vi.fn()}
|
||||
modelData={{
|
||||
...MODEL_DATA,
|
||||
litellm_params: {
|
||||
...MODEL_DATA.litellm_params,
|
||||
complexity_router_config: STORED_CONFIG,
|
||||
complexity_router_default_model: complexityRouterDefaultModel,
|
||||
},
|
||||
}}
|
||||
accessToken="token"
|
||||
userRole="Admin"
|
||||
/>,
|
||||
);
|
||||
|
||||
it("preserves a stored pin through an untouched open-and-save", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithStoredPin("out-of-band-default");
|
||||
|
|
@ -611,6 +632,71 @@ describe("EditAutoRouterModal default model", () => {
|
|||
expect(savedConfig()).toMatchObject({ default_model: STORED_CONFIG.tiers.MEDIUM[0] });
|
||||
});
|
||||
|
||||
// Greptile P1 on #36615: with no config blob marker, a litellm_params default that merely
|
||||
// matches what the tiers derive is indistinguishable from the pre-PR auto-derive-and-write
|
||||
// behavior (main always wrote a tier-derived value there on every save). Treating it as a pin
|
||||
// would freeze every pre-existing router's default away from its tiers, so it stays unpinned.
|
||||
it("treats a litellm_params default matching tier-derivation as unpinned, not a frozen-in pin", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithLitellmParamsDefaultOnly(STORED_CONFIG.tiers.MEDIUM[0]);
|
||||
|
||||
const select = await screen.findByRole("combobox", { name: "Default model" });
|
||||
expect(select.closest(".ant-select")?.querySelector(".ant-select-selection-item")).toBeNull();
|
||||
|
||||
await user.click(screen.getByRole("button", { name: /save changes/i }));
|
||||
await waitFor(() => expect(modelPatchUpdateCall).toHaveBeenCalled());
|
||||
expect(savedConfig()).not.toHaveProperty("default_model");
|
||||
expect(savedDefaultModel()).toBe(STORED_CONFIG.tiers.MEDIUM[0]);
|
||||
});
|
||||
|
||||
// Greptile P1 on #36615: a litellm_params default that diverges from tier-derivation could only
|
||||
// have gotten there via an explicit override — set by the API directly, since this UI's own
|
||||
// save path keeps it in sync with tiers whenever there's no pin. That divergence must survive
|
||||
// the next save instead of being silently recomputed away.
|
||||
it("treats a diverging litellm_params default as an external pin and preserves it", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithLitellmParamsDefaultOnly("claude-sonnet-4");
|
||||
|
||||
const select = await screen.findByRole("combobox", { name: "Default model" });
|
||||
expect(within(select.closest(".ant-select") as HTMLElement).getByTitle("claude-sonnet-4")).toBeInTheDocument();
|
||||
|
||||
await user.click(screen.getByRole("button", { name: /save changes/i }));
|
||||
await waitFor(() => expect(modelPatchUpdateCall).toHaveBeenCalled());
|
||||
expect(savedConfig()).toMatchObject({ default_model: "claude-sonnet-4" });
|
||||
expect(savedDefaultModel()).toBe("claude-sonnet-4");
|
||||
});
|
||||
|
||||
// The config blob marker is this UI's own authoritative record of intent (see
|
||||
// hydratePinnedDefaultModel), so it wins even over a litellm_params value that disagrees —
|
||||
// e.g. a stale value from before the operator most recently changed the pin.
|
||||
it("prefers the config blob marker over a diverging litellm_params value", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithProviders(
|
||||
<EditAutoRouterModal
|
||||
isVisible
|
||||
onCancel={vi.fn()}
|
||||
onSuccess={vi.fn()}
|
||||
modelData={{
|
||||
...MODEL_DATA,
|
||||
litellm_params: {
|
||||
...MODEL_DATA.litellm_params,
|
||||
complexity_router_config: { ...STORED_CONFIG, default_model: "blob-pin" },
|
||||
complexity_router_default_model: "stale-litellm-params-value",
|
||||
},
|
||||
}}
|
||||
accessToken="token"
|
||||
userRole="Admin"
|
||||
/>,
|
||||
);
|
||||
|
||||
const select = await screen.findByRole("combobox", { name: "Default model" });
|
||||
expect(within(select.closest(".ant-select") as HTMLElement).getByTitle("blob-pin")).toBeInTheDocument();
|
||||
|
||||
await user.click(screen.getByRole("button", { name: /save changes/i }));
|
||||
await waitFor(() => expect(modelPatchUpdateCall).toHaveBeenCalled());
|
||||
expect(savedConfig()).toMatchObject({ default_model: "blob-pin" });
|
||||
});
|
||||
|
||||
// This modal only requires one non-empty tier, so a COMPLEX-only router is reachable here even
|
||||
// though the backend raises on it. The block keeps that failure at save time instead of init.
|
||||
it("blocks a save when neither the tiers nor a pin give the backend a default", async () => {
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ import { DEFAULT_MATCH_THRESHOLD } from "../add_model/SemanticKeywordMatching";
|
|||
import { hydrateKeywordTierRules, serializeKeywordTierRules } from "../add_model/complexity_router_keywords";
|
||||
import ComplexityRouterConfig, {
|
||||
ComplexityRouterConfigValue,
|
||||
ComplexityTiers,
|
||||
DEFAULT_ADAPTIVE_WEIGHTS,
|
||||
DEFAULT_SESSION_AFFINITY,
|
||||
DEFAULT_DEPLOYMENT_AFFINITY,
|
||||
|
|
@ -82,8 +83,23 @@ const toRecord = (value: unknown): Record<string, unknown> => {
|
|||
: {};
|
||||
};
|
||||
|
||||
export const hydratePinnedDefaultModel = (stored: unknown): string | undefined =>
|
||||
typeof stored === "string" && stored.trim() ? stored : undefined;
|
||||
// A pin lives in two places: complexity_router_config.default_model (this UI's own marker, added
|
||||
// by PR #36615) and litellm_params.complexity_router_default_model (what the backend reads). Only
|
||||
// the marker proves an operator picked it, because before #36615 every save wrote a tier-derived
|
||||
// value into litellm_params. So with no marker, a litellm_params value counts as a pin only when
|
||||
// it diverges from what the tiers alone derive; a match stays unpinned and keeps tracking tiers.
|
||||
export const hydratePinnedDefaultModel = (
|
||||
storedConfigDefaultModel: unknown,
|
||||
litellmParamsDefaultModel: string | null | undefined,
|
||||
tiers: ComplexityTiers,
|
||||
): string | undefined => {
|
||||
if (typeof storedConfigDefaultModel === "string" && storedConfigDefaultModel.trim()) {
|
||||
return storedConfigDefaultModel;
|
||||
}
|
||||
const tierDerived = resolveComplexityDefaultModel(tiers);
|
||||
const externalOverride = litellmParamsDefaultModel?.trim();
|
||||
return externalOverride && externalOverride !== tierDerived ? externalOverride : undefined;
|
||||
};
|
||||
|
||||
export interface KeywordMatchingState {
|
||||
keywordTierRules: KeywordTierRule[];
|
||||
|
|
@ -241,14 +257,20 @@ const EditAutoRouterModal: React.FC<EditAutoRouterModalProps> = ({
|
|||
parsedConfig = JSON.parse(parsedConfig);
|
||||
}
|
||||
|
||||
const hydratedTiers: ComplexityTiers = {
|
||||
SIMPLE: normalizeTierModels(parsedConfig.tiers?.SIMPLE),
|
||||
MEDIUM: normalizeTierModels(parsedConfig.tiers?.MEDIUM),
|
||||
COMPLEX: normalizeTierModels(parsedConfig.tiers?.COMPLEX),
|
||||
REASONING: normalizeTierModels(parsedConfig.tiers?.REASONING),
|
||||
};
|
||||
|
||||
const hydratedComplexityRouterConfig: ComplexityRouterConfigValue = {
|
||||
tiers: {
|
||||
SIMPLE: normalizeTierModels(parsedConfig.tiers?.SIMPLE),
|
||||
MEDIUM: normalizeTierModels(parsedConfig.tiers?.MEDIUM),
|
||||
COMPLEX: normalizeTierModels(parsedConfig.tiers?.COMPLEX),
|
||||
REASONING: normalizeTierModels(parsedConfig.tiers?.REASONING),
|
||||
},
|
||||
default_model: hydratePinnedDefaultModel(parsedConfig.default_model),
|
||||
tiers: hydratedTiers,
|
||||
default_model: hydratePinnedDefaultModel(
|
||||
parsedConfig.default_model,
|
||||
modelData.litellm_params?.complexity_router_default_model,
|
||||
hydratedTiers,
|
||||
),
|
||||
tier_labels: hydrateTierLabels(parsedConfig.tier_labels),
|
||||
classifier_type: parsedConfig.classifier_type || "heuristic",
|
||||
classifier_llm_config: parsedConfig.classifier_llm_config,
|
||||
|
|
@ -369,7 +391,8 @@ const EditAutoRouterModal: React.FC<EditAutoRouterModalProps> = ({
|
|||
}
|
||||
|
||||
// Unlike the create form, this modal only requires one non-empty tier, so a router can reach
|
||||
// here with nothing the backend would pick as a default. init_complexity_router_deployment
|
||||
// here with nothing the backend would pick as a default (see getMissingTiersError in
|
||||
// build_complexity_router_config.ts for why create never can). init_complexity_router_deployment
|
||||
// raises in that case (litellm/router.py), so block it rather than saving a router that
|
||||
// fails at init.
|
||||
const defaultModel = resolveComplexityDefaultModel(tiers, complexityRouterConfig.default_model);
|
||||
|
|
@ -381,6 +404,9 @@ const EditAutoRouterModal: React.FC<EditAutoRouterModalProps> = ({
|
|||
return;
|
||||
}
|
||||
|
||||
// Dual write: complexity_router_config.default_model (the pin marker hydratePinnedDefaultModel
|
||||
// reads back) and complexity_router_default_model (what the backend routes on) must always be
|
||||
// written together from the same value. Same pairing in add_auto_router_tab.tsx.
|
||||
const updatedLitellmParams = {
|
||||
...modelData.litellm_params,
|
||||
complexity_router_config: buildUpdatedComplexityRouterConfig(
|
||||
|
|
|
|||
|
|
@ -987,6 +987,44 @@ describe("ModelInfoView", () => {
|
|||
expect(mockTestModelGroupConnection).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Bugbot finding on #36615: complexity_router_config.default_model is a UI-only bookkeeping
|
||||
// marker — init_complexity_router_deployment (litellm/router.py) never reads it, falling back
|
||||
// to tier-derivation instead when litellm_params.complexity_router_default_model is absent.
|
||||
// Probing the blob field here would test a model the running router never calls.
|
||||
it("ignores an unused config blob pin when litellm_params has no default, matching the backend's own tier-derivation fallback", async () => {
|
||||
const complexityRouterModelData = {
|
||||
...defaultModelData,
|
||||
litellm_params: {
|
||||
...defaultModelData.litellm_params,
|
||||
model: "auto_router/complexity_router",
|
||||
complexity_router_config: {
|
||||
tiers: { SIMPLE: ["gpt-4o-mini"], MEDIUM: [], COMPLEX: [], REASONING: [] },
|
||||
default_model: "unused-blob-pin",
|
||||
},
|
||||
// no complexity_router_default_model
|
||||
},
|
||||
};
|
||||
|
||||
mockUseModelsInfo.mockReturnValue({
|
||||
data: {
|
||||
data: [complexityRouterModelData],
|
||||
},
|
||||
isLoading: false,
|
||||
error: null,
|
||||
});
|
||||
mockTestModelGroupConnection.mockResolvedValue({ status: "success" });
|
||||
|
||||
render(<ModelInfoView {...DEFAULT_ADMIN_PROPS} />, { wrapper });
|
||||
const testConnectionButton = await screen.findByTestId("test-connection-button");
|
||||
await userEvent.click(testConnectionButton);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockTestModelGroupConnection).toHaveBeenCalledWith("test-token", "gpt-4o-mini", "chat");
|
||||
});
|
||||
expect(mockTestModelGroupConnection).not.toHaveBeenCalledWith("test-token", "unused-blob-pin", "chat");
|
||||
expect(mockTestModelGroupConnection).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("should display model access groups field", async () => {
|
||||
render(<ModelInfoView {...DEFAULT_ADMIN_PROPS} />, { wrapper });
|
||||
await waitFor(() => {
|
||||
|
|
|
|||
|
|
@ -178,14 +178,17 @@ const buildComplexityRouterTestTargets = (
|
|||
REASONING: normalizeTierModels(config.tiers?.REASONING),
|
||||
};
|
||||
|
||||
// litellm_params overrides the config blob at init (complexity_router.py), so it wins here too.
|
||||
const pinned = modelData?.litellm_params?.complexity_router_default_model || config.default_model;
|
||||
// Mirrors init_complexity_router_deployment (litellm/router.py): litellm_params wins, otherwise
|
||||
// pure tier-derivation. complexity_router_config.default_model is a UI-only marker the backend
|
||||
// never reads — folding it in here could point Test Connection at a model the router never
|
||||
// calls (see PR #36615 discussion).
|
||||
const effectiveDefaultModel = modelData?.litellm_params?.complexity_router_default_model || undefined;
|
||||
|
||||
const testTargetParams = {
|
||||
tiers,
|
||||
semanticMatchingEnabled: Boolean(config.semantic_keyword_matching),
|
||||
embeddingModel: config.embedding_model,
|
||||
defaultModel: resolveComplexityDefaultModel(tiers, pinned),
|
||||
defaultModel: resolveComplexityDefaultModel(tiers, effectiveDefaultModel),
|
||||
};
|
||||
return buildAutoRouterTestTargets(testTargetParams);
|
||||
};
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue