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

illicitonion
Copy link
Contributor

Fixes #7716

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Eric-Arellano
Copy link
Contributor

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

@stuhood
Copy link
Member

stuhood 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
@illicitonion illicitonion deleted the dwagnerhall/digest-to-snapshot branch May 14, 2019 21:55
Eric-Arellano added a commit that referenced this pull request May 16, 2019
### 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide in V2 a builtin rule to go from Digest -> Snapshot
3 participants