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

hundreds-breakfast-49010

01/17/2020, 9:04 PM
some_rule
is doing to wind up a
Task
node, right?
a

aloof-angle-91616

01/17/2020, 9:05 PM
yes
in
src/python/pants/engine/rules.py
you can see how it goes:
w

witty-crayon-22786

01/17/2020, 9:05 PM
Yes.
a

aloof-angle-91616

01/17/2020, 9:05 PM
@rule => _make_rule() => TaskRule
no difference in behavior for async rules
h

hundreds-breakfast-49010

01/17/2020, 9:06 PM
and at some point,
Graph::get
is going to be called with a
src_id
argument whose
EntryId
is the one that corresponds to some particular
Node
that was generated in a particular spot where
some_rule
is used, right?
a

aloof-angle-91616

01/17/2020, 9:07 PM
i think so, let me read that again
could you elaborate or restate?
h

hundreds-breakfast-49010

01/17/2020, 9:07 PM
the context here is that I'm trying to understand how
Graph::get
works
👌 1
a

aloof-angle-91616

01/17/2020, 9:07 PM
ok
w

witty-crayon-22786

01/17/2020, 9:07 PM
Yes. That or Graph::create, which is how a node at the root of the graph is created... ie, one that has no parents
a

aloof-angle-91616

01/17/2020, 9:08 PM
node at the root of the graph is created
@witty-crayon-22786 right now, is this equivalent to a
RootRule
?
w

witty-crayon-22786

01/17/2020, 9:09 PM
No. RootRule declares a parameter
h

hundreds-breakfast-49010

01/17/2020, 9:09 PM
so
get
takes three arguments -
src_id
, a
Context
, and
dst_id
✍️ 1
a

aloof-angle-91616

01/17/2020, 9:09 PM
oh ok
h

hundreds-breakfast-49010

01/17/2020, 9:09 PM
in this case, where
some_rule
is being executed by the engine
w

witty-crayon-22786

01/17/2020, 9:09 PM
Graph::create is "equivalent" to a call to scheduler.product_request, essentially
h

hundreds-breakfast-49010

01/17/2020, 9:10 PM
should I expect that at some point there will be an invocation of
get
where the
src_id
is the
Node
that represents this invocation of
some_rule
, and
dst_id
is the
Node
that represents the
ExecuteProcessRequest
intrinsic?
a

aloof-angle-91616

01/17/2020, 9:11 PM
at some point
what point are you thinking of?
(not trying to be difficult)
h

hundreds-breakfast-49010

01/17/2020, 9:12 PM
at some point during the execution of all the rules in a particular pants run
w

witty-crayon-22786

01/17/2020, 9:12 PM
@hundreds-breakfast-49010: yes.
☝️ 1
Argh, sorry. Didn't mean to send to channel
h

hundreds-breakfast-49010

01/17/2020, 9:13 PM
okay
a

aloof-angle-91616

01/17/2020, 9:13 PM
it's a useful convo
w

witty-crayon-22786

01/17/2020, 9:13 PM
All nodes have declared dependencies on other nodes, and this is how that happens.
h

hundreds-breakfast-49010

01/17/2020, 9:13 PM
yeah this is probably useful information for anyone who wants to hack on the guts the engine
so, this is the sort of dependency that happens when a
@rule
internally calls
await Get[Something]
w

witty-crayon-22786

01/17/2020, 9:14 PM
See the last few comments in the other thread though: it's fine to manually thread through the parent information like this, but we might also consider passing it via Task/Thread-local variables.
h

hundreds-breakfast-49010

01/17/2020, 9:14 PM
and there's also a notion of dependency that relies on some
Node
providing
InputType
before this rule can run
@witty-crayon-22786 yeah I just noticed those comments
a

aloof-angle-91616

01/17/2020, 9:14 PM
Task/Thread-local variables.
100000% agree
h

hundreds-breakfast-49010

01/17/2020, 9:14 PM
I'm actually thinking that using Taks/Thread local variables might be the right decision here for this usecase
becuase we don't want to report most of the workunits that could in principle exist
so the same parent Id might need to get passed through a bunch of nodes in the graph before something else uses it
does that logic make sense?
a

aloof-angle-91616

01/17/2020, 9:17 PM
yes
it does
w

witty-crayon-22786

01/17/2020, 9:17 PM
Yep.
h

hundreds-breakfast-49010

01/17/2020, 9:18 PM
the current
workunit_store
code is using the
task_local!
macro to set a
static TASK_PARENT_ID
variable, and updating it in `set_parent_id`/`get_parent_id`
a

aloof-angle-91616

01/17/2020, 9:18 PM
especially i'm thinking task-local variables are what we probably need to have for spans / maybe there's already code that does this?
yes ok
w

witty-crayon-22786

01/17/2020, 9:18 PM
Our logging crate also passes thread/task-local info
👌 1
h

hundreds-breakfast-49010

01/17/2020, 9:19 PM
this is currently not working, and I had a brnach where I thought I added calls in the right places to correctly set parent IDs in
<http://nodes.rs|nodes.rs>
, but I still wound up with parents ids not being set when child nodes needed to read them
which is why I was asking all the questions about the Graph earlier
@witty-crayon-22786 yeah I can see that
<http://logger.rs|logger.rs>
is invocing
thread_local!
a

aloof-angle-91616

01/17/2020, 9:20 PM
but I still wound up with parents ids not being set when child nodes needed to read them
if you can give brief steps to repro we might be able to look into it
h

hundreds-breakfast-49010

01/17/2020, 9:20 PM
er,
task_local
@aloof-angle-91616 let me get back to that code state and I'll push a branch
./pants test tests/python/pants_test/engine:engine
has a new test I added to exercise correct workunit parent ID setting, it's currently failing
oh it's currently failing because I had a deliberate
assert 1 ==2
line, but it's also not actually doing the right thing to begin wiht
w

witty-crayon-22786

01/17/2020, 9:24 PM
Well, you'll also see that the task/thread local info for logging needs to be propagated when we spawn new tasks/threads
Similar here
h

hundreds-breakfast-49010

01/17/2020, 9:34 PM
ah, okay, my patch doesn't do that
so maybe that's the problem
I"m also not sure how to do it, unless I can just copy whatever the logging crate is doing
w

witty-crayon-22786

01/17/2020, 9:35 PM
Yes.
h

hundreds-breakfast-49010

01/17/2020, 9:35 PM
hm, okay there's a distinction between
thread_local
and
task_local
former is rust std latter is a futures thing
w

witty-crayon-22786

01/17/2020, 9:36 PM
Iirc, it defines methods that need to be called in spawned tasks/threads: extending those methods to propagate workunit info as well would make sense
logging/tracing are close buddies.
h

hundreds-breakfast-49010

01/17/2020, 9:37 PM
you're talking about
set_destination
and
get_destination
right?
I wonder then if we don't even want a separate
workunit_store
crate and just want all the workunit related code to live with logging
well that's a bigger architectural change, maybe not
w

witty-crayon-22786

01/17/2020, 9:37 PM
Possibly
h

hundreds-breakfast-49010

01/17/2020, 9:37 PM
or not right now
so
task_executor
is making sure this gets called by invoking
future_with_correct_logging_context
w

witty-crayon-22786

01/17/2020, 9:39 PM
Yep: that's the ticket.
h

hundreds-breakfast-49010

01/17/2020, 9:39 PM
and also some FFI functions call
set_destination
directly
so we want to do something similar here wrt the
workunit_store
methods
w

witty-crayon-22786

01/17/2020, 9:40 PM
Or rename to
future_with_correct_context
Etc. But yea
h

hundreds-breakfast-49010

01/17/2020, 9:41 PM
oh this might be really simple, if
task_executor
can import
workunit_store
okay yeah I think this might be the right solution