[engine] rm python graphmaker; create dot formatted display #4295

Merged
merged 5 commits into from Mar 7, 2017

Conversation

Projects
None yet
2 participants
@baroquebobcat
Contributor

baroquebobcat commented Mar 1, 2017

Problem

Printing rule graphs has not been converted. Also, generating subgraphs.

Solution

(describe the modifications you've made.)

Result

(describe how your changes affect the end-user behavior of the system. this section is
optional, and should generally be summarized in the title of the pull request.
)

src/rust/engine/src/lib.rs
+ product_type: TypeConstraint
+) -> RuleGraph {
+ let graph_maker = GraphMaker::new(&raw.scheduler.tasks,
+ RootSubjectTypes { subject_types: vec![subject_type.clone()] });

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

This struct doesn't appear to be carrying its weight. Could either make it a tuple struct so that you can construct it more easily (RootSubjectTypes(vec![subject_type.clone())) or just remove it, probably.

@stuhood

stuhood Mar 1, 2017

Member

This struct doesn't appear to be carrying its weight. Could either make it a tuple struct so that you can construct it more easily (RootSubjectTypes(vec![subject_type.clone())) or just remove it, probably.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Yeah. I added it thinking it might accrete more behavior, but then it didn't. xxing

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Yeah. I added it thinking it might accrete more behavior, but then it didn't. xxing

src/rust/engine/src/lib.rs
+ let file = try!(OpenOptions::new().append(true).open(path));
+ let mut f = BufWriter::new(file);
+
+ try!(write!(&mut f, "{}\n", format!("{}", graph)));

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

We've mostly switched to ? rather than try!... will make a pass over the old code at some point, but might as well use ? in new code.

@stuhood

stuhood Mar 1, 2017

Member

We've mostly switched to ? rather than try!... will make a pass over the old code at some point, but might as well use ? in new code.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

updating to it.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

updating to it.

src/rust/engine/src/lib.rs
+}
+
+fn write_to_file(path: &Path, graph: &RuleGraph) -> io::Result<()> {
+ let file = try!(OpenOptions::new().append(true).open(path));

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

Append doesn't seem like a good default... I don't want a graph appended to an existing file, since dot isn't appendable afaik?

@stuhood

stuhood Mar 1, 2017

Member

Append doesn't seem like a good default... I don't want a graph appended to an existing file, since dot isn't appendable afaik?

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Makes sense. This was cargo culted from one of the other file creating fns. -- trace in graph.rs

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Makes sense. This was cargo culted from one of the other file creating fns. -- trace in graph.rs

src/rust/engine/src/rule_graph.rs
+ unfulfillable_rules: full_unfulfillable_rules,
+ }
+ }
+ let beginning_root = beginning_root_opt.unwrap();

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

A cleaner way to work with an Option while still returning early might be:

let beginning_root = 
  if let Some(beginning_root) = self.gen_root_entry(subject_type, product_type) {
    beginning_root
  } else {
    return ...
  };
@stuhood

stuhood Mar 1, 2017

Member

A cleaner way to work with an Option while still returning early might be:

let beginning_root = 
  if let Some(beginning_root) = self.gen_root_entry(subject_type, product_type) {
    beginning_root
  } else {
    return ...
  };

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

yeah, my python / java is showing a bit there.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

yeah, my python / java is showing a bit there.

+ full_dependency_edges = constructed_graph.rule_dependency_edges.clone();
+ full_unfulfillable_rules = constructed_graph.unfulfillable_rules.clone();
+
+ self.add_unreachable_rule_diagnostics(&full_dependency_edges, &mut full_unfulfillable_rules);

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

It certainly seems like you should be able to pass these in without cloning them... is the issue just that constructed_graph isn't mutable? let mut constructed_graph = ...

@stuhood

stuhood Mar 1, 2017

Member

It certainly seems like you should be able to pass these in without cloning them... is the issue just that constructed_graph isn't mutable? let mut constructed_graph = ...

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Yep. I started to see that when I extracted the method. Thanks for pointing it out.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Yep. I started to see that when I extracted the method. Thanks for pointing it out.

src/rust/engine/src/rule_graph.rs
+ unfulfillable_rules: full_unfulfillable_rules
+ };
+
+ self._remove_unfulfillable_rules_and_dependents(unfinished_graph)

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

Another reminder that this pattern of methods that take a value, mutate it, and then return the same value... feels weird to me. I would expect a method that mutates something and then gives it back to take a &mut to the thing.

@stuhood

stuhood Mar 1, 2017

Member

Another reminder that this pattern of methods that take a value, mutate it, and then return the same value... feels weird to me. I would expect a method that mutates something and then gives it back to take a &mut to the thing.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

So instead of taking ownership, modifying and passing back ownership, just doing an &mut is more idiomatic?

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

So instead of taking ownership, modifying and passing back ownership, just doing an &mut is more idiomatic?

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Yes, I believe so. I'm not sure whether LLVM or rustc will sneakily turn the call into a reference for you, but afaik: moving values in is ... literally moving (ie memcpying) them. The graph is not a large struct (it only has Vecs in it, and they are always 3 words long?) but I think the idea is that when you can pass by reference, you should.

@stuhood

stuhood Mar 2, 2017

Member

Yes, I believe so. I'm not sure whether LLVM or rustc will sneakily turn the call into a reference for you, but afaik: moving values in is ... literally moving (ie memcpying) them. The graph is not a large struct (it only has Vecs in it, and they are always 3 words long?) but I think the idea is that when you can pass by reference, you should.

src/rust/engine/src/rule_graph.rs
@@ -602,6 +690,54 @@ impl RuleGraph {
}
}
+impl fmt::Display for RuleGraph {

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

This seems like an abuse of Display. I wouldn't accidentally want to drop this in a log message, for example.

@stuhood

stuhood Mar 1, 2017

Member

This seems like an abuse of Display. I wouldn't accidentally want to drop this in a log message, for example.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Yeah. I agree. I was planning on taking this out, but with the support rotation stuff---I wanted to start the CI runs before fixing it.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

Yeah. I agree. I was planning on taking this out, but with the support rotation stuff---I wanted to start the CI runs before fixing it.

src/rust/engine/src/tasks.rs
@@ -71,7 +71,11 @@ impl Tasks {
}
pub fn all_product_types(&self) -> Vec<TypeConstraint> {
- self.all_rules().iter().map(|t| t.product).collect()
+ let mut product_types: Vec<_> = self.all_rules().iter().map(|t| t.product).collect();
+ // NB sorted by id so that dedup will consolidate runs of duplicates.

This comment has been minimized.

@stuhood

stuhood Mar 1, 2017

Member

Any reason not to just collect into a hashset instead?

@stuhood

stuhood Mar 1, 2017

Member

Any reason not to just collect into a hashset instead?

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

I think that should work.

@baroquebobcat

baroquebobcat Mar 2, 2017

Contributor

I think that should work.

@stuhood

stuhood approved these changes Mar 2, 2017

Thanks!

src/rust/engine/src/lib.rs
- try!(write!(&mut f, "{}\n", format!("{}", graph)));
+ let file = File::create(path)?;
+ let mut f = io::BufWriter::new(file);
+ graph.visualize(&mut f)?;

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Rather than:

graph.visualize(&mut f)?;
Ok(())

... you can just return the result of visualize:

graph.visualize(&mut f)
@stuhood

stuhood Mar 2, 2017

Member

Rather than:

graph.visualize(&mut f)?;
Ok(())

... you can just return the result of visualize:

graph.visualize(&mut f)

@baroquebobcat baroquebobcat merged commit 45507e2 into pantsbuild:master Mar 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] rm python graphmaker; create dot formatted display (#4295)
### Problem

Printing rule graphs has not been converted to Rust. This can make it hard to visualize what the rule sets look like. Also, the Python rule graph construction is effectively dead code already.

### Solution

Port rule graph visualization and visualization based testing to the rust side of the engine implementation.

### Result

Now we're setup to use the RuleGraph from the Rust side for functions as described in #4303 and #4304.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment