Skip to content

Update dashboard documentation for memory plot, fix #9433#9768

Merged
ncclementi merged 6 commits intodask:mainfrom
jayeshmanani:main
Jan 19, 2023
Merged

Update dashboard documentation for memory plot, fix #9433#9768
ncclementi merged 6 commits intodask:mainfrom
jayeshmanani:main

Conversation

@jayeshmanani
Copy link
Copy Markdown
Contributor

@jayeshmanani jayeshmanani commented Dec 16, 2022

@GPUtester
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@github-actions github-actions bot added the documentation Improve or add to documentation label Dec 16, 2022
@ncclementi
Copy link
Copy Markdown
Member

ok to test.

Copy link
Copy Markdown
Member

@ncclementi ncclementi left a comment

Choose a reason for hiding this comment

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

@jayeshmanani Thank you for this PR and apologies it took a while to get to it.

The GIF looks great, I left a couple of comments to review.

I think we can work on the descriptions, having two columns that say almost the same is a bit repetitive, and I think is causing some indentations issues.

I also pinged the person that worked on this change to confirm what the colors mean, because I think there is a misunderstanding regarding the percentages. I think when you say 50% is actually 70%.

@jayeshmanani
Copy link
Copy Markdown
Contributor Author

I have updated few things in the documentation files, can you check again, and see if I can improve it more @ncclementi.

@ncclementi
Copy link
Copy Markdown
Member

ncclementi commented Jan 18, 2023

@jayeshmanani I think this looks great. I'd like to get a confirmation from @crusaderky or @jrbourbeau to make sure the description of the plot is accurate. Link to render docs here

@charlesbluca I see a gpuCI failure can you confirm this is unrelated to this PR? It says "Aborted by user anonymous"

@charlesbluca
Copy link
Copy Markdown
Member

Looks like the build timed out and got marked as aborted from this log:

https://gpuci.gpuopenanalytics.com/job/dask/job/dask/job/prb/job/dask-prb/3198/console

rerun tests

jayeshmanani and others added 3 commits January 19, 2023 20:17
updated the description as suggested

Co-authored-by: crusaderky <crusaderky@gmail.com>
update description as suggested

Co-authored-by: crusaderky <crusaderky@gmail.com>
@jayeshmanani
Copy link
Copy Markdown
Contributor Author

I have updated a few things in the dashboard.rst file as suggested by @crusaderky, can you check again, and see if there is room for improvement @ncclementi

@ncclementi
Copy link
Copy Markdown
Member

@jayeshmanani It looks like you addressed all the comments suggested, I will wait for CI to finish and merge this in!

@ncclementi
Copy link
Copy Markdown
Member

@jayeshmanani I noticed this is your first PR, horay 🎉

Thank you for the contribution, this goes in.

@ncclementi ncclementi merged commit 5a6bbdc into dask:main Jan 19, 2023
@jayeshmanani
Copy link
Copy Markdown
Contributor Author

@jayeshmanani I noticed this is your first PR, horay 🎉

Thank you for the contribution, this goes in.

Yeah, It was @ncclementi , thank you very much. Looking forward to contributing more in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improve or add to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update dashboard documentation for memory plot

5 participants