-
Notifications
You must be signed in to change notification settings - Fork 749
FIX: Render target capabilities as structured columns in config table #1691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e4ed297
4e6aa88
d336751
d577275
e7bdfce
357213f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,12 +17,28 @@ const sampleTargets: TargetInstance[] = [ | |
| target_type: 'OpenAIChatTarget', | ||
| endpoint: 'https://api.openai.com', | ||
| model_name: 'gpt-4', | ||
| capabilities: { | ||
| supports_multi_turn: true, | ||
| supports_multi_message_pieces: true, | ||
| supports_json_schema: true, | ||
| supports_json_output: true, | ||
| supports_editable_history: true, | ||
| supports_system_prompt: true, | ||
| }, | ||
| }, | ||
| { | ||
| target_registry_name: 'azure_image_dalle', | ||
| target_type: 'AzureImageTarget', | ||
| endpoint: 'https://azure.openai.com', | ||
| model_name: 'dall-e-3', | ||
| capabilities: { | ||
| supports_multi_turn: false, | ||
| supports_multi_message_pieces: false, | ||
| supports_json_schema: false, | ||
| supports_json_output: false, | ||
| supports_editable_history: false, | ||
| supports_system_prompt: false, | ||
| }, | ||
| }, | ||
| { | ||
| target_registry_name: 'text_target_basic', | ||
|
|
@@ -58,7 +74,7 @@ describe('TargetTable', () => { | |
| expect(screen.getAllByText('TextTarget').length).toBeGreaterThanOrEqual(1) | ||
| }) | ||
|
|
||
| it('should display Type, Model, Endpoint and Parameters columns', () => { | ||
| it('should display Type, Model, Endpoint, capability columns and Parameters columns', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <TargetTable {...defaultProps} /> | ||
|
|
@@ -68,6 +84,12 @@ describe('TargetTable', () => { | |
| expect(screen.getByText('Type')).toBeInTheDocument() | ||
| expect(screen.getByText('Model')).toBeInTheDocument() | ||
| expect(screen.getByText('Endpoint')).toBeInTheDocument() | ||
| expect(screen.getByText('Multi-turn')).toBeInTheDocument() | ||
| expect(screen.getByText('Multi-piece')).toBeInTheDocument() | ||
| expect(screen.getByText('JSON Schema')).toBeInTheDocument() | ||
| expect(screen.getByText('JSON Output')).toBeInTheDocument() | ||
| expect(screen.getByText('Edit History')).toBeInTheDocument() | ||
| expect(screen.getByText('System Prompt')).toBeInTheDocument() | ||
| expect(screen.getByText('Parameters')).toBeInTheDocument() | ||
| }) | ||
|
|
||
|
|
@@ -151,10 +173,24 @@ describe('TargetTable', () => { | |
| </TestWrapper> | ||
| ) | ||
|
|
||
| // Dashes for model, endpoint, and 6 capability columns (all unknown) | ||
| const dashes = screen.getAllByText('—') | ||
| expect(dashes.length).toBeGreaterThanOrEqual(2) | ||
| }) | ||
|
|
||
| it('should show dash for capability columns when capabilities is absent', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <TargetTable {...defaultProps} targets={[sampleTargets[2]]} /> | ||
| </TestWrapper> | ||
| ) | ||
|
|
||
| // TextTarget has no capabilities — all 6 should be dashes | ||
| const dashes = screen.getAllByText('—') | ||
| // model (—) + endpoint (—) + 6 capabilities (—) + params (—) = 9 | ||
| expect(dashes.length).toBeGreaterThanOrEqual(8) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the comment above is correct don't we want this to be |
||
| }) | ||
|
|
||
| it('should display target_specific_params when present', () => { | ||
| const targetWithParams: TargetInstance[] = [ | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import { | |
| Tooltip, | ||
| Select, | ||
| } from '@fluentui/react-components' | ||
| import { CheckmarkRegular } from '@fluentui/react-icons' | ||
| import { CheckmarkRegular, CheckmarkCircleFilled, DismissCircleFilled } from '@fluentui/react-icons' | ||
| import type { TargetInstance } from '../../types' | ||
| import { useTargetTableStyles } from './TargetTable.styles' | ||
|
|
||
|
|
@@ -39,6 +39,27 @@ function formatParams(params?: Record<string, unknown> | null): string { | |
| return parts.join('\n') | ||
| } | ||
|
|
||
| /** Capability column definitions with tooltip descriptions. */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really like these tooltips! could go either way on this but should we add tooltips for the other fields in this view too (type, model, endpoint, parameters)? |
||
| const CAPABILITY_COLUMNS = [ | ||
| { key: 'supports_multi_turn', label: 'Multi-turn', tooltip: 'Supports multi-turn conversation history' }, | ||
| { key: 'supports_multi_message_pieces', label: 'Multi-piece', tooltip: 'Supports multiple message pieces in a single request' }, | ||
| { key: 'supports_json_schema', label: 'JSON Schema', tooltip: 'Supports constraining output to a JSON schema' }, | ||
| { key: 'supports_json_output', label: 'JSON Output', tooltip: 'Supports JSON output format' }, | ||
| { key: 'supports_editable_history', label: 'Edit History', tooltip: 'Allows attack history to be modified' }, | ||
| { key: 'supports_system_prompt', label: 'System Prompt', tooltip: 'Supports system prompts' }, | ||
| ] as const | ||
|
|
||
| /** Render a capability indicator: ✓ (green) / ✗ (red) / — (unknown). */ | ||
| function CapabilityCell({ value }: { value: boolean | undefined }) { | ||
| if (value === undefined) { | ||
| return <Text size={200}>—</Text> | ||
| } | ||
| if (value) { | ||
| return <CheckmarkCircleFilled style={{ color: 'green', fontSize: '18px' }} /> | ||
| } | ||
| return <DismissCircleFilled style={{ color: 'red', fontSize: '18px' }} /> | ||
|
Comment on lines
+57
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that we should be hardcoding these values for color and fontSize. Looking at a comparable element in AttackTable.tsx it uses tokens.colorPaletteGreenForeground1 and tokens.colorPaletteRedForeground1. We can probably either drop the explicit font size or use a token for this as well. AttackTable lets the icons inherit, so we could do that same thing here. Worth noting as well that AttackTable uses Regular instead of Filled icons, not necessarily a concern but it is a bit of insistency between tables. |
||
| } | ||
|
|
||
| /** Render the model cell with a tooltip when underlying model differs. */ | ||
| function ModelCell({ target }: { target: TargetInstance }) { | ||
| const displayName = target.model_name || '—' | ||
|
|
@@ -62,6 +83,21 @@ function ModelCell({ target }: { target: TargetInstance }) { | |
| return <Text size={200}>{displayName}</Text> | ||
| } | ||
|
|
||
| /** Render capability cells for a target. */ | ||
| function CapabilityCells({ target }: { target: TargetInstance }) { | ||
| return ( | ||
| <> | ||
| {CAPABILITY_COLUMNS.map(({ key }) => ( | ||
| <TableCell key={key} style={{ width: '80px', textAlign: 'center' }}> | ||
| <CapabilityCell | ||
| value={target.capabilities?.[key]} | ||
| /> | ||
| </TableCell> | ||
| ))} | ||
| </> | ||
| ) | ||
| } | ||
|
|
||
| export default function TargetTable({ targets, activeTarget, onSetActiveTarget }: TargetTableProps) { | ||
| const styles = useTargetTableStyles() | ||
| const [typeFilter, setTypeFilter] = useState('') | ||
|
|
@@ -99,6 +135,7 @@ export default function TargetTable({ targets, activeTarget, onSetActiveTarget } | |
| {activeTarget.endpoint || '—'} | ||
| </Text> | ||
| </TableCell> | ||
| <CapabilityCells target={activeTarget} /> | ||
| <TableCell style={{ width: '200px' }}> | ||
| <Text size={200} className={styles.paramsCell}> | ||
| {formatParams(activeTarget.target_specific_params) || '—'} | ||
|
|
@@ -126,12 +163,19 @@ export default function TargetTable({ targets, activeTarget, onSetActiveTarget } | |
| )} | ||
|
|
||
| <Table aria-label="Target instances" className={styles.table}> | ||
| <TableHeader> | ||
| <TableHeader className={styles.stickyHeader}> | ||
| <TableRow> | ||
| <TableHeaderCell style={{ width: '120px' }} /> | ||
| <TableHeaderCell style={{ width: '200px' }}>Type</TableHeaderCell> | ||
| <TableHeaderCell style={{ width: '180px' }}>Model</TableHeaderCell> | ||
| <TableHeaderCell style={{ minWidth: '300px' }}>Endpoint</TableHeaderCell> | ||
| {CAPABILITY_COLUMNS.map(({ key, label, tooltip }) => ( | ||
| <TableHeaderCell key={key} style={{ width: '80px', textAlign: 'center' }}> | ||
| <Tooltip content={tooltip} relationship="description"> | ||
| <Text size={200} style={{ cursor: 'help' }}>{label}</Text> | ||
| </Tooltip> | ||
| </TableHeaderCell> | ||
| ))} | ||
| <TableHeaderCell style={{ width: '200px' }}>Parameters</TableHeaderCell> | ||
| </TableRow> | ||
| </TableHeader> | ||
|
|
@@ -167,6 +211,7 @@ export default function TargetTable({ targets, activeTarget, onSetActiveTarget } | |
| {target.endpoint || '—'} | ||
| </Text> | ||
| </TableCell> | ||
| <CapabilityCells target={target} /> | ||
| <TableCell> | ||
| <Text size={200} className={styles.paramsCell}> | ||
| {formatParams(target.target_specific_params) || '—'} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,15 @@ export interface PaginationInfo { | |
|
|
||
| // --- Targets --- | ||
|
|
||
| export interface TargetCapabilitiesInfo { | ||
| supports_multi_turn: boolean | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit is the font size for this table purposefully different? It's slightly smaller looking than the other headers |
||
| supports_multi_message_pieces: boolean | ||
| supports_json_schema: boolean | ||
| supports_json_output: boolean | ||
| supports_editable_history: boolean | ||
| supports_system_prompt: boolean | ||
| } | ||
|
|
||
| export interface TargetInstance { | ||
| target_registry_name: string | ||
| target_type: string | ||
|
|
@@ -63,6 +72,7 @@ export interface TargetInstance { | |
| top_p?: number | null | ||
| max_requests_per_minute?: number | null | ||
| supports_multi_turn?: boolean | ||
| capabilities?: TargetCapabilitiesInfo | null | ||
| target_specific_params?: Record<string, unknown> | null | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,23 @@ | |
| from pyrit.backend.models.common import PaginationInfo | ||
|
|
||
|
|
||
| class TargetCapabilitiesInfo(BaseModel): | ||
| """Structured capability flags for a target instance.""" | ||
|
|
||
| supports_multi_turn: bool = Field(..., description="Whether the target supports multi-turn conversation history") | ||
| supports_multi_message_pieces: bool = Field( | ||
| ..., description="Whether the target supports multiple message pieces in a single request" | ||
| ) | ||
| supports_json_schema: bool = Field( | ||
| ..., description="Whether the target supports constraining output to a JSON schema" | ||
| ) | ||
| supports_json_output: bool = Field(..., description="Whether the target supports JSON output format") | ||
| supports_editable_history: bool = Field( | ||
| ..., description="Whether the target allows the attack history to be modified" | ||
| ) | ||
| supports_system_prompt: bool = Field(..., description="Whether the target supports system prompts") | ||
|
|
||
|
|
||
| class TargetInstance(BaseModel): | ||
| """ | ||
| A runtime target instance. | ||
|
|
@@ -37,6 +54,7 @@ class TargetInstance(BaseModel): | |
| top_p: Optional[float] = Field(None, description="Top-p parameter for generation") | ||
| max_requests_per_minute: Optional[int] = Field(None, description="Maximum requests per minute") | ||
| supports_multi_turn: bool = Field(True, description="Whether the target supports multi-turn conversation history") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we intentionally keep this top-level instance of supports_multi_turn or should this one be removed in favor of the one that will be captured in the nested capabilities field? |
||
| capabilities: Optional[TargetCapabilitiesInfo] = Field(None, description="Structured capability flags") | ||
| target_specific_params: Optional[dict[str, Any]] = Field(None, description="Additional target-specific parameters") | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment on 191, should this also be
toBeGreaterThanOrEqual(9)/toBe(9)? Also, does the comment above miss the additional dash for params?