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 widgets for logs #142

Merged
merged 1 commit into from
May 17, 2019
Merged

Add widgets for logs #142

merged 1 commit into from
May 17, 2019

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.

None yet

3 participants