https://pantsbuild.org/ logo
h

hundreds-breakfast-49010

06/19/2019, 10:08 PM
I'm looking at https://github.com/pantsbuild/pants/issues/7745 right now, I'm trying to get a handle on what
Graph
is and why there's a distinction between FallibleExecutProcessResult and ExecuteProcessResult, and other things pertaining to these types
h

hundreds-father-404

06/19/2019, 10:10 PM
a
FallibleExecuteProcessResult
allows for failures. It’s useful for things like unit tests, where we are okay with the process failing
In general, another really helpful file to familiarize yourself with is https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/fs.py, which defines the core V2 abstractions for the file system
engine/fs.py
and
engine/isolated_process.py
define most of the primitives that you use to create a V2 rule
h

hundreds-breakfast-49010

06/19/2019, 11:07 PM
so the issue talks about memoization of the description of a process in the
NodeKey
type
I assume that's the one in
engine/src/nodes.rs
?
h

hundreds-father-404

06/19/2019, 11:08 PM
yes
h

hundreds-breakfast-49010

06/19/2019, 11:39 PM
anyway, I'm not sure how the human-readable description is included in this data structure
or how the memoization in graph works
h

hundreds-father-404

06/19/2019, 11:43 PM
cc @witty-crayon-22786 or @average-vr-56795
w

witty-crayon-22786

06/20/2019, 12:12 AM
@hundreds-breakfast-49010: the entire
NodeKey
(which derives
Eq
) is the "key"/identity for a
Node
in the
Graph
graph
is a separate crate with tests and a small set of traits: https://github.com/pantsbuild/pants/blob/master/src/rust/engine/graph/src/node.rs
impl Node for NodeKey
(or WrappedNodeKey...? i forget which.)
h

hundreds-breakfast-49010

06/20/2019, 12:14 AM
ah ok
I see the impl
I'm not sure how the description of a process comes into this though
w

witty-crayon-22786

06/20/2019, 12:15 AM
@hundreds-breakfast-49010: it's in the derived
Eq
for that NodeKey
h

hundreds-breakfast-49010

06/20/2019, 12:16 AM
ah okay
so
ExecutProcess
is a newtype wrapper around
ExecuteProcessRequest
h

hundreds-breakfast-49010

06/20/2019, 12:17 AM
I see
so basically we want to tweak the implemention of
Eq
on
NodeKey
to omit the description?
w

witty-crayon-22786

06/20/2019, 12:18 AM
sorry, still catching up. could have included these links in the ticket description, but wasn't sure which ticket you would latch onto
possibly. see the two possibilities on the ticket
when a user's
@rule
requests a process execution, they include a description
that is the python/rust boundary again... sec
convert a python ExecuteProcessRequest into the rust equivalent: https://github.com/pantsbuild/pants/blob/master/src/rust/engine/src/nodes.rs#L348-L407
h

hundreds-breakfast-49010

06/20/2019, 12:21 AM
sounds like we might want
NodeKey::ExecuteProcess
to not wrap
ExecuteProcess
, which has the descritpion, ultimately
w

witty-crayon-22786

06/20/2019, 12:22 AM
if we are able to preserve the description without including it in the Node, then that would be great
h

hundreds-breakfast-49010

06/20/2019, 12:22 AM
but for it to wrap some other type which duplicates most of
ExecuteProcessRequest
?
w

witty-crayon-22786

06/20/2019, 12:22 AM
if we aren't, then so be it
h

hundreds-breakfast-49010

06/20/2019, 12:22 AM
what is the Node doing here?
w

witty-crayon-22786

06/20/2019, 12:22 AM
@hundreds-breakfast-49010: or to compose them
a
Node
is a item in the
graph
... it's the unit of memoization
h

hundreds-breakfast-49010

06/20/2019, 12:23 AM
also why does the
ExecuteProcess
newtype exist?
that intermediate newtype might not need to exist...?
h

hundreds-breakfast-49010

06/20/2019, 12:24 AM
ok
w

witty-crayon-22786

06/20/2019, 12:24 AM
NodeKey::ExecuteProcess
could probably directly hold a
process_execution::ExecuteProcess
unsure if there is a coherence issue there, but i don't think so.
h

hundreds-breakfast-49010

06/20/2019, 12:27 AM
okay
except, if
NodeKey
holds a
ExecuteProcessRequest
at all, it's going to have the description field internally
so maybe we should change the definition of
ExecuteProcess
to have all the fields of
ExecuteProessRequest
except the description
w

witty-crayon-22786

06/20/2019, 12:28 AM
Yea, that's one of the possibilities
h

hundreds-breakfast-49010

06/20/2019, 12:28 AM
rewrite
ExecuteProcessRequest
to have a description and a field of a new type
ExecuteProcess
, that same
ExecuteProcess
is used in
NodeKey
I don't know where the memoization lookups happen such that looking up this new version of
ExecuteProcess
(without the description) would end up pulling the wrong description
w

witty-crayon-22786

06/20/2019, 12:29 AM
That might mean that we would have to change how we render rules: see the other thread about UX with @red-balloon-89377