Skip to content
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

Add GPU info to new dashboard #27074

Merged
merged 16 commits into from
Aug 2, 2022
Merged

Add GPU info to new dashboard #27074

merged 16 commits into from
Aug 2, 2022

Conversation

alanwguo
Copy link
Contributor

@alanwguo alanwguo commented Jul 27, 2022

Have first node be default expanded

Signed-off-by: Alan Guo [email protected]

Why are these changes needed?

Screen Shot 2022-07-27 at 1 54 13 PM

Related issue number

fixes #13889

Addresses comment from #26996

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Have first node be default expanded

Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
@pcmoritz
Copy link
Contributor

This is great :)

@rkooo567
Copy link
Contributor

@alanwguo is it possible to assign someone else as well for reviewing the frontend code? I can review briefly, but I am not confident I can do a quality review. (only superficial one is possible from my end)

Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High level feedback -> should we have the same type of bar like other columns than [index] [used] / [total]MB format?

Screen Shot 2022-07-27 at 7 32 46 AM

alanwguo added 10 commits July 27, 2022 13:28
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
Signed-off-by: Alan Guo <[email protected]>
node: NodeDetail;
}) => {
const workerGRAMEntries = (node.gpus ?? [])
.map((gpu, i) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use reduce() to handle workerGRAMEntries instead of map().filter()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for the suggestion? I find map + filter to be easier to understand (better for code maintainability) than reduce functions.

@alanwguo
Copy link
Contributor Author

pick for 2.0.0 but not an rc0 blocker

@rkooo567
Copy link
Contributor

One question: What's the reason why GPU is N/A when there's GRAM available?

Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for a high level approach. Didn't do detailed review in code

Only show N/A if gpu utilization is undefined

Signed-off-by: Alan Guo <[email protected]>
@alanwguo
Copy link
Contributor Author

alanwguo commented Aug 1, 2022

One question: What's the reason why GPU is N/A when there's GRAM available?

That was a bad screenshot. This is the the normal case:
Screen Shot 2022-07-31 at 5 21 16 PM

@rkooo567
Copy link
Contributor

rkooo567 commented Aug 1, 2022

@alanwguo do you think I can merge it without further frontend review? (should we ask someone like Shawn?)

@alanwguo
Copy link
Contributor Author

alanwguo commented Aug 1, 2022

@alanwguo do you think I can merge it without further frontend review? (should we ask someone like Shawn?)

shawn is reviewing

Copy link
Contributor

@shawnpanda shawnpanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would have liked to see some tests but I guess that there isn't test setup rn

export const NodeGPUView = ({ node }: { node: NodeDetail }) => {
return (
<div style={{ minWidth: GPU_COL_WIDTH }}>
{node.gpus !== undefined && node.gpus.length !== 0 ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could do node.gpus?.length instead of node.gpus !== undefined && node.gpus.length !== 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the typescript compiler isn't smart enough to realize node.gpus is not null in the next line when node.gpus.length is not null.

Signed-off-by: Alan Guo <[email protected]>
@rkooo567
Copy link
Contributor

rkooo567 commented Aug 2, 2022

Lmk when it is ready to merge!

@alanwguo
Copy link
Contributor Author

alanwguo commented Aug 2, 2022

@rkooo567 please merge anytime

@rkooo567 rkooo567 merged commit c083ca5 into master Aug 2, 2022
@rkooo567 rkooo567 deleted the aguo/fix-small-dashboard-bugs branch August 2, 2022 22:32
alanwguo added a commit that referenced this pull request Aug 2, 2022
Support a GPU column for the new dashboard

Have first node be default expanded

Signed-off-by: Alan Guo [email protected]

fixes #13889

Addresses comment from #26996
@alanwguo alanwguo mentioned this pull request Aug 2, 2022
7 tasks
scv119 pushed a commit that referenced this pull request Aug 3, 2022
Support a GPU column for the new dashboard

Have first node be default expanded

Signed-off-by: Alan Guo [email protected]

fixes #13889

Addresses comment from #26996
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
Support a GPU column for the new dashboard

Have first node be default expanded

Signed-off-by: Alan Guo [email protected]

fixes ray-project#13889

Addresses comment from ray-project#26996

Signed-off-by: Stefan van der Kleij <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GPU/GRAM usage for workers and actors and nodes
7 participants