Skip to content

Conversation

@jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Apr 30, 2019

Add a couple of new log classes which extend str and dict to add some ipywidgets. This is aimed to make viewing logs easier.

Each log is a string which is rendered as a <code> block in an HTML widget. Then all logs are a dictionary of the log strings wrapped in an accordion view.

Before

image

After

image

Would appreciate some pointers on testing ipywidgets stuff.

@mrocklin
Copy link
Member

That's quite neat!

"""A container for logs."""

def _widget(self):
from ipywidgets import HTML
Copy link
Member

Choose a reason for hiding this comment

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

What happens if ipywidgets isn't installed?

Also, do we need ipywidgets here? Would implementing a _repr_html_ method work instead?

def _repr_html_(self):
    return "<pre><code>... "

I think that this has the advantage of also rendering nicely in static notebooks.

Perhaps that wouldn't work for the accordion though, so maybe this isn't worth it.

Copy link
Member

Choose a reason for hiding this comment

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

What happens if ipywidgets isn't installed?

I don't think obj._widget is called if ipywidgets is not importable.

@mrocklin
Copy link
Member

We might also consider this within the core dask/distributed project and the Client.get_scheduler.logs and Client.get_worker_logs methods.

@mrocklin
Copy link
Member

I think that this is great, and would be happy to see it merged.

@jacobtomlinson given the less-than-active maintenance response on some of these repositories I encourage you to self-merge in the future after a suitable time. I typically say something like "Merging in 24 hours if there are no further comments" and then merge away the next day.

@jhamman jhamman mentioned this pull request May 17, 2019
@jhamman
Copy link
Member

jhamman commented May 17, 2019

@mrocklin - what do you think we should here? I personally think this can be merged as is.

@mrocklin
Copy link
Member

@mrocklin - what do you think we should here? I personally think this can be merged as is.

Same, I say go for it

@mrocklin
Copy link
Member

Also, FWIW, I personally prefer squash-and-merge merging if you're comfortable with that

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.

3 participants