-
Notifications
You must be signed in to change notification settings - Fork 151
Model card performance filters toggle changes and hide filtered latency columns in hardware configuration table #2053
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?
Conversation
…cy columns Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
YuliaKrimerman
left a comment
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 job here ,2 small things
| }, | ||
| filterData, | ||
| filterOptions, | ||
| isValidated, // Only fetch if validated |
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.
Now that you use useCatalogPerformanceArtifacts with performanceViewEnabled I think you should add isValidated && performanceViewEnabled, // Only fetch if validated AND toggle is ON here. This will ensure the req of "The /artifacts fetch per-card can be disabled when the toggle is off" and we will not make an unnecessary API call
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.
But we need the "View x benchmarks" hyperlink leading to performance insights tab of catalog details page even when the toggle is off for validated models
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.
aha.. that's a good point, I was wrong to specify that we can avoid this fetch if the toggle is off I guess
clients/ui/frontend/src/app/pages/modelCatalog/components/ModelCatalogCardBody.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
mturley
left a comment
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.
Small question and small nit inline.
@manaswinidas we'll need to make sure we follow up after the filters are implemented on the landing page to make these cards change which latency property is being shown based on the user's latency filter selection. Technically you already could do that here if you want since that filter can be set on the details page and is available in context when rendering the cards, but it may make sense to make it part of the scope of implementing the filters. Just don't want to lose track of that detail since it was scoped as part of this story.
| }, | ||
| "ttft_mean": { | ||
| MetadataDoubleValue: &openapi.MetadataDoubleValue{ | ||
| DoubleValue: 35.48818160947744, |
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.
Why change these values? This is what we get from the API (I believe) and we have logic in the frontend to round it to 2 decimal places, we want to make sure that still works.
| const percentileSuffix = activeLatencyField.split('_').pop(); | ||
| // Build the matching TPS field name (e.g., 'tps_p90', 'tps_mean') | ||
| const matchingTpsField = `${tpsPrefix}_${percentileSuffix}`; |
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.
You should be able to use the parseLatencyFieldName and getLatencyFieldName utils here instead of splitting and joining with _ directly.
Description
Screen.Recording.2025-12-30.at.7.38.43.PM.mov
How Has This Been Tested?
Merge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes