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

Improve support for intrinsic tasks #5792

Merged
merged 5 commits into from May 9, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented May 8, 2018

Problem

#5718 describes a case which resulted in an attempt to lift the wrong input type for a process execution, and #5784 demonstrates a case where intrinsic types can't be used as root subject types.

Both of these are caused by a fragile/hacky implementation of intrinsics that the RuleGraph was (mostly) not aware of.

Solution

Made Intrinsic a RuleGraph::Entry type, and then cleaned up the product requests made by intrinsics so that they will always select the input type before executing.

Result

Fixes #5718, unblocks testing of #5784, and lays cleaner groundwork for addition of further Intrinsic tasks.

@stuhood stuhood changed the title WIP: Improve support for intrinsic tasks Improve support for intrinsic tasks May 9, 2018

@stuhood stuhood force-pushed the twitter:stuhood/cleaner-intrinsics branch from e8842cf to 4fb8ba8 May 9, 2018

@stuhood stuhood force-pushed the twitter:stuhood/cleaner-intrinsics branch from 4fb8ba8 to 1918cc5 May 9, 2018

@stuhood stuhood requested review from illicitonion , ity and cosmicexplorer May 9, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 9, 2018

Reviewable.

@illicitonion
Copy link
Contributor

illicitonion left a comment

This looks great! Thanks for the clean-up!

I restarted a few travis shards because they timed out; I think/hope that travis was just being super slow, and that this change doesn't change pants' performance characteristics...

def execute_process_noop(*args):
raise Exception('This task is replaced intrinsically, and should never run.')
return [
RootRule(ExecuteProcessRequest),

This comment has been minimized.

@illicitonion

illicitonion May 9, 2018

Contributor

Why is this a RootRule?

.core
.rule_graph
.edges_for_inner(entry)
.expect("edges for Snapshot exist.");

This comment has been minimized.

@illicitonion

illicitonion May 9, 2018

Contributor

The way this ends up getting displayed in panic traces, I think this should be negated: edges for Snapshot didn't exist

.core
.rule_graph
.edges_for_inner(entry)
.expect("edges for ExecuteProcessResult exist.");

This comment has been minimized.

@illicitonion

illicitonion May 9, 2018

Contributor

As above

///
/// Begins by selecting PathGlobs for the subject, and then computes a Snapshot for the
/// PathStats matched by the PathGlobs.
/// A Node that captures an fs::Snapshot for a PathGlobs subject.
///
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct Snapshot {

This comment has been minimized.

@illicitonion

illicitonion May 9, 2018

Contributor

I'd be tempted to make this a tuple-struct now

@@ -62,10 +62,16 @@ impl From<RootEntry> for Entry {
}
}

#[derive(Eq, Hash, PartialEq, Clone, Debug)]
pub enum Rule {

This comment has been minimized.

@illicitonion

illicitonion May 9, 2018

Contributor

Can you add some comments explaining what each of these variants represents?

# probably some work required in isolated_process.py to fix this (see #5718).
cat_proc_req = yield Get(ExecuteProcessRequest, CatExecutionRequest, cat_exe_req)
cat_process_result = yield Get(ExecuteProcessResult, ExecuteProcessRequest, cat_proc_req)
cat_process_result = yield Get(ExecuteProcessResult, CatExecutionRequest, cat_exe_req)

This comment has been minimized.

@illicitonion

illicitonion May 9, 2018

Contributor

There are two more of these intermediate request tests we can clean up, too :)

@stuhood stuhood merged commit 8567785 into pantsbuild:master May 9, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/cleaner-intrinsics branch May 9, 2018

stuhood added a commit to twitter/pants that referenced this pull request May 9, 2018

illicitonion added a commit that referenced this pull request May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment