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

Stop caching console rules (related to #6146) #6516

Merged
merged 12 commits into from Oct 25, 2018

Conversation

Projects
None yet
3 participants
@blorente
Contributor

blorente commented Sep 18, 2018

Problem

@console_rule tasks were cached, as were @rules. This leads to undesired behaviour, such as empty output on non-first runs of ./pants list.

Solution

To include a field cacheable to console rules, that gets passed down to the engine and used to trigger the execution every time the rule is invoked. This entails:

  • Modifying _make_rule in python to pass an extra parameter cacheable, with True by default and False for @console_rules

  • Augmenting TaskRule with the cacheable field.

  • Augmenting tasks_task_begin in the FFI with an int field to represent cacheable. This int gets coerced to rust booleans.

  • Creating a method fn cacheable in Node that uses the existing (unused) cacheable flag.

  • (tentative) Modifying Entry::complete to account for non-cacheable nodes.

Result

Executing a @console_rule multiple times with pantsd enabled produces the same output.

Related to #6146

Comments

The last commit is broken, it is a tentative effort. Can drop it if needed.

@@ -168,7 +168,7 @@
Handle val_for(Key);
Tasks* tasks_create(void);
void tasks_task_begin(Tasks*, Function, TypeConstraint);
void tasks_task_begin(Tasks*, Function, TypeConstraint, int);

This comment has been minimized.

@stuhood

stuhood Sep 18, 2018

Member

Rather than an int, this should be a _Bool: see the examples above.

I'll go ahead and make the edit directly here (you'll need to pull your branch).

@@ -423,6 +423,25 @@ impl<N: Node> Entry<N> {
None,
previous_result,
)
} else if self.node.content().cacheable() /* Task Not cachable */ {

This comment has been minimized.

@stuhood

stuhood Sep 18, 2018

Member

This condition should be inverted: will edit.

@stuhood

Thanks! This is definitely the bulk of it. A few tweaks are necessary in the final commit, but the rest is ready to land. Can either make those tweaks here, or defer them.

let _ = waiter.send(
previous_result
.clone()
.expect("I expected a result")

This comment has been minimized.

@stuhood

stuhood Sep 18, 2018

Member

What we should do here is either use the previous result (which we retrieved from the Node's current "running" state, or use the current result (which comes in as a parameter to this method).

So this is related to the case below in the "If the new result does not match the previous result" if/else block. If both the previous and current results are None, it would be justifiable to panic (for the reason mentioned in the method doc). But we need to check both of them.

}
// Change to not started, with:
// a new run,
// the same generation, because we ignore it?

This comment has been minimized.

@stuhood

stuhood Sep 18, 2018

Member

You're incrementing the generation here, which is correct: we should treat every run of the node as a new generation, and never store its result.

@@ -1069,6 +1070,13 @@ impl Node for NodeKey {
| NodeResult::Value(_) => None,
}
}
fn cacheable(&self) -> bool {

This comment has been minimized.

@stuhood

stuhood Sep 18, 2018

Member

As discussed in the ticket: it would be totally fine to mark this method (and anything else that needs it) as #[allow(dead_code)] and defer actually using the cacheable flag for another PR. Up to you!

@blorente blorente force-pushed the blorente:blorente/6146/no-cache-console-rules branch from a2e6e1b to 16f38af Sep 19, 2018

@blorente blorente changed the title from [WIP] Stop caching console rules (close #6146) to [WIP] Stop caching console rules (related to #6146) Sep 21, 2018

@blorente blorente force-pushed the blorente:blorente/6146/no-cache-console-rules branch from 16f38af to f57e798 Oct 19, 2018

@blorente blorente changed the title from [WIP] Stop caching console rules (related to #6146) to Stop caching console rules (related to #6146) Oct 23, 2018

@blorente

This comment has been minimized.

Contributor

blorente commented Oct 23, 2018

@stuhood Should be ready for review.

@stuhood

Thanks! Will merge on green.

@stuhood

This comment has been minimized.

Member

stuhood commented Oct 24, 2018

@blorente : One thing that can help with inspecting travis errors is a script that @illicitonion wrote:

$ build-support/bin/get_failing_travis_targets_for_pr.sh 6516
tests/python/pants_test/engine
tests/python/pants_test/engine:scheduler

It also creates a logs directory containing the downloaded logs for easier grepping.

blorente added some commits Oct 25, 2018

@stuhood stuhood merged commit 49146ef into pantsbuild:master Oct 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment