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

Targets always have a Snapshot #5994

Merged
merged 10 commits into from Jun 27, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 21, 2018

Unless they're somehow specially generated, their Snapshot will be
populated during parsing in parallel in the engine.

If they weren't generated during parsing, the scheduler arg will be used
to synchronously capture one on demand.

This enables the sources of all Targets to be used in remote execution.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/lazyfileset/snapshotseverywhere branch 2 times, most recently from 41536af to 1a81185 Jun 21, 2018

@illicitonion illicitonion changed the title WIP Targets always have a Snapshot Jun 21, 2018

@illicitonion illicitonion requested review from stuhood , dotordogh , kwlzn , ity and benjyw Jun 21, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jun 21, 2018

@stuhood This is now ready for review.

The first two commits are #5993 and #5997 and the last two are independently reviewable.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/lazyfileset/snapshotseverywhere branch from 1a81185 to 82341e7 Jun 21, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good!

I do think we'll need to take advantage of the batch Snapshot capture though.

"""
:param rel_root: The root for the given filespec, relative to the buildroot.
:param filespec: A filespec as generated by `FilesetRelPathWrapper`, which represents
what globs or file list it came from. Must be relative to buildroot.
:param files: A list of matched files, with declared order and duplicates preserved.

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

The "duplicates" bit was a lie anyway.

# Build the target and handle duplicate sources.
if not vt.valid:
if self._do_validate_sources_present(vt.target):
self.execute_codegen(vt.target, vt.results_dir)
self._handle_duplicate_sources(vt.target, vt.results_dir)
sources = self._capture_sources(vt.target, vt.results_dir)

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

This would be a very good usecase for using the batchness of the capture_snapshots method, and I think we're going to need to here to get adequate performance on a large graph.

If you split out a "generate and collect sources" loop before a "inject synthetic targets" loop, you could make one call to capture_snapshots for all targets in between the two loops.

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Done

@kwlzn

kwlzn approved these changes Jun 22, 2018

'deprecated, and now takes a slow path. Instead, class {} should have its sources '
'argument populated by the engine, either by using the standard parsing pipeline, or by '
'requesting a SourcesField product.'
'the v2 engine.').format(self.__class__.__name__)

This comment has been minimized.

@kwlzn

kwlzn Jun 22, 2018

Member

s/requesting a SourcesField product.the v2 engine./requesting a SourcesField product from the v2 engine./ ?

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Done

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jun 22, 2018

For my reference: I can reproduce the CI failures by running:
./pants run contrib/go/examples/src/go/distance

illicitonion added some commits Jun 21, 2018

Targets always have a Snapshot
Unless they're somehow specially generated, their Snapshot will be
popualted during parsing in parallel in the engine.

If they weren't generated during parsing, the scheduler arg will be used
to synchronously capture one on demand.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/lazyfileset/snapshotseverywhere branch from 82341e7 to 5d9ac89 Jun 26, 2018

@kwlzn

kwlzn approved these changes Jun 27, 2018

Copy link
Member

kwlzn left a comment

lgtm

@stuhood
Copy link
Member

stuhood left a comment

Looks good!

@stuhood stuhood merged commit 6229c41 into pantsbuild:master Jun 27, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/lazyfileset/snapshotseverywhere branch Aug 15, 2018

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