Implement the Hardware Configuration table to sort by ascending order#2194
Implement the Hardware Configuration table to sort by ascending order#2194Taj010 wants to merge 5 commits intokubeflow:mainfrom
Conversation
YuliaKrimerman
left a comment
YuliaKrimerman
left a comment
There was a problem hiding this comment.
Why do we need both sortedData pre-sorting AND defaultSortColumn? Does the Table component not automatically sort on the default column, or is there a specific reason we need to pre-sort the data?
| const sortedData = React.useMemo(() => { | ||
| if (!sortColumn) { | ||
| return performanceArtifacts; | ||
| } | ||
| const sortFn = sortColumn.sortable; | ||
| if (typeof sortFn !== 'function') { | ||
| return performanceArtifacts; | ||
| } | ||
| return [...performanceArtifacts].toSorted((a, b) => sortFn(a, b, sortColumn.field)); | ||
| }, [sortColumn, performanceArtifacts]); |
There was a problem hiding this comment.
The Table component already has sorting built-in via defaultSortColumn. Why are we pre-sorting the data AND setting defaultSortColumn? This feels like we're doing the work twice. Can we just rely on the Table's sorting with defaultSortColumn={sortIndex} and not pre-sort?
mturley
left a comment
mturley
left a comment
There was a problem hiding this comment.
@Taj010 the reason we needed your mod-arch-shared update to lift sort state outside the Table is that we need more than just to set the default, we need to update the sort each time the user changes the latency filter's metric or percentile so it is sorted by the matching column. That requires us to lift the sort state out of the Table so we can update it from elsewhere, which will require installing the new version of mod-arch-shared that includes your new changes to Table to make that possible.
You might need that state to originate in the useHardwareConfigColumns hook. That's where we already have an effect that changes the selected columns in the manage columns modal when the latency filter changes, you can probably piggyback on that same logic, it already parses out the correct latency filter key you need. https://github.com/kubeflow/model-registry/blob/main/clients/ui/frontend/src/app/pages/modelCatalog/components/useHardwareConfigColumns.ts#L115
|
[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 |
| // Add the matching TPS column (if not already present) | ||
| if (!newVisibleIds.includes(matchingTpsPropertyKey)) { | ||
| newVisibleIds.push(matchingTpsPropertyKey); | ||
| // Ensure sort is set correctly when columns are ready (handles initial mount case) |
There was a problem hiding this comment.
I ended up using two useEffect hooks to keep the sorting in sync. The first one handles the immediate visibility change when a filter is picked, and the second one acts as a 'safety check' to make sure the sortIndex actually points to the right column after the table re-renders.
I tried combining them, but it kept breaking the sort during transitions. This is the best I could come up with, but let me know if you think there's a cleaner way to handle the state dependency
Signed-off-by: Taj010 <rsheen003@gmail.com>
Signed-off-by: Taj010 <rsheen003@gmail.com>
Signed-off-by: Taj010 <rsheen003@gmail.com>
Taj010
force-pushed
the
hardware-table-sort
branch
from
9ee0699 to
36d5591
Compare
3 participants
Description
Fixed the current default sorting in Performance Insights. The default order of records in the Hardware Configuration table is now sorted in ASC order by the selected Metric & Percentile.
How Has This Been Tested?
This has been manually tested by checking if the selected metric values are sorted by ascending order.
Merge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes