Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions web/packages/common/src/api/common/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,10 +288,16 @@ describe('getErrorMessage', () => {
expect(getErrorMessage(error)).toBe('Something went wrong');
});

it('returns fallback message when provided', () => {
const error = new Error('Internal error');
it('prefers the error message over the fallback', () => {
const error = new Error('boom');

expect(getErrorMessage(error, 'Failed to load data')).toBe('Failed to load data');
expect(getErrorMessage(error, 'fallback')).toBe('boom');
});

it('falls back only when the error carries no message', () => {
const error = new Error('');

expect(getErrorMessage(error, 'fallback')).toBe('fallback');
});
});
});
9 changes: 6 additions & 3 deletions web/packages/common/src/api/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export const isVersionConflictError = (error: unknown): boolean =>
* Handles both ValidationError arrays and simple string errors from the backend.
*
* @param error - The error object (typically from an API call)
* @param fallbackMessage - Optional fallback message if no backend detail is available
* @param fallbackMessage - Used only when the error carries no message of its own
* @returns A user-friendly error message string
*
* @example
Expand Down Expand Up @@ -121,6 +121,9 @@ export const getErrorMessage = (error: AxiosError | Error, fallbackMessage?: str
}
}

// Return fallback or generic error message
return fallbackMessage ?? error.message;
// The error's own message first: callers pass `fallbackMessage` for errors that carry
// nothing useful (an AxiosError whose branches above all missed), not to overwrite a
// message that was written to be read. Preferring the fallback turned a precise
// "config has no usable model" into "Unknown error" at the guardrail-run call site.
return error.message || fallbackMessage || '';
};
Original file line number Diff line number Diff line change
Expand Up @@ -48,20 +48,21 @@ describe('resolveConfigModel', () => {
expect(resolveConfigModel(config, 'pii-filter')).toBe('gpt-4');
});

it('falls back to the first model that declares a reference', () => {
const config: RailsConfig = {
models: [{ type: 'embeddings', engine: 'openai', model: 'text-embedding-ada-002' }],
};
expect(resolveConfigModel(config, 'pii-filter')).toBe('text-embedding-ada-002');
});

it.each([
['no models', { models: [] } satisfies RailsConfig],
['models without a reference', { models: [{ type: 'main', engine: 'openai' }] }],
['an absent config', undefined],
// A task LLM is addressed by a rail via `$model=`; running generation against it would
// produce a result that looks valid and means nothing.
[
'only task LLMs',
{
models: [{ type: 'embeddings', engine: 'openai', model: 'text-embedding-ada-002' }],
} satisfies RailsConfig,
],
])('throws a named error for %s', (_label, config) => {
expect(() => resolveConfigModel(config as RailsConfig | undefined, 'pii-filter')).toThrow(
"Guardrail config 'pii-filter' has no usable model to run checks against."
"Guardrail config 'pii-filter' has no main model. Set one on the Configuration tab."
);
});
});
Expand Down Expand Up @@ -199,7 +200,9 @@ describe('runGuardrailCheck against a draft', () => {
it('rejects a draft with no usable model before calling /checks', async () => {
await expect(
runGuardrailCheck(WORKSPACE, snapshot('benign-greeting'), { models: [] })
).rejects.toThrow("Guardrail config 'pii-filter' has no usable model to run checks against.");
).rejects.toThrow(
"Guardrail config 'pii-filter' has no main model. Set one on the Configuration tab."
);
expect(recordedCheckRequests).toHaveLength(0);
});
});
Expand Down
25 changes: 17 additions & 8 deletions web/packages/studio/src/api/guardrail-checks/guardrailChecks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
// Layering wrinkle: this reaches into the components layer. Deliberate — it keeps one
// definition of guardrail identity shared with the config editor's detector catalog.
import { getActivatedGuardrails } from '@studio/components/sidePanels/GuardrailCheckDetailSidePanel/railLabels';
import { getMainModelName } from '@studio/routes/guardrails/GuardrailConfigTab/mainModel';

// ---------------------------------------------------------------------------
// Query keys
Expand Down Expand Up @@ -173,17 +174,25 @@ export async function deleteGuardrailCheck(

/**
* Resolve the model to run a check against from its parent config's model list.
* NeMo Guardrails configs mark the primary generation model with `type: 'main'`;
* we fall back to the first model that declares a `model` reference.
*
* Only the `main` entry is eligible. Every other entry is a task LLM that a specific rail
* addresses by `$model=` — sending `system/nemoguard-8b-content-safety` as the generation
* model would produce a run that looks fine and means nothing.
*
* Studio-specific by design: the service reads `request.model`, not the config
* (`nemo_guardrails_plugin/rails.py:236` — "model name: always request_body['model']"), and
* injects a placeholder when a `main` entry omits one. Storing the name on the config and
* reading it back here is how Studio remembers the user's choice; it is not how the
* service routes. Do not delete the config field on the grounds that the service ignores it.
*/
export function resolveConfigModel(config: RailsConfig | undefined, configLabel: string): string {
const models = config?.models ?? [];
const main = models.find((m) => m.type === 'main' && m.model);
const chosen = main ?? models.find((m) => m.model);
if (!chosen?.model) {
throw new Error(`Guardrail config '${configLabel}' has no usable model to run checks against.`);
const model = getMainModelName(config?.models);
if (!model) {
throw new Error(
`Guardrail config '${configLabel}' has no main model. Set one on the Configuration tab.`
);
}
return chosen.model;
return model;
}

/** What the run targeted, for stamping onto the record. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

import type { RailsConfig } from '@nemo/sdk/generated/platform/schema';
import { getMainModelName as getMainModelNameFromModels } from '@studio/routes/guardrails/GuardrailConfigTab/mainModel';

/**
* Count the total number of configured rail flows across input, output, and
Expand All @@ -22,7 +23,7 @@ export function countRails(data?: RailsConfig): number {

/** Return the `model` field of the first model entry with type "main", or undefined. */
export function getMainModelName(data?: RailsConfig): string | undefined {
return data?.models?.find((m) => m.type === 'main')?.model;
return getMainModelNameFromModels(data?.models) || undefined;
}

export interface RailCounts {
Expand Down
10 changes: 10 additions & 0 deletions web/packages/studio/src/constants/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ export const EMPTY_FIELD_EMDASH_VALUE = '—';
export const DEFAULT_BUILD_MODEL_NAME = 'nvidia-nemotron-nano-3-30b-a3b';
export const DEFAULT_EMBEDDER_MODEL_NAME = 'nvidia-nv-embedqa-e5-v5';

/**
* Engine for a guardrail config's `main` model entry.
*
* Mirrors `DEFAULT_MAIN_ENGINE` in
* `plugins/nemo-guardrails/src/nemo_guardrails_plugin/constants.py`, which is what the
* service falls back to when a config declares no `main` entry. Writing the same value
* keeps a Studio-authored config behaviourally identical to one without the entry.
*/
export const GUARDRAIL_DEFAULT_ENGINE = 'nim';

export const DEFAULT_MAX_PARALLEL_REQUESTS = 2;
export const MAX_PARALLEL_REQUESTS_MIN = 1;
export const MAX_PARALLEL_REQUESTS_MAX = 64;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,24 @@ import type { Mock } from 'vitest';
const WORKSPACE = 'test-workspace';
const CONFIGS_URL = `${PLATFORM_BASE_URL}/apis/guardrails/v2/workspaces/:workspace/configs`;

// The modal seeds a main model from the workspace catalogue. Stubbed rather than served over
// MSW so each test states the catalogue it wants; `modelGroups` is what the hook returns.
let modelGroups: unknown[] = [];
const useModelsFromWorkspaceSpy = vi.fn();
vi.mock('@nemo/common/src/api/models/useModelsFromWorkspace', () => ({
useModelsFromWorkspace: (options: unknown) => {
useModelsFromWorkspaceSpy(options);
return { groups: modelGroups };
},
}));

const MODEL_GROUPS = [
{
workspace: WORKSPACE,
models: [{ name: 'llama-3.1-8b-instruct', workspace: WORKSPACE, model_providers: ['p'] }],
},
];

const SOURCE_CONFIG: GuardrailConfig = {
name: 'my-rail',
workspace: WORKSPACE,
Expand Down Expand Up @@ -58,6 +76,59 @@ describe('CreateGuardrailModal', () => {
mockUseParams({ [ROUTE_PARAMS.workspace]: WORKSPACE });
navigate = vi.fn();
mockUseNavigate(navigate);
modelGroups = [];
useModelsFromWorkspaceSpy.mockClear();
});

it('seeds a fresh config with the resolved main model', async () => {
modelGroups = MODEL_GROUPS;
let body: unknown;
server.use(
http.post(CONFIGS_URL, async ({ request }) => {
body = await request.json();
return HttpResponse.json({ name: 'my-rail' }, { status: 201 });
})
);
const user = userEvent.setup();
renderModal();

await user.type(screen.getByRole('textbox'), 'my-rail');
await user.click(screen.getByRole('button', { name: 'Create' }));

await waitFor(() => {
expect(body).toEqual({
name: 'my-rail',
data: {
models: [
{
type: 'main',
engine: 'nim',
mode: 'chat',
model: `${WORKSPACE}/llama-3.1-8b-instruct`,
},
],
},
});
});
});

it('creates without models when the workspace serves none', async () => {
let body: unknown;
server.use(
http.post(CONFIGS_URL, async ({ request }) => {
body = await request.json();
return HttpResponse.json({ name: 'my-rail' }, { status: 201 });
})
);
const user = userEvent.setup();
renderModal();

await user.type(screen.getByRole('textbox'), 'my-rail');
await user.click(screen.getByRole('button', { name: 'Create' }));

await waitFor(() => {
expect(body).toEqual({ name: 'my-rail' });
});
});

it('creates the guardrail and navigates to its detail page', async () => {
Expand Down Expand Up @@ -116,7 +187,21 @@ describe('CreateGuardrailModal', () => {
expect(screen.getByText('Duplicate Guardrail')).toBeInTheDocument();
});

it('does not query the model catalogue', async () => {
renderModal({ sourceConfig: SOURCE_CONFIG });

// Settle the modal's open effect (name reset + focus) before asserting.
await waitFor(() => {
expect(screen.getByRole('textbox')).toHaveValue('my-rail-copy');
});
// The duplicate carries the source's models; resolving a default would be wasted work.
expect(useModelsFromWorkspaceSpy).toHaveBeenCalledWith(
expect.objectContaining({ queryOptions: { enabled: false } })
);
});

it('creates a copy carrying the source description and data', async () => {
modelGroups = MODEL_GROUPS;
let body: unknown;
server.use(
http.post(CONFIGS_URL, async ({ request }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

import { zodResolver } from '@hookform/resolvers/zod';
import { getErrorMessage } from '@nemo/common/src/api/common/utils';
import { BASIC_ALL_MODELS_DROPDOWN_FILTER } from '@nemo/common/src/api/models/useModels';
import { useModelsFromWorkspace } from '@nemo/common/src/api/models/useModelsFromWorkspace';
import { ControlledTextInput } from '@nemo/common/src/components/form/ControlledTextInput';
import { FormModal } from '@nemo/common/src/components/FormModal';
import { ENTITY_NAME_HELP, entityNameSchema, toCopyName } from '@nemo/common/src/utils/entityName';
Expand All @@ -11,8 +13,11 @@ import type {
GuardrailConfig,
GuardrailConfigInput,
GuardrailConfigInputData,
RailsConfig,
} from '@nemo/sdk/generated/platform/schema';
import { useWorkspaceFromPath } from '@studio/hooks/useWorkspaceFromPath';
import { resolveDefaultGuardrailModel } from '@studio/routes/guardrails/defaultModel';
import { setMainModelName } from '@studio/routes/guardrails/GuardrailConfigTab/mainModel';
import { getGuardrailDetailRoute } from '@studio/routes/utils';
import { useQueryClient } from '@tanstack/react-query';
import { type FC, useEffect, useRef } from 'react';
Expand Down Expand Up @@ -41,6 +46,14 @@ export const CreateGuardrailModal: FC<Props> = ({ open, onClose, sourceConfig })
const isDuplicate = Boolean(sourceConfig);
const defaultName = sourceConfig?.name ? toCopyName(sourceConfig.name) : '';

// Only for a fresh config — a duplicate inherits the source's models below. Held to the
// open modal so closing it doesn't leave a query running.
const { groups } = useModelsFromWorkspace({
workspace: workspace ?? null,
query: BASIC_ALL_MODELS_DROPDOWN_FILTER,
queryOptions: { enabled: open && !isDuplicate },
});

const {
control,
handleSubmit,
Expand Down Expand Up @@ -86,6 +99,16 @@ export const CreateGuardrailModal: FC<Props> = ({ open, onClose, sourceConfig })
if (sourceConfig) {
if (sourceConfig.description) payload.description = sourceConfig.description;
if (sourceConfig.data) payload.data = { ...sourceConfig.data } as GuardrailConfigInputData;
} else {
// Seed the main model so the new config can run its tests without a detour through
// the Configuration tab. Resolution returning null (no models served, or none with a
// provider) creates the config without `models` — the tab's field then shows empty,
// which beats writing a name that fails at run time.
const model = resolveDefaultGuardrailModel(groups);
if (model) {
const railsConfig: RailsConfig = { models: setMainModelName(undefined, model) };
payload.data = railsConfig as GuardrailConfigInputData;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

let config: GuardrailConfig;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { GuardrailCheckDetailSidePanel } from '@studio/components/sidePanels/Gua
import { ROUTE_PARAMS } from '@studio/constants/routes';
import { GuardrailChecksSubTab } from '@studio/routes/guardrails/GuardrailChecksTab/constants';
import { GuardrailTestCard } from '@studio/routes/guardrails/GuardrailChecksTab/GuardrailTestCard';
import { getMainModelName } from '@studio/routes/guardrails/GuardrailConfigTab/mainModel';
import { getGuardrailChecksSubTabRoute } from '@studio/routes/utils';
import { useRequiredPathParams } from '@studio/util/hooks/useRequiredPathParams';
import { ListChecks, Plus } from 'lucide-react';
Expand Down Expand Up @@ -95,8 +96,17 @@ export const GuardrailTestCasesEditor: FC<GuardrailTestCasesEditorProps> = ({

const isRunning = isFlushing || runMutation.isPending;

// The config a run would actually target. A run without a main model fails identically for
// every check, so refuse up front rather than after N round trips.
const targetConfig = runTarget === 'draft' ? draftConfig : configData;
const hasMainModel = Boolean(getMainModelName(targetConfig?.models));
const runBlockedReason =
!checks.length || hasMainModel
? undefined
: 'Set a main model on the Configuration tab to run tests';

const handleRunAll = async () => {
if (!checks.length || isRunning) return;
if (!checks.length || isRunning || !hasMainModel) return;
setIsFlushing(true);
try {
// Clicking Run blurs the focused message, dispatching that card's save. Await it, or the
Expand Down Expand Up @@ -152,7 +162,8 @@ export const GuardrailTestCasesEditor: FC<GuardrailTestCasesEditorProps> = ({
kind="primary"
height={32}
loading={isRunning}
disabled={!checks.length || isRunning}
disabled={!checks.length || isRunning || !hasMainModel}
title={runBlockedReason}
onClick={() => void handleRunAll()}
>
<ListChecks size={16} />
Expand Down
Loading
Loading