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

Refactor dashboard code #3138

Merged
merged 14 commits into from Oct 15, 2019
Merged

Refactor dashboard code #3138

merged 14 commits into from Oct 15, 2019

Conversation

jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Oct 11, 2019

Closes #3048.

In an attempt to tidy up the dashboard code I've done some major shuffling of things within the dashboard submodule. I think overall this puts things in more obvious places and should make it more accessible to new contributors.

Notable changes:

  • Components have been broken out in a new distributed.dashboard.components submodule and placed into logical groupings "scheduler specific", "worker specific", "shared" and "nvml/gpu". This could be broken down further but there is a balance to be struck between indirection and giant scary files.
  • The additional server routes from scheduler_html.py and worker_html.py have been moved into the scheduler.py and worker.py files to keep all server things together.
  • Shared functions and utilities have been moved around into more appropriate places.

Other things I'd like to do:

  • Make use of the update source function outside of the scheduler components
  • Re-order components to group _doc functions with their DashboardComponent classes to reduce indirection (or at least distance). On second thoughts as some docs use multiple components having the docs together at the bottom actually makes more sense.
  • Rename functions for consistency (there are a few which should probably follow the _doc names)
  • Identify and remove unused components. (Could use some pointers on this)

Copy link
Member

@mrocklin mrocklin left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for doing this.

A few minor comments. I'm mostly criticizing my old code. My apologies for directing these comments at you :)

update(self.source, result)


class CurrentLoad(DashboardComponent):
Copy link
Member

Choose a reason for hiding this comment

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

This should maybe be broken out into one class per plot? I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. The nvml stuff also seems to follow this pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've had a go at pulling this apart into two classes and ended up with quite a bit of duplication. I'm tempted to say that while this class is quite big and complex it does make sense to gather these metrics in one go.

An alternative would be to make a common base class which deals with the data updating and then subclasses which deal with plotting the parts they want. However, that introduces quite a lot of indirection.

What do you think @mrocklin?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine to me

distributed/dashboard/components/scheduler.py Outdated Show resolved Hide resolved
distributed/dashboard/components/scheduler.py Outdated Show resolved Hide resolved
distributed/dashboard/components/scheduler.py Show resolved Hide resolved
@jacobtomlinson
Copy link
Member Author

This should be ready for final review and potential merging now.

@mrocklin
Copy link
Member

Do we need to include distributed.dashboard.components in setup.py ? (I honestly don't fully understand how package building works)

@jacobtomlinson
Copy link
Member Author

Hmm possibly, I've only ever used find_packages to specify the packages kwarg in the setup. Perhaps someone with better knowledge like @jakirkham could weigh in here?

@mrocklin
Copy link
Member

@jcrist can you take a look at this?

@jcrist
Copy link
Member

jcrist commented Oct 15, 2019

Yeah, you need to add distributed.dashboard.components to packages. packages needs to be a list of all directories in the library. We could swap out a static list for find_packages which generally does a good job of doing this automatically.

@mrocklin
Copy link
Member

#3150

@mrocklin mrocklin merged commit c970ec0 into dask:master Oct 15, 2019
@mrocklin
Copy link
Member

Thanks for this @jacobtomlinson !

@jacobtomlinson jacobtomlinson deleted the 3048-dashboard branch October 16, 2019 07:59
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.

Cleanup dashboard code
3 participants