-
Notifications
You must be signed in to change notification settings - Fork 138
Add filter toolbar for hardware configuration table #1725
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?
Add filter toolbar for hardware configuration table #1725
Conversation
Signed-off-by: manaswinidas <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
52eb607
to
9e3b315
Compare
Signed-off-by: manaswinidas <[email protected]>
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.
Great progress, we just need to adjust a bit so we're using the central filter state object driven by keys that we'll continue using when we eventually bring these same filter components over to the landing page.
WORKLOAD_TYPE = 'workload_type', | ||
HARDWARE_TYPE = 'hardware', |
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.
According to the ADR (see "Performance Metric Properties" these two aren't real filters - well, workload_type isn't and hardware is a string filter.
I think what we need here is keys based on the properties we're filtering by which isn't going to match exactly 1:1 with the way these ones are presented in the UI. So for the workload type filter, really we are hard-coding those 4 options and filtering on the numbers in them (maxInputTokens and maxOutputTokens), and we'll need to translate the selected dropdown option into logic that filters by both of those. The max latency filter is a little more complex - we'll need to use the combination of the user's selected Metric and Percentile to determine which number we're filtering by (e.g. if they selected E2E and P90, we're filtering by e2e_p90
, and we'll need to use that key to pull out the min/max numbers for the slider from the filter options from the API (edit: see my comment below this review - we can't get min/max from the API for these yet). When the user changes that selection we need to clear the old filter and move its new value to the new filter key - and in the process make sure it's always between the new min and max - if the min/max of the new filter key are different and the current value is out of range change it to the new min or max (nearest). So we'll need our types and mocks etc to have all those permutations of metric and percentile added as separate keys 😅
One thing I want to avoid is having to sync state between "what dropdown item is selected" and "what filters are present in the filter values lifted into context". I wonder if we can entirely use the filter values in context as the source of truth. i.e. instead of state for which dropdown item is selected, we have a function that determines which one is selected based on what we know about the filter values that will match - if the filter state has maxInputTokens: 2048 and maxOutputTokens: 256, we know the UI should show the "Summarization" workload type as selected. Conversely, when the user clicks to select a different workload type we can just update those two numbers and that'll cause the dropdown to update. So for each dropdown we'd need utilities to convert a partial filter state object to a dropdown item and vice versa (or in the case of max latency, converting a partial filter state object to both the metric and percentile dropdown items, and converting back from the pair of those to the filter key we want to use). If this is too much of a pain we could sync the two with a useEffect but that feels like asking for trouble. We'll need fallback behavior for weird states (what happens if there are filters on two of the max latency filter properties? That's invalid and shouldn't be possible to encounter from our dropdowns, so maybe we log a warning and just default to the first one present or something. Good unit tests for those will be good.
Also it looks like when filtering, we'll have rpsMean
and ttftMean
rather than the snake_case versions... seems inconsistent with the properties on the objects, I'll start a thread to ask the MR team...
) => void, | ||
): void => { | ||
// Clear hardware filters | ||
setFilterData(ModelCatalogStringFilterKey.PROVIDER, []); |
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.
Provider isn't one of the hardware filters, that's one of the model filters e.g. Meta or IBM.
// Clear number filters | ||
setFilterData(ModelCatalogNumberFilterKey.MIN_RPS, undefined); | ||
setFilterData(ModelCatalogNumberFilterKey.MAX_LATENCY, undefined); | ||
setFilterData(ModelCatalogNumberFilterKey.WORKLOAD_TYPE, undefined); |
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.
I think maybe we can just loop over the Object.values(ModelCatalogNumberFilterKey)
to reset them all.
export const useHardwareTypeFilterState = (): { | ||
appliedHardwareTypes: string[]; | ||
setAppliedHardwareTypes: React.Dispatch<React.SetStateAction<string[]>>; | ||
clearHardwareFilters: () => void; | ||
} => { | ||
const [appliedHardwareTypes, setAppliedHardwareTypes] = React.useState<string[]>([]); | ||
|
||
return { | ||
appliedHardwareTypes, | ||
setAppliedHardwareTypes, | ||
clearHardwareFilters: () => setAppliedHardwareTypes([]), | ||
}; |
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.
Ideally we can also drive this from the central filter state object. You could just use useCatalogStringFilterState(ModelCatalogStringFilterKey.hardware)
.
<ToolbarItem | ||
style={{ borderLeft: '1px solid #d2d2d2', paddingLeft: '16px', marginLeft: '16px' }} | ||
> |
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.
Can we try to avoid inline styles? We should be able to use <ToolbarItem variant="separator" />
here. Toolbar docs
<Button | ||
variant="plain" | ||
aria-label="More info for workload type" | ||
onClick={(e) => e.stopPropagation()} | ||
> | ||
<HelpIcon /> | ||
</Button> |
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.
I think we want to use the icon
prop instead of passing the icon as children. That should give a color change hover effect on the icon. Same for the other two help popovers in the latency/RPS dropdowns too.
[ModelCatalogNumberFilterKey.WORKLOAD_TYPE]: { | ||
type: 'number', | ||
range: { | ||
min: 0, | ||
max: 10, | ||
}, | ||
}, | ||
[ModelCatalogNumberFilterKey.HARDWARE_TYPE]: { | ||
type: 'number', | ||
range: { | ||
min: 0, | ||
max: 10, | ||
}, | ||
}, |
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.
yeah it doesn't make sense for these to be number filters
const validFields = [ | ||
'ttft_mean', | ||
'ttft_p90', | ||
'ttft_p95', | ||
'ttft_p99', | ||
'e2e_mean', | ||
'e2e_p90', | ||
'e2e_p95', | ||
'e2e_p99', | ||
'tps_mean', | ||
'tps_p90', | ||
'tps_p95', | ||
'tps_p99', | ||
'itl_mean', | ||
'itl_p90', | ||
'itl_p95', | ||
'itl_p99', | ||
]; |
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.
We can derive these values from the ModelCatalogNumberFilterKey enum once we add them there
artifacts: CatalogPerformanceMetricsArtifact[], | ||
): string[] => { | ||
const hardwareTypes = artifacts | ||
.map((artifact) => getStringValue(artifact.customProperties, 'hardware')) |
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.
we can put hardware in as a ModelCatalogStringFilterKey and use that instead of hard coding it
artifacts.filter((artifact) => { | ||
// Hardware Type Filter (using dedicated hardware filter state) | ||
if (appliedHardwareTypes && appliedHardwareTypes.length > 0) { | ||
const hardwareType = getStringValue(artifact.customProperties, 'hardware'); |
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.
same here for the hardware string
@manaswinidas - from this slack thread it looks like:
|
Description
Added Workload type, Latency, RPS and Hardware type filters for hardware configuration table.
Note: Hardware type and RPS filters are working - investigating the other filters(Workload type and Max latency are hardcoded as of now and the filters don't work)
Chip toolbar hasn't been added but the Clear all filters functionality has been added
Need to add unit tests
Screen.Recording.2025-10-09.at.8.56.01.PM.mov
How Has This Been Tested?
Go to Performance insights of first model of Model catalog and play around with the filters
Merge criteria:
DCO
check)ok-to-test
has been added to the PR.If you have UI changes