https://pantsbuild.org/ logo
Join Slack
Powered by
I'm looking at <https://github.com/pantsbuild/pant...
# development
h
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
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
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
yes
h
anyway, I'm not sure how the human-readable description is included in this data structure
or how the memoization in graph works
h
cc @witty-crayon-22786 or @average-vr-56795
w
@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
ah ok
I see the impl
I'm not sure how the description of a process comes into this though
w
@hundreds-breakfast-49010: it's in the derived 
Eq
for that NodeKey
h
ah okay
so 
ExecutProcess
is a newtype wrapper around 
ExecuteProcessRequest
w
h
I see
so basically we want to tweak the implemention of 
Eq
on 
NodeKey
to omit the description?
w
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
sounds like we might want 
NodeKey::ExecuteProcess
to not wrap 
ExecuteProcess
, which has the descritpion, ultimately
w
if we are able to preserve the description without including it in the Node, then that would be great
h
but for it to wrap some other type which duplicates most of 
ExecuteProcessRequest
?
w
if we aren't, then so be it
h
what is the Node doing here?
w
@hundreds-breakfast-49010: or to compose them
a 
Node
is a item in the 
graph
... it's the unit of memoization
h
also why does the 
ExecuteProcess
newtype exist?
w
that intermediate newtype might not need to exist...?
h
ok
w
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
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
Yea, that's one of the possibilities
h
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
That might mean that we would have to change how we render rules: see the other thread about UX with @red-balloon-89377
Open in Slack
PreviousNext
Powered by