Nhoward/rust graph validation pt i #4227

Merged
merged 22 commits into from Feb 6, 2017

Conversation

Projects
None yet
2 participants
@baroquebobcat
Contributor

baroquebobcat commented Feb 1, 2017

This is the first step towards adapting the static noop culling I worked on last year to the rust code.

This first patch is working towards replacing the graph validator written in python with one in rust. At this point it covers quite a lot of behavior, but it isn't working quite correctly yet.

I'm putting it up in the current state to get some other eyes on it while I iterate.

I'm hoping to have this initial patch wrapped up tomorrow, modulo bugs.

Things to look at:

  • I replaced the root selector fn dict with a set of subjects and a method. Since all of the subjects now use the same selector, it didn't make sense.
  • rule_graph.rs is where the meat of the port is. It still is a bit messy with commented python in places. I'm planning on removing the rest of them and leaving TODOs where I want to pick up the thread in the next patch.
  • I've also added a few comments in the extension points for the next phase, but I can remove or clear them up if they are a problem.
@@ -149,6 +149,8 @@
ExecutionStat execution_execute(RawScheduler*);
RawNodes* execution_roots(RawScheduler*);
+ void run_validator(RawScheduler*, TypeId*, uint64_t);

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Naming nit... the methods are mostly namespaced. So maybe validator_run.

@stuhood

stuhood Feb 1, 2017

Member

Naming nit... the methods are mostly namespaced. So maybe validator_run.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

handled.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

handled.

src/rust/engine/src/graph.rs
@@ -213,6 +216,7 @@ impl Graph {
let src = self.entry_for_id(src_id);
dsts.into_iter()
.filter(|dst_id| !(src.dependencies.contains(dst_id) || src.cyclic_dependencies.contains(dst_id)))
+ // RGTODO add filter that removes deps if they will noop based on their rule edges

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

I think that this will obviously be part of phase 2/3, but it's not clear that this is where it would happen, so the todo is probably misplaced.

@stuhood

stuhood Feb 1, 2017

Member

I think that this will obviously be part of phase 2/3, but it's not clear that this is where it would happen, so the todo is probably misplaced.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

removed.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

removed.

src/rust/engine/src/rule_graph.rs
+
+// I think I ought to be able to replace the below with a set of structs keyed by EntryType.
+// My first couple attempts failed.
+#[derive(Eq, Hash, PartialEq, Clone, Debug)]

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Since this struct is probably 1 byte wide, just marking it Copy rather than Clone would be perfectly reasonable, and allow you to avoid explicitly calling clone in a bunch of places.

@stuhood

stuhood Feb 1, 2017

Member

Since this struct is probably 1 byte wide, just marking it Copy rather than Clone would be perfectly reasonable, and allow you to avoid explicitly calling clone in a bunch of places.

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

(...and ignore this if you end up going the "one big enum" route.)

@stuhood

stuhood Feb 1, 2017

Member

(...and ignore this if you end up going the "one big enum" route.)

src/rust/engine/src/rule_graph.rs
+ entry_type: EntryType::Unreachable,
+ subject_type: None,
+ value: None,
+ rule: Some(rule.clone()), // TODO decide on clone vs lifetimes

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

I'd stick with cloning for now. Can revisit later if it's an issue, but it seems unlikely to be.

@stuhood

stuhood Feb 1, 2017

Member

I'd stick with cloning for now. Can revisit later if it's an issue, but it seems unlikely to be.

src/rust/engine/src/rule_graph.rs
+ selector: None,
+ reason: None,
+ }
+ }

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

missing whitespace

@stuhood

stuhood Feb 1, 2017

Member

missing whitespace

src/rust/engine/src/tasks.rs
@@ -8,9 +8,13 @@ use selectors::{Selector, Select, SelectDependencies, SelectLiteral, SelectProje
* Registry of tasks able to produce each type, along with a few fundamental python
* types that the engine must be aware of.
*/
+#[derive(Clone)]

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Can you open a github issue about removing Clone here? It's a bit error prone, because the tasks get passed around everywhere currently, and cloning them accidentally (probably in a parent struct) would get costly fast.

@stuhood

stuhood Feb 1, 2017

Member

Can you open a github issue about removing Clone here? It's a bit error prone, because the tasks get passed around everywhere currently, and cloning them accidentally (probably in a parent struct) would get costly fast.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

Done. #4236

+ self.all_rules().iter().map(|t| t.product).collect()
+ }
+
+ pub fn is_singleton_task(&self, sought_task: &Task) -> bool {

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Worth adding a Singleton/Intrinisic/Normal enum on Task?

@stuhood

stuhood Feb 1, 2017

Member

Worth adding a Singleton/Intrinisic/Normal enum on Task?

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

hm. that would certainly simplify these. I think I'd want to split that out as a separate change though. Sound reasonable?

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

hm. that would certainly simplify these. I think I'd want to split that out as a separate change though. Sound reasonable?

src/rust/engine/src/tasks.rs
+ }
+
+ pub fn all_rules(&self) -> Vec<Task> {
+ let mut result: Vec<Task> = Vec::new();

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Another way to accomplish method would be:

let rules: Vec<_> = self.tasks.values().chain(self.singletons.values()).chain(self.singletons.values()).collect();

@stuhood

stuhood Feb 1, 2017

Member

Another way to accomplish method would be:

let rules: Vec<_> = self.tasks.values().chain(self.singletons.values()).chain(self.singletons.values()).collect();

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

neat. I had to add a flat_map, but that's much more concise without losing readability.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

neat. I had to add a flat_map, but that's much more concise without losing readability.

src/rust/engine/src/rule_graph.rs
+ let rules_in_graph: HashSet<Task> = full_dependency_edges.keys().map(|f| f.rule().clone()).collect();
+ let unfulfillable_discovered_during_construction: HashSet<Task> = full_unfulfillable_rules.keys().map(|f| f.rule().clone()).collect();
+ let declared_rules = self.tasks.all_rules();
+ let unreachable_rules: HashSet<&Task> = declared_rules.iter()

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Note that when the type annotation is use to guide inference for collect, it isn't necessary to actually fill in its parameters:

let unreachable_rules: HashSet<_> = declared_rules.iter()...
@stuhood

stuhood Feb 1, 2017

Member

Note that when the type annotation is use to guide inference for collect, it isn't necessary to actually fill in its parameters:

let unreachable_rules: HashSet<_> = declared_rules.iter()...

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

ah, so it's like the diamond operator in Java. Also neat. Is doing it that way preferred over the turbo fish? ie let foo = ...collect::<Vec<_>>()

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

ah, so it's like the diamond operator in Java. Also neat. Is doing it that way preferred over the turbo fish? ie let foo = ...collect::<Vec<_>>()

src/rust/engine/src/rule_graph.rs
+ .filter(|g| !root_rule_dependency_edges.contains_key(g))
+ .map(|g| g.clone());
+ for unseen_rule in unseen_dep_rules {
+ rules_to_traverse.push_back(unseen_rule);

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Can use extend here, which consumes an iterator.

rules_to_traverse.extend(unseen_dep_rules);

@stuhood

stuhood Feb 1, 2017

Member

Can use extend here, which consumes an iterator.

rules_to_traverse.extend(unseen_dep_rules);

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

done

@baroquebobcat

baroquebobcat Feb 3, 2017

Contributor

done

@stuhood

I think my only blocker for landing this would be cleaning up the commented code a bit.

Tweaking the datamodel a bit would likely lay important groundwork too, but I won't block on it.

@stuhood

stuhood approved these changes Feb 3, 2017

Thanks Nick!

src/rust/engine/src/graph.rs
@@ -1,3 +1,5 @@
+// Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@stuhood

stuhood Feb 3, 2017

Member

If you're going to add these, might as well go with the current year.

@stuhood

stuhood Feb 3, 2017

Member

If you're going to add these, might as well go with the current year.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

roger

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

roger

src/rust/engine/src/selectors.rs
@@ -59,6 +59,9 @@ impl Selector {
)
}
+ /*
+ * The product type this selector will ultimately produce.

This comment has been minimized.

@stuhood

stuhood Feb 3, 2017

Member

I know we're not really following rust style at all, but the indentation is funny here.

@stuhood

stuhood Feb 3, 2017

Member

I know we're not really following rust style at all, but the indentation is funny here.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

yeah. I'll reformat to be at least Javaish

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

yeah. I'll reformat to be at least Javaish

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

Hm. After thinking a bit, I looked at the Rust book again. It seems like line comments are preferred. And the doc comments are form of line comments. I'm changing it to line comments
https://doc.rust-lang.org/beta/book/comments.html

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

Hm. After thinking a bit, I looked at the Rust book again. It seems like line comments are preferred. And the doc comments are form of line comments. I'm changing it to line comments
https://doc.rust-lang.org/beta/book/comments.html

src/rust/engine/src/rule_graph.rs
+// I think I ought to be able to replace the below with a set of structs keyed by EntryType.
+// My first couple attempts failed.
+#[derive(Eq, Hash, PartialEq, Clone, Debug)]
+enum EntryType {

This comment has been minimized.

@stuhood

stuhood Feb 3, 2017

Member

Much better! It looks like is now just Entry?

@stuhood

stuhood Feb 3, 2017

Member

Much better! It looks like is now just Entry?

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

Good call.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

Good call.

+
+ },
+ &Selector::Task(ref select) =>{
+ // TODO, not sure what task is in this context exactly

This comment has been minimized.

@stuhood

stuhood Feb 3, 2017

Member

It's for symmetry: every Node type has a Selector currently. But probably possible to remove.

@stuhood

stuhood Feb 3, 2017

Member

It's for symmetry: every Node type has a Selector currently. But probably possible to remove.

baroquebobcat added some commits Feb 6, 2017

Merge branch 'master' into nhoward/rust_graph_validation_pt_i
-- also removed the Clone on Tasks since an upstream change caused it to not be viable
src/rust/engine/src/tasks.rs
@@ -8,9 +8,14 @@ use selectors::{Selector, Select, SelectDependencies, SelectLiteral, SelectProje
* Registry of tasks able to produce each type, along with a few fundamental python
* types that the engine must be aware of.
*/
+// TODO remove Clone https://github.com/pantsbuild/pants/issues/4236
+#[derive(Clone)]

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

With the merge from master, I was forced to handle this. So it's gone. Closing the issue.

@baroquebobcat

baroquebobcat Feb 6, 2017

Contributor

With the merge from master, I was forced to handle this. So it's gone. Closing the issue.

baroquebobcat added some commits Feb 6, 2017

respond to last round of comments
- fix year
- s/EntryType/Entry/
- change comments
- clarify panics in a few places

@baroquebobcat baroquebobcat merged commit 62f2b10 into pantsbuild:master Feb 6, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

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

[engine] Begin port of engine rule graph validation to Rust (#4227)
This is the first step towards adapting the static noop culling I worked on last year to the Rust code base.

This first patch is working towards replacing the graph validator written in python with one in rust. At this point it covers quite a lot of behavior, but it isn't working quite correctly yet.

This patch includes porting the majority of the rule graph construction. Including
- replacing the root selector fn dict with a set of subjects and a method. Since all of the subjects now use the same selector, it didn't make sense to keep.
- running the rust validator, but ignoring the results.
- adding copyright headers.

rule_graph.rs is where the meat of the port is. There are still a number of unported sections. I'll be removing the commented python and replacing it with rust in subsequent pull requests

The remaining portions are
- adding a enum on tasks for their task type
- removing unfulfillable entries from the graph
- handling SelectDependencies / SelectProjection selectors
- handling snapshots
- propagating the results back to the python side
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment