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

Visualize task graphs using ipycytoscape #9091

Merged
merged 14 commits into from May 24, 2022
Merged

Conversation

ian-r-rose
Copy link
Collaborator

@ian-r-rose ian-r-rose commented May 15, 2022

This adds a new graph visualization engine based on ipycytoscape. Right now it's pretty much a re-implementation of the graphviz visualizer, but with zooming and panning. However, there are a few things that this could be more useful for in the near future.

  1. The ability to run in pyodide without compiling graphviz to WASM (cf. Dask in pyodide #9053)
  2. More interactivity, especially with different coloring schemes and layouts.
  3. The possibility to add richer tooltips/popups (cf. Clearer colourmap / labels when showing task order in visualize #9069)

cytoscape

@ian-r-rose ian-r-rose self-assigned this May 16, 2022
@ian-r-rose ian-r-rose added core feature Something is missing labels May 16, 2022
@jrbourbeau jrbourbeau self-requested a review May 17, 2022 16:15
@jakirkham
Copy link
Member

This is really cool! 🤩 Thanks for working on this Ian 🙏

dask/dot.py Outdated
@@ -306,3 +308,237 @@ def graphviz_to_file(g, filename, format):
f.write(data)

return display_cls(filename=full_filename)


def to_cytoscape_json(
Copy link
Member

Choose a reason for hiding this comment

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

Given dot is the CLI for GraphViz, wonder if we should put this in a different module for organizational clarity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I waffled on whether to create a new module for this, but decided to keep them in the same module since they share some of the naming utilities.

What about changing this to dask/visualize.py (with perhaps a dot.py backwards compatibility shim)?

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 @ian-r-rose! This is cool to see. I'm not very familiar with the interactive-Jupyter-graph-visualization space -- are there other packages we should be considering?

Another benefit I of adding a cytoscape engine is it helps avoid some of the pain around installing the system graphviz package

dask/base.py Outdated Show resolved Hide resolved
dask/base.py Show resolved Hide resolved
dask/base.py Outdated Show resolved Hide resolved
dask/dot.py Show resolved Hide resolved
dask/dot.py Show resolved Hide resolved
dask/dot.py Outdated Show resolved Hide resolved
@ian-r-rose
Copy link
Collaborator Author

ian-r-rose commented May 17, 2022

are there other packages we should be considering?

Perhaps there are. I chose cytoscape for this primarily because (1) it is a well-supported JS library, (2) python bindings for it already exist through ipycytoscape, and (3) it already works in pyodide. But I wouldn't be averse to investigating others if they had additional benefits.

There is some discussion of alternatives in #7301, but cytoscape still seems like a reasonably safe choice.

@jakirkham
Copy link
Member

jakirkham commented May 17, 2022

The only alternative that stood out to me from reading that issue was ipyelk. Did we evaluate that one? If so, what conclusions did we draw?

Martin also mentioned a Google doc with other alternatives, but the link appears to be broken. Asked if there was a more up-to-date link

@ian-r-rose
Copy link
Collaborator Author

The only alternative that stood out to me from reading that issue was ipyelk. Did we evaluate that one? If so, what conclusions did we draw?

I didn't really evaluate it -- I looked at the screenshots and they looked less like the final result I wanted than the cytoscape ones.

@jakirkham
Copy link
Member

jakirkham commented May 17, 2022

That was my impression as well, but didn't know if that was just a perception gotten from those screenshots or if that is really representative of all ipyelk visualizations

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This is awesome. I love the implementation because it doesn't close us off from alternatives in the future. I also like that the default is configurable, really neat!

The only thing I would say this PR needs is some docs.

@github-actions github-actions bot added the documentation Improve or add to documentation label May 19, 2022
@ian-r-rose
Copy link
Collaborator Author

The only thing I would say this PR needs is some docs.

Good point, thanks @jacobtomlinson. I've added some docs.

@ian-r-rose
Copy link
Collaborator Author

That was my impression as well, but didn't know if that was just a perception gotten from those screenshots or if that is really representative of all ipyelk visualizations

I took a look at ipyelk for a few minutes and had a bit of a hard time getting it working (it has also not seen much activity recently). I feel comfortable moving forward with cytoscape for the time being. I also think that the plumbing work done to make the visualization engine configurable should make adding new visualizers easier in the future.

dask/base.py Outdated Show resolved Hide resolved
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.

One last comment -- apologies for not noticing before

dask/base.py Outdated Show resolved Hide resolved
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 @ian-r-rose

@jrbourbeau jrbourbeau merged commit 0e15e12 into dask:main May 24, 2022
@freyam
Copy link
Contributor

freyam commented May 25, 2022

Wow, this is incredibly awesome! @ian-r-rose 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation Improve or add to documentation feature Something is missing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive alternatives to graphviz for .visualize()
5 participants