https://pantsbuild.org/ logo
Join Slack
Powered by
`some_rule` is doing to wind up a `Task` node, rig...
# development
h
some_rule
is doing to wind up a 
Task
node, right?
a
yes
in 
src/python/pants/engine/rules.py
you can see how it goes:
w
Yes.
a
@rule => _make_rule() => TaskRule
no difference in behavior for async rules
h
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
i think so, let me read that again
could you elaborate or restate?
h
the context here is that I'm trying to understand how 
Graph::get
works
👌 1
a
ok
w
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
node at the root of the graph is created
@witty-crayon-22786 right now, is this equivalent to a 
RootRule
?
w
No. RootRule declares a parameter
h
so 
get
takes three arguments - 
src_id
, a 
Context
, and 
dst_id
✍️ 1
a
oh ok
h
in this case, where 
some_rule
is being executed by the engine
w
Graph::create is "equivalent" to a call to scheduler.product_request, essentially
h
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
at some point
what point are you thinking of?
(not trying to be difficult)
h
at some point during the execution of all the rules in a particular pants run
w
@hundreds-breakfast-49010: yes.
☝️ 1
Argh, sorry. Didn't mean to send to channel
h
okay
a
it's a useful convo
w
All nodes have declared dependencies on other nodes, and this is how that happens.
h
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
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
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
Task/Thread-local variables.
100000% agree
h
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
yes
it does
w
Yep.
h
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
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
Our logging crate also passes thread/task-local info
👌 1
h
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
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
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
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
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
Yes.
h
hm, okay there's a distinction between 
thread_local
and 
task_local
former is rust std latter is a futures thing
w
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
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
Possibly
h
or not right now
so 
task_executor
is making sure this gets called by invoking 
future_with_correct_logging_context
w
Yep: that's the ticket.
h
and also some FFI functions call 
set_destination
directly
so we want to do something similar here wrt the 
workunit_store
methods
w
Or rename to 
future_with_correct_context
Etc. But yea
h
oh this might be really simple, if 
task_executor
can import 
workunit_store
okay yeah I think this might be the right solution
Open in Slack
PreviousNext
Powered by