Skip to content
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 to lift a Digest to a Snapshot #7725

Merged

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

commented May 14, 2019

Fixes #7716

@Eric-Arellano
Copy link
Contributor

left a comment

Awesome. Thank you Daniel for doing this and for pairing on it!

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Could you please rename the title to Add builtin Rust rule to lift a Digest to a Snapshot?

@stuhood

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Could you please rename the title to Add builtin Rust rule to lift a Digest to a Snapshot?

Let's hold off on that pending #7707. I'd be interested in seeing justification of the name change.

@illicitonion illicitonion merged commit 1461d4b into pantsbuild:master May 14, 2019

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/digest-to-snapshot branch May 14, 2019

Eric-Arellano added a commit that referenced this pull request May 16, 2019

Extract a generalized V2 rule to inject `__init__.py` files (#7722)
### Problem
Python requires packages to have an `__init__.py` file to be recognized as a proper module for the sake of imports.

#7696 does this for Pytest, but inlines the logic, even though it will likely be helpful for other Python rules as well.

Further, because this logic was originally written before being able to from `Digest->Snapshot` thanks to #7725, we had to use `FilesContent` to grab the paths of the digest. This would mean that every single source file would be materialized and persisted to memory, resulting in extremely high memory usage (found as part of investigation in #7741). There is no need for the actual content, just the paths, so this is a huge inefficiency.

Will close #7715.

### Solution
Generalize into `@rule(InitInjectedDigest, [Snapshot])`, where `InitInjectedDigest` is a thin wrapper around a `Digest`.

We take a `Snapshot` because we need the file paths to work properly. This contrasts with earlier using `FileContents` to get the same paths. A `Snapshot` is much more light weight.

We return a `Digest` because that is the minimum information necessary to work properly, and the caller of the rule can then convert that `Digest` back into a `Snapshot`.

### Result
It will now be easier for other Python rules to work with Python packages.

The unnecessary memory usage is now fixed. The V2 Pytest runner now has a space complexity of `O(t + t*e + b)`, rather than `O(t + t*e + s)`, where `t` is # targets, `e` is # env vars, `b` is # `BUILD` files, and `s` is # source files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.