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 HTML repr to scheduler_info and incorporate into client and cluster reprs #4857

Merged
merged 17 commits into from
Jun 15, 2021

Conversation

jacobtomlinson
Copy link
Member

Scheduler info

This PR builds upon #4853 and adds an HTML repr to Client.scheduler_info() and cluster.scheduler_info.

Everything is drawn with basic HTML and some inline CSS styling. Most of the heavy lifting is being done by the Jupyter CSS.

I've made heavy use of f-strings throughout but perhaps switching to jinja for HTML reprs may be a better choice in the long run as it will hugely simplify code and duplication. Plus allow for partial templating and reuse. But that would add jinja2 as a dependency so may be better to have a separate discussion about that.

Before

image

After

image

Client

I've then expanded the Client HTML repr to add a little info about the client and then reuse the scheduler_info repr.

The client info shows how the client was connected, either with a cluster object, a scheduler file or directly with an address. It also shows more information about the cluster object and the path to the scheduler file if applicable.

Before

image

After

image

Cluster

Lastly I've updated the cluster HTML repr and widget. The repr got the same treatment as Client and adds a little cluster specific info and then reuses scheduler_info.

The Cluster class adds minimal cluster info but adds a kwarg to _repr_html_ so that classes lower down can add their own info. I imagine for KubeCluster we may add some Kubernetes specific info, for EC2Cluster some AWS specific info, etc.

For the ipywidget I've added a tabs view where the first tab reuses the HTML repr and the second tab contains the scaling controls which existed before. This should bring some consistency to folks with/without ipywidgets installed.

Before (without ipywidgets)

image

After (without ipywidgets)

image

Before (with ipywidgets)

image

After (with ipywidgets)

image
image

@jacobtomlinson jacobtomlinson requested a review from ian-r-rose May 27, 2021 10:31
@fjetter
Copy link
Member

fjetter commented May 27, 2021

That's amazing

@jacobtomlinson
Copy link
Member Author

It should all handle dark mode too.

image

@mrocklin
Copy link
Member

I think that this is great. Thank you @jacobtomlinson . Two questions:

  1. Does this break anything downstream? Presumably things that used to be strings are now more complex objects.

    We probably can't know this for sure, and I think that it's worth the breakage, but I just wanted this concern voiced before we smash the merge button.

  2. What else can we do here? While we have you on a roll with HTML reprs are there other things we should consider augmenting?

@jacobtomlinson
Copy link
Member Author

jacobtomlinson commented May 27, 2021

@mrocklin

  1. The scheduler_info dict objects are now derived dict objects with an HTML repr. So they still behave as dicts. All the other changes are to HTML reprs. My hope here is that not too much gets broken downstream.
  2. I've found this a pretty satisfying exercise. It's not particularly difficult but has high visibility and high impact. I'm happy to keep building things like this for other objects from time to time. If folks have suggestions I'm very happy to hear them.

@jacobtomlinson
Copy link
Member Author

jacobtomlinson commented May 27, 2021

My biggest worry here is putting HTML inside strings inside Python. It's not particularly easy to parse with your eyes due to a lack of syntax highlighting.

String manipulation gets messy quickly.

I think a next step might be to refactor all of this out into some jinja2 templates. That would make building more things in the future easier and most importantly more maintainable.

@mrocklin
Copy link
Member

mrocklin commented May 27, 2021 via email

Copy link
Member

@jrbourbeau jrbourbeau 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 all your work here @jacobtomlinson : )

My biggest worry here is putting HTML inside strings inside Python. It's not particularly easy to parse with your eyes due to a lack of syntax highlighting.

Agreed that HTML strings in Python are difficult to parse. Incorporating some kind of templating would be a welcomed improvement, though can probably be handled in a follow-up PR

@jacobtomlinson
Copy link
Member Author

I've nudged things here to support async for the Client and Cluster objects. There was a small issue if you tried to view the repr before awaiting them.

I expect there may be a small conflict when #4865 is merged. But once resolved this should be good to go.

@GenevieveBuckley
Copy link
Contributor

Hey @jacobtomlinson I notice that the light grey text colour fails the contrast accessibility check here on a white background. Can we update that so it's readable? (Potentially we'll need to be different shades of grey for light vs dark mode backgrounds, not super sure about that.)

@mrocklin
Copy link
Member

@jacobtomlinson checking in here. I'd love to see this in. It looks like the who_has/has_what stuff was merged. Is it the right time to gently nudge you on this one? :)

@jacobtomlinson
Copy link
Member Author

Yeah I'll take a look at this today.

@jacobtomlinson
Copy link
Member Author

Failed tests are now passing for me locally so 🤞🏻 this is ready for review.

@jrbourbeau jrbourbeau mentioned this pull request Jun 14, 2021
4 tasks
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @jacobtomlinson! This looks great : )

distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
@jacobtomlinson
Copy link
Member Author

Thanks @jrbourbeau for the test fixes and the review. Provided CI passes this should be good to merge.

@jacobtomlinson
Copy link
Member Author

Test failure seems flaky, rerunning.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @jacobtomlinson!

(test failures are unrelated)

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.

5 participants