https://pantsbuild.org/ logo
Powered by
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
w

witty-crayon-22786

06/20/2019, 12:17 AM
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?
w

witty-crayon-22786

06/20/2019, 12:23 AM
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
Powered by