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 GPUCurrentLoad dashboard plots #2944

Merged
merged 1 commit into from Aug 12, 2019

Conversation

@mrocklin
Copy link
Member

commented Aug 9, 2019

A first step towards fixing rapidsai/dask-cuda#36

@pentschev

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

A bit of an unrelated question, but I thought you wanted to keep GPU-specific code mostly out of dask/distributed. This is just for my own understanding, what's the current policy on whether GPU code should go in here rather than dask-cuda?

@mrocklin

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

It's a good question. I could imagine moving this around later in the future.

There was some non-trivial amount of work to make the dashboarding general enough to take information like this. I was also able to find a way to keep things nicely separate. I also have decent confidence that this code won't need much experimentation in the future, and will be low churn.

@mrocklin

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

For example, we defined cuda_serialize in distributed because the code changes needed for it are mostly the same as the code changes needed for dask_serialize. The infrastructure is there. Things like the dask-cuda-worker code is changing rapidly enough that I'd prefer to keep it external for a while. I could imagine moving it in once it's more stable.

@pentschev

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Ok, makes sense. Thanks for the clarifications @mrocklin .

Now going back to this PR, would you like me to test it out and review it?

@mrocklin

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Review would be welcome

@pentschev

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

It looks good to me, I added a couple of minor comments. I don't have experience with bokeh, superficially it looks fine for me, but maybe someone else with bokeh experience could take a look at that, otherwise, good to be merged by me.

@pentschev

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

LGTM, looking forward to see this being used by more people!

@mrocklin mrocklin merged commit b27f7b7 into dask:master Aug 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mrocklin mrocklin deleted the mrocklin:gpu-dashbaord branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.