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

Add intrinsic task to merge DirectoryDigests #6635

Merged
merged 6 commits into from Oct 17, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Oct 16, 2018

Fixes #5502

@illicitonion illicitonion requested review from stuhood and blorente Oct 16, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Oct 16, 2018

Commits are independently reviewable

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Oct 16, 2018

One thing I thought about doing, but didn't, was requiring that the inputs be sorted in some way, to ensure cache hits. This node is cheap to compute, though, and I guess the thing which depends on the DirectoryDigest will still hit the cache, so maybe not be worth it...

@stuhood
Copy link
Member

stuhood left a comment

Thanks! Just naming nits.

@@ -117,6 +117,10 @@ def file_stats(self):
return [p.stat for p in self.files]


class MergeDirectoriesRequest(datatype([('directories', tuple)])):

This comment has been minimized.

@stuhood

stuhood Oct 16, 2018

Member

Naming nit: the "request" bit makes more sense (imo) for process executions, because it is hard to pretend that they are referentially transparent. But for something that is definitely deterministic like snapshot merging, it feels easier to say that we want "the snapshot corresponding to these merged directories". So I'd drop Request and make this more nouny... DirectoryMerger MergedDirectories, etc.

This comment has been minimized.

@illicitonion

illicitonion Oct 17, 2018

Contributor

Changed to MergedDirectories

.edges_for_inner(&select.entry)
.ok_or_else(|| {
throw(&format!(
"Tried to select product {} for intrinsic but found no edges",

This comment has been minimized.

@stuhood

stuhood Oct 16, 2018

Member

This mentions selecting for an intrinsic, but this method doesn't look intrinsic specific. Should maybe put intrinsic in the name of the method.

This comment has been minimized.

@illicitonion

illicitonion Oct 17, 2018

Contributor

Moved this from being an inner function to being a struct-level function, pulled out param for description

@@ -198,6 +156,21 @@ impl Select {
return future::ok(value.clone()).to_boxed();
}

fn select(select: &Select, context: &Context, product: TypeConstraint) -> NodeFuture<Value> {

This comment has been minimized.

@stuhood

stuhood Oct 16, 2018

Member

Should this take the Select node as &self?

This comment has been minimized.

@illicitonion

illicitonion Oct 17, 2018

Contributor

Promoted to struct-level fn, done

@@ -99,6 +99,11 @@ impl Tasks {
product: types.files_content,
input: types.directory_digest,
},
Intrinsic {
kind: IntrinsicKind::DirectoryDigest,

This comment has been minimized.

@stuhood

stuhood Oct 16, 2018

Member

Calling this MergedDirectories would be a bit more clear I think.

This comment has been minimized.

@illicitonion

illicitonion Oct 17, 2018

Contributor

Removed kind entirely; it currently duplicates product. This way, we don't have the weird "you may try to project the wrong input" issue, too.

Happy to revert this if you prefer the exhaustiveness checking of the enum

f.write("European Burmese")
with open(os.path.join(temp_dir, "susannah"), "w") as f:
f.write("Not sure actually")
scheduler = self.mk_scheduler(rules=create_fs_rules())

This comment has been minimized.

@stuhood

stuhood Oct 16, 2018

Member

Since this extends TestBase, I think that you should be able to use self.scheduler? Fine either way though.

This comment has been minimized.

@illicitonion

illicitonion Oct 17, 2018

Contributor

Done

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood stuhood merged commit f7caae0 into pantsbuild:master Oct 17, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:dwagnerhall/mergedirectories branch Oct 17, 2018

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