materialsproject / fireworks

The Fireworks Workflow Management Repo.
https://materialsproject.github.io/fireworks
Other
351 stars 184 forks source link

when rendering a workflow with wf_to_graph(), add option to show each firetask #466

Closed janosh closed 2 years ago

janosh commented 2 years ago

Follow up to #437.

janosh commented 2 years ago

Looks like an older test is failing due to missing dependency

    """ A utility to validate and visualize workflows """

    __author__ = "Ivan Kondov"
    __email__ = "ivan.kondov@kit.edu"
    __copyright__ = "Copyright 2017, Karlsruhe Institute of Technology"

    from itertools import combinations
    from typing import Any, Dict

>   import igraph
E   ModuleNotFoundError: No module named 'igraph'

Just pushed what I hope is a fix. Also took the liberty to stop running CI under Python 2.7, now 3.7 only.

This is what the output of wf_to_graph() for wf_hse_bandgap now looks like. Could still be improved with white text on dark colors and sans serif text.

hse-dag

hse-dag.pdf

computron commented 2 years ago

Looks great!

Two notes:

  1. Can you remove the py27 tests in a separate PR?
  2. The igraph test seems to still be failing. Can you either fix it, or if you think the new features remove the need for the old igraph features, to remove the old code?
janosh commented 2 years ago

@computron Good morning. Thanks for merging #464 and #468!

Can you remove the py27 tests in a separate PR?

Good point! Those changes are now in #470.

The igraph test seems to still be failing. Can you either fix it, or if you think the new features remove the need for the old igraph features, to remove the old code?

I did try to use the old code for visualizing workflows first but got weird output: fireworkf_plot_wf fireworkf_plot_wf.pdf

Maybe it could be improved by passing some options but this didn't strike me as very informative default behavior so as far as I can tell the old def plot_wf() in fireworks/utilities/dagflow.py could be dropped.

janosh commented 2 years ago

Just realized there's yet another plotting function plot_fw() already in FWs under fireworks/utilities/fw_utilities.py. So even more reason to get rid of at least 1 of the 3.

https://github.com/materialsproject/fireworks/blob/f6c6a0fbd55799f64364a9e5fb72bbad6a680273/fireworks/utilities/fw_utilities.py#L242-L259

Gives better output IMO than the igraph function and only needs matplotlib which everyone has installed anyway so de facto no extra deps for this one: hse.pdf hse

Since it doesn't plot Firetasks unlike the new graphviz wf_to_graph() function, I'd argue for keeping both wf_to_graph() and plot_wf from fw_utilities.py.

janosh commented 2 years ago

I think it makes sense to move the plotting functions plot_wf() and wf_to_graph() to a new module. Seems a cleaner separation of functionality and also avoids the circular import between fireworks/utilities/fw_utilities.py and fireworks/core/firework.py currently crashing CI. If no objections, question is should fw_utilities.py keep exporting plot_wf() to avoid making this a breaking change? I mean it's already a breaking change since we dropped the old graphviz plot_wf().

ikondov commented 2 years ago

I find a very good idea moving all workflow graph plotting and visualization code to one single place, including all graphviz-based stuff from dagflow.py. I also think there might be a better way to visualize the workflow graph than by igraph and graphviz. But the DAGFlow class using the igraph package that is used for workflow checking should stay in dagflow.py.

janosh commented 2 years ago

But the DAGFlow class using the igraph package that is used for workflow checking should stay in dagflow.py.

Agreed. It's still there. No plans on my end to move it.

computron commented 2 years ago

@janosh let me know if/when the PR is ready and I'll go ahead and merge

janosh commented 2 years ago

@computron Good thing you didn't merge yet. Just noticed I'd accidentally reverted some changes. Here are some examples of what the output from wf_to_graph now looks like. Happy to try my hand at implementing any suggestions for improvement. Of course we could also do that in a separate PR and merge now.

I also think there might be a better way to visualize the workflow graph than by igraph and graphviz.

@ikondov Interesting, I'm starting to think you might have a point. Found the output from graphviz a bit hard to customize and the docs weren't a great help. Were you thinking matplotlib, plotly or something else entirely?

wf_bandstructure_no_opt wf_bandstructure_plus_boltztrap wf_bandstructure_plus_hse wf_bandstructure wf_dielectric_constant