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

Nicer rule graph viz #7024

Merged

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Jan 4, 2019

Problem

When looking at @benjyw's comment thread with @stuhood on #6993, it seemed like it was difficult to create effective screenshots of the rule graph. This seemed to be for two reasons:

  1. The nodes of the drawn graph are exceedingly wide due to the text being written in a single line.
  2. The context of each node isn't clear (root nodes are outlined in blue, which is hard to make out compared to black against a white background, especially as the HSV coloration that dot outputs or that the OSX Preview app displays seems to be a bit off compared to other HSV selectors).

Solution

  • Insert some newlines in the formatting of nodes in the rule graph to make it skinnier.
  • Color many different types of nodes by printing the node name, then its attributes, before printing the node again within a pair of { ... } in the .dot file.

TODO

  • Use Display impls as noted by @stuhood below, and/or make clear the difference between string representations for diagnostics and errors vs strings for graph drawing.
  • Color intrinsics the way we now do with Params, Singletons, and root nodes. This would require modifying entry_node_str_with_attrs() in much the same way as is done with entry_str() and entry_node_str_with_attrs() here.
  • Remove the stub __str__() implementations which currently have ???s.
  • Consider using more formatting attributes, such as height, width, fixed-size -- see dot(1).
  • (visualize Gets as edges in the rule graph #9016) Weight edges with Gets, as described in Nicer rule graph viz #7024 (comment).

Result

graph-thin
graph-wider
graph-thicc

@baroquebobcat
Copy link
Contributor

This looks a lot more clear.

@cosmicexplorer cosmicexplorer changed the title Nicer rule graph viz [WIP] Nicer rule graph viz Jan 4, 2019
@benjyw
Copy link
Contributor

benjyw commented Jan 4, 2019

The image looks great! Not for this change perhaps, but it would be great to generate an interactive, navigable visualization in HTML, so you can expand and collapse parts of the DAG etc. @TansyArron has been playing with such things in another context.

@cosmicexplorer
Copy link
Contributor Author

I somehow missed @benjyw's comment until just now, and have absolutely been waiting to inject the web into this. It's my impression that graph drawing in things like d3.js and friends has figured out a lot of the things we might be doing here, so I'm going to spend a bit of time looking into how that might be done, which might allow us to supersede this as well.

@Eric-Arellano
Copy link
Contributor

Hey @cosmicexplorer, could we try to revive this one please? I see there are a couple more improvements you wanted to make first, but I'd encourage us to merge what you have now (after rebase). Even though there are further ways to go, these changes could be making a meaningful impact today, and we can always have followup PRs.

@cosmicexplorer
Copy link
Contributor Author

This should be ready for review assuming it passes CI!

Copy link
Contributor

@pierrechevalier83 pierrechevalier83 left a comment

Choose a reason for hiding this comment

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

Very happy with this change. It's gonna be a nice usability win for us when dealing with dot representations of rule graphs. Totally agree with @Eric-Arellano that this is worth merging as-is even if we're considering doing something fancier later.

left the same comment in a few places with a suggestion to enhance readibility by naming colors, but looks good to me overall.

entry_with_deps_str(e),
// Color "singleton" entries (with no params) as a nice olive green!
if e.params().is_empty() {
Some("[color=\"0.2214,0.7179,0.8528\",style=filled]".to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

Would need to test first, but I think this could be replaced with "olive" as graphviz seems to support the svg color scheme: https://www.graphviz.org/doc/info/colors.html.
This would improve readability imo compared to float based rgb. Alternatively, if you want something very specific, I find hex representation to be a bit more readable.

From reading graphviz's doc (https://www.graphviz.org/pdf/dotguide.pdf), I see that these three floating points represent hue, saturation and brightness; but that seems pretty unusual to me as the hue is generally an int form 0-360 degrees.

Pro-tip for finding what a color looks like from its name from the command line (assuming your terminal supports 256-bit colors, but of course it does, amirite? 😄 )

cargo install pastel
pastel color olive

Copy link
Contributor

Choose a reason for hiding this comment

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

If this doesn't work out, storing the colors as constants like let olive = "0.576,0,0.6242" would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a Palette enum to support named colors. I kept the use of HSV for selecting these particular colors since I liked these colors specifically (in an attempt to compromise between brightness and not blocking out the graph node text), but I've left a comment next to the method that produces the color string for each Palette instance that any other color scheme can also be used to specify colors. I've added a link to https://www.graphviz.org/doc/info/colors.html in a comment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and thank you for all the context!!!!!)

src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay! Thank you so much for picking this back up!

Pardon that the rule graph errors PR causes some serious merge conflicts, here.

--

As discussed over DM, I do not think we want to change any of the output for the rule graph errors, i.e. only change the graph visualization.

For colors, it's tricky to get them right on a terminal where we cannot assume the background, unlike our assumption that the graph has a white background. Also, background colors are generally frowned upon with terminals afaict.

For new lines, this is also not ideal imo because we cannot assume the size of the terminal so it's unclear where we should wrap and where we shouldn't. Python will already soft wrap the rule graph errors for us, which I believe is the correct behavior.

src/python/pants/build_graph/build_configuration.py Outdated Show resolved Hide resolved
src/python/pants/engine/mapper.py Outdated Show resolved Hide resolved
src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
entry_with_deps_str(e),
// Color "singleton" entries (with no params) as a nice olive green!
if e.params().is_empty() {
Some("[color=\"0.2214,0.7179,0.8528\",style=filled]".to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

If this doesn't work out, storing the colors as constants like let olive = "0.576,0,0.6242" would be great.

src/rust/engine/src/tasks.rs Show resolved Hide resolved
tests/python/pants_test/engine/test_rules.py Outdated Show resolved Hide resolved
@stuhood
Copy link
Member

stuhood commented Jan 24, 2020

One other thing is this comment: #7509 (comment) ... I think that it would significantly clarify the visualization to label the edges with Get/$Type, with the added bonus that it would shrink each node in the visualization.

@Eric-Arellano
Copy link
Contributor

One other thing is this comment: #7509 (comment) ... I think that it would significantly clarify the visualization to label the edges with Get/$Type, with the added bonus that it would shrink each node in the visualization.

This would be great! However, please continue to render gets in rule graph errors and only make this change for the graph visualization. I intentionally deviated from #7509 to still include gets in errors because I've found it very helpful when debugging.

@cosmicexplorer cosmicexplorer force-pushed the nicer-rule-graph-viz branch 2 times, most recently from b903e47 to 8b940b2 Compare January 24, 2020 23:02
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you for updating this to not change rule graph errors!

@cosmicexplorer cosmicexplorer force-pushed the nicer-rule-graph-viz branch 4 times, most recently from 6bb1abe to 9ed3301 Compare January 25, 2020 09:35
@cosmicexplorer
Copy link
Contributor Author

This should be ready for review once it passes CI.

@stuhood
Copy link
Member

stuhood commented Jan 25, 2020

This should be ready for review once it passes CI.

Would you mind updating the screenshots in the description? Thank you!

@cosmicexplorer
Copy link
Contributor Author

Would you mind updating the screenshots in the description? Thank you!

Done!

One other thing is this comment: #7509 (comment) ... I think that it would significantly clarify the visualization to label the edges with Get/$Type, with the added bonus that it would shrink each node in the visualization.

I think this is a really good idea! I will see how easy it is to produce edge labels as well -- in a followup if not in this PR!

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks.

src/rust/engine/rule_graph/src/rules.rs Outdated Show resolved Hide resolved
tests/python/pants_test/engine/test_rules.py Outdated Show resolved Hide resolved
@cosmicexplorer
Copy link
Contributor Author

Created #9016 to track the followup work of turning Gets into labelled edges, as @stuhood describes above!

I'll wait for a few more people to sign off before merging.

src/python/pants/testutil/engine/util.py Outdated Show resolved Hide resolved
src/python/pants/testutil/engine/util.py Outdated Show resolved Hide resolved
src/python/pants/testutil/engine/util.py Outdated Show resolved Hide resolved
src/python/pants/testutil/engine/util.py Outdated Show resolved Hide resolved
src/python/pants/testutil/engine/util.py Outdated Show resolved Hide resolved
src/rust/engine/src/tasks.rs Outdated Show resolved Hide resolved
src/rust/engine/src/tasks.rs Outdated Show resolved Hide resolved
src/rust/engine/src/tasks.rs Outdated Show resolved Hide resolved
tests/python/pants_test/engine/test_rules.py Outdated Show resolved Hide resolved
tests/python/pants_test/engine/test_rules.py Outdated Show resolved Hide resolved
fill in the color of root nodes to make them more noticeable!

colorize non-inner nodes

fix coloration and add some dummy __str__()s for singletons

fix rule graph display tests

remove last ??? string!

clean up

add special coloring for intrinsics!

add helper methods to fix one out of the many failing tests!

TODO: rest!

make all test_rules.py tests pass!!!

abstract out color selection

vastly improve the use of vertical space in graph nodes

remove now-unnecessary __str__ overrides

add some comments

remove is_intrinsic abstraction leak

use dedent() in test_rules.py again!!

move graph message utils to test_rules.py

respond to review comments
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

Please wait for one more review from someone who can validate the Rust code, e.g. Pierre or Greg.

Copy link
Contributor

@pierrechevalier83 pierrechevalier83 left a comment

Choose a reason for hiding this comment

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

A few style nitpicks, but definitely happy with the overall change and anxious to see it merged 😄

src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/rule_graph/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/src/tasks.rs Show resolved Hide resolved
@cosmicexplorer
Copy link
Contributor Author

There's a clippy error in CI that doesn't show up locally, but making the change suggested by the clippy CI run appears to work, so going to push that change now and make sure it passes clippy.

@Eric-Arellano
Copy link
Contributor

There's a clippy error in CI that doesn't show up locally, but making the change suggested by the clippy CI run appears to work, so going to push that change now and make sure it passes clippy.

I recommend pulling master if you haven't recently. I think we upgraded to a new version of Rust recently and Clippy made some changes.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Feb 8, 2020

I'd pulled master recently, just didn't realize I needed to rebase. Doing that now!

@cosmicexplorer cosmicexplorer merged commit 17d4575 into pantsbuild:master Feb 8, 2020
@stuhood
Copy link
Member

stuhood commented Mar 17, 2020

It looks like this might have ended up affecting the "ambiguous rules to compute X" output:

    Ambiguous rules to compute Get[SourceRootStrippedSources](StripSnapshotRequest) with parameter type LegacyStripTargetRequest:
      @rule(LegacyStripTargetRequest) -> SourceRootStrippedSources,
gets=[Get[SourceRootStrippedSources](StripSnapshotRequest)]
pants.rules.core.strip_source_roots:166:legacy_strip_source_roots_from_target
for ()
      @rule(StripSourcesFieldRequest) -> SourceRootStrippedSources,
gets=[
Get[SourcesResult](SourcesRequest)
Get[SourceRootStrippedSources](StripSnapshotRequest),
]
pants.rules.core.strip_source_roots:124:strip_source_roots_from_sources_field
for ()
    Ambiguous rules to compute Get[SourceRootStrippedSources](StripSnapshotRequest) with parameter type OptionsBootstrapper:
      @rule(LegacyStripTargetRequest) -> SourceRootStrippedSources,
gets=[Get[SourceRootStrippedSources](StripSnapshotRequest)]
pants.rules.core.strip_source_roots:166:legacy_strip_source_roots_from_target
for ()
      @rule(StripSourcesFieldRequest) -> SourceRootStrippedSources,
gets=[
Get[SourcesResult](SourcesRequest)
Get[SourceRootStrippedSources](StripSnapshotRequest),
]
pants.rules.core.strip_source_roots:124:strip_source_roots_from_sources_field
for ()
    Ambiguous rules to compute Get[SourceRootStrippedSources](StripSnapshotRequest) with parameter types (LegacyStripTargetRequest, OptionsBootstrapper):
      @rule(LegacyStripTargetRequest) -> SourceRootStrippedSources,
gets=[Get[SourceRootStrippedSources](StripSnapshotRequest)]
pants.rules.core.strip_source_roots:166:legacy_strip_source_roots_from_target
for ()
      @rule(StripSourcesFieldRequest) -> SourceRootStrippedSources,
gets=[
Get[SourcesResult](SourcesRequest)
Get[SourceRootStrippedSources](StripSnapshotRequest),
]
pants.rules.core.strip_source_roots:124:strip_source_roots_from_sources_field
for ()
    Ambiguous rules to compute Get[SourceRootStrippedSources](StripSnapshotRequest)():
      @rule(LegacyStripTargetRequest) -> SourceRootStrippedSources,
gets=[Get[SourceRootStrippedSources](StripSnapshotRequest)]
pants.rules.core.strip_source_roots:166:legacy_strip_source_roots_from_target
for ()
      @rule(StripSourcesFieldRequest) -> SourceRootStrippedSources,
gets=[
Get[SourcesResult](SourcesRequest)
Get[SourceRootStrippedSources](StripSnapshotRequest),
]
pants.rules.core.strip_source_roots:124:strip_source_roots_from_sources_field
for ()

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.

7 participants