[ray dashboard] Sort actor table columns by resource utilization #42788
Conversation
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
nikitavemuri
changed the title
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
…ita/sort_actor_cols
| export const sortActors = (actorList: ActorDetail[]) => { | ||
| const sortedActors = [...actorList]; | ||
| return _.sortBy(sortedActors, (actor) => { | ||
| const actorOrder = isActorEnum(actor.state) ? stateOrder[actor.state] : 0; | ||
| const actorTime = actor.startTime || 0; | ||
| return [actorOrder, actorTime]; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
looks like we already have sort by state, but is it not working correctly?
#42666 was reported recently.
There was a problem hiding this comment.
From my testing sort by state for actors seems to be working correctly, I think #42666 is only for the clusters table
There was a problem hiding this comment.
oh whoops. i got that mixed up.
| @@ -99,11 +91,36 @@ const ActorTable = ({ | |||
| const [actorIdFilterValue, setActorIdFilterValue] = useState(filterToActorId); | |||
| const [pageSize, setPageSize] = useState(10); | |||
|
|
|||
| const defaultSorterKey = ""; | |||
There was a problem hiding this comment.
let's extract this out of the component and name it DEFAULT_SORTER_KEY.
Let's also make the default state something like "actor-state" and make it one of the options in teh "Sort by" input so users can go back to that sort.
There was a problem hiding this comment.
What does Sort by "All" mean in your screenshot?
"All" was a menu option added by default to the SearchSelect component to represent whatever the default sorting would be if no keys are specified by the user. This component is also used for the "State" filter on the clusters page where "All" would represent all states. I agree for this sort by input we should just specify one of the keys as the default for clarity.
I'm not sure having a separate sort by state is useful for the actors page given we have a "State" filter to either show alive or dead or all actors. If the state filter isn't set the current behavior of showing "Alive" actors at the top seems reasonable. The user specified sort by key would sort actors within the "Alive" and "Dead" state groups.
Current behavior is to sort by reverse uptime as the default for the "All" option. I think sorting by "Uptime" (newest actors at top) probably makes more sense for many use cases so we can select that as the default key.
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
| // the table by default | ||
| const uptime = | ||
| startTime && startTime > 0 | ||
| ? getDurationVal({ startTime, endTime }) |
There was a problem hiding this comment.
I just realized because duration is very dynamic field (changes every second), the sorting of this list may not update when the durations change.
I think this is probably fine... an edge case most people may not notice. Can you add a comment stating we will only re-sort rows on re-renders.
Signed-off-by: Nikita Vemuri <[email protected]>
Why are these changes needed?
Uptime,Used memory,Total memory,CPU,GPU utilization,GRAM usage). Reverse toggle also availableRelated issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.