https://pantsbuild.org/ logo
#development
Title
# development
w

witty-crayon-22786

01/13/2022, 6:08 PM
@fast-nail-55400, @enough-analyst-54434: a thought about the
immutable_input_digests
api before we lean in further on porting things to use it
it occurred to me that it could likely instead be an includelist of paths which could be treated as immutable, but which were expected to already be in the digest
so
Copy code
input_digest: Digest
immutable_paths: set[str]
then we’d adjust materialization to materialize as immutable (i.e., symlink) if it encountered an immutable path
f

fast-nail-55400

01/13/2022, 6:15 PM
that would require running
MergeDigests
though on the all of the digests regardless though?
versus avoiding that work if some of the subdirs are immutable
that said, materialization is what dominates sandbox setup so merging to produce the root
Directory
proto shouldn’t be too much of a performance issue
^^ makes assumptions, please confirm/reject assumptions
h

hundreds-father-404

01/13/2022, 6:23 PM
Fwit, that was the API I was originally envisioning because it makes it more ergonomic for rule authors to use the feature, which I think we want to encourage
w

witty-crayon-22786

01/13/2022, 6:25 PM
that would require running 
MergeDigests
 though on the all of the digests regardless though?
yes, but we do anyway in
InputDigests
to validate that there are no collisions
we’d also discussed using a merged digest to do materialization anyway
@hundreds-father-404: yea, i think that it would be.
h

hundreds-father-404

01/13/2022, 6:26 PM
Would paths be files and/or dirs, like the new REAPI
output_paths
field?
w

witty-crayon-22786

01/13/2022, 6:27 PM
afaict, it doesn’t impact the ability to mount an immutable path… as long as we never introduce a “mutable paths” field which we’d then have to validate as not overlapping
@hundreds-father-404: shouldn’t matter either way, afaict
that sounds like rough agreement.
@hundreds-father-404: if you have any interest in working on this, it might be a nice diversion from the lockfile stuff. it’s lower priority, but would be good to do before https://github.com/pantsbuild/pants/issues/14070
f

fast-nail-55400

01/13/2022, 7:15 PM
I’m fine with whatever the API is. If the merging is necessary for correctness any way, no objection from me on what is proposed.
h

hundreds-father-404

01/13/2022, 7:28 PM
I'd love to have a diversion from lockfile stuff hehe. Will finish unifying the lockfile goals first though
I started looking into this. Iiuc, we change
<http://local.rs:prepare_workdir|local.rs:prepare_workdir>
. We'd need to subset the `input_digest`: partition into mutable vs immutable, right? And then
store.materialize_directory()
as we do now on the mutable digest, but still
symlink()
on each
immutable_path
? If that's the case, I'm confused how we handle multiple distinct immutable input digests. My read of
<http://immutable_inputs.rs|immutable_inputs.rs>
is that we intentionally separate out distinct digests like
__toolcp
vs
__processorcp
, which each get their own
path()
. Those each might have n files, but they're grouped by that digest's same
path()
to the dir. We'd need to start having one path-per-file iiuc, as we no longer can group related files since it's a flattened list?
w

witty-crayon-22786

01/19/2022, 11:55 PM
We’d need to subset the `input_digest`: partition into mutable vs immutable, right? And then 
store.materialize_directory()
 as we do now on the mutable digest, but still 
symlink()
 on each 
immutable_path
?
no need to subset in
input_digest
i don’t think. because
materialize_directory
will take the whole digest, and then symlink on a case-by-case basis
If that’s the case, I’m confused how we handle multiple distinct immutable input digests. My read of 
<http://immutable_inputs.rs|immutable_inputs.rs>
 is that we intentionally separate out distinct digests like 
__toolcp
 vs 
__processorcp
, which each get their own 
path()
.
we wouldn’t have multiple distinct immutable input digests anymore: we’d go back to just
input_digest
the idea would be for the caller to lay out the directory structure (via `MergeDigest`/prefix/etc) as they were before (…oops), but maintaining the directory based nesting that we have now
they’d then mark directories as immutable
h

hundreds-father-404

01/19/2022, 11:59 PM
Oh, so there would still be distinct directories? That makes more sense. And then is it everything in and under each
immutable_dir
is symlinked? Meaning, we do not need to change to processing per-file. We can still do per-directory
And then is it everything in and under each
immutable_dir
is symlinked?
if
input_digest
has
a/f1.txt
,
a/f2.txt
,
a/sub/f.txt
, and
b/f.txt
, and you have
immutable_dirs=['a']
, then
materialize_directory
will see to symlink
a/f1.txt
,
a/f2.txt
, and
a/sub/f.txt
together?
w

witty-crayon-22786

01/20/2022, 12:58 AM
And then is it everything in and under each 
immutable_dir
 is symlinked?
when you symlink a path, you’re done operating under that path, because all of the content under that path is provided (immutably) by the symlink
so in your example, you need exactly one symlink for
a
👍 1
h

hundreds-father-404

01/20/2022, 1:01 AM
But we do need to know to not try writing via conventional methods those 3 files, right? Or perhaps symlink everything first and then only write if not already present in the sandbox
w

witty-crayon-22786

01/20/2022, 1:01 AM
no, because
materialize_directory
operates recursively. when you hit the path in question, you would symlink, and then not recurse any further
1
there is a bit of an odd interplay here because
materialize_directory
will be checking for paths that it should symlink, but it won’t know know the digest to symlink until here: https://github.com/pantsbuild/pants/blob/e77191184f66e866a7282f21f40f6ad652566d62/src/rust/engine/fs/store/src/lib.rs#L993-L997
h

hundreds-father-404

01/24/2022, 5:36 PM
I'm not certain I'll be able to work more on this this week, lots of writing I have queued up for blogs. We might want to land the Pex immutable_digests improvement now without blocking on this? We already have 15+ uses of immutable_digests iirc, doesn't seem that big of a deal to have 20-30 when we make this refactor. The performance gains for users is worth the pain of refactoring more code
w

witty-crayon-22786

01/24/2022, 5:36 PM
yep, that’s fine with me.
h

hundreds-father-404

01/24/2022, 5:39 PM
cool, I'll add that this week