https://pantsbuild.org/ logo
Join Slack
Powered by
I think I need to refactor `context.core.command_r...
# development
h
I think I need to refactor 
context.core.command_runners
to let you dynamically change which CommandRunners are used for a particular process, whereas right now it's the same for the whole Pants session *scheduler 🧵
âś… 1
I see two ways of doing this: 1) After plugin initialization, discover all defined 
docker_environment
targets & re-initialize the scheduler to create the command runners. Then, individual Processes can change which is used, or 2) Allow defining a new CommandRunner value on-the-fly
any guidance @witty-crayon-22786?
f
or have a "switch command runner" in Rust that can read an attribute off the 
Process
and switch accordingly
it would sit at or below the nailgun command runner
đź‘€ 1
đź‘Ť 1
h
ohhhh I forgot about Nailgun. Thanks! I think we still have the problem of creating the CommandRunner, which we currently cannot do until plugins have been evaluated. But at least there's precedent for the conditional use of the runner
oh shoot, I just realized another bootstrap problem with our current target modeling. The Target API will complain about unrecognized fields for any Plugin Fields. I'll work on that rn fixed
f
so make a "wrapper command runner" that fails if not configured with a real command runner and forwards execution requests when it is set with one.
then provide an API from Rust to Python to allow doing the late configuration of that command runner.
w
The CommandRunner creation itself shouldn't be a dependent on any values from the environment right? I have been thinking that each process request would differentiate which CommandRunner to use, but that we would create them statically at startup.
h
The Docker CommandRunner needs an 
image
string, so I figured we must know what images the user told us to use. @fast-nail-55400 how hard would that be to change to not be eagerly set?
w
That needs to be fixed to be process driven
h
Then we must discover eagerly what the universe of images are.
After plugin initialization, discover all defined docker_environment targets & re-initialize the scheduler to create the command runners. Then, individual Processes can change which is used, or
Or, we need to move this info into the (bootstrap) options system
After plugin initialization, discover all defined docker_environment targets & re-initialize the scheduler to create the command runners. Then, individual Processes can change which is used, or
But I think I may be off base here. We can discover the docker targets before plugin resolver runs. The plugin resolver does not impact that
w
We don't need to know the universe of images: the docker CommandRunner should dynamically create containers as needed based on Process inputs
h
K, so then we need to modify this:
Copy code
/// `CommandRunner` executes processes using a local Docker client.
pub struct CommandRunner {
  docker: Docker,
  store: Store,
  executor: Executor,
  work_dir_base: PathBuf,
  named_caches: NamedCaches,
  immutable_inputs: ImmutableInputs,
  keep_sandboxes: KeepSandboxes,
  image: String,
}
I can try that refactor right now. I really want to understand this Rust code better
w
Yes. Tom is currently working on a caching version of the docker CommandRunner
đź‘Ť 1
f
Even the existing naive version can use a dynamic image coming from the 
Process
.
I.e., the 
docker::CommandRunner
doesn't need to be initialized with an image.
đź‘Ť 1
h
Cool, I'll refactor this now (more for my understanding than anything). If that's okay with you Tom, re merge conflicts
f
The existing code is the way it is because 
Process
doesn't yet have a Docker image attribute.
đź‘Ť 1
Feel free to refactor as necessary.
I'll incorporate any changes into the caching version if merge conflicts.
h
cool so this was a total non-issue. Thanks for the guidance 🙂 https://github.com/pantsbuild/pants/pull/16758/files Tomorrow morning I will try to figure out hooking up the naive CommandRunner in run_wrapped_node. Which will allow you to use the docker command runner from Python rules
I think I see how to make the docker CommandRunner work. Add 
inner: super::local::CommandRunner,
and then inside 
run()
Copy code
if req.docker_image.is_none() {
      return self.inner.run(context, workunit, req).await;
    }
I'm not certain what order the naive docker command runner should take inside the stack created at 
<http://context.rs|context.rs>
, though. Is it a 
leaf_runner()
? Also, my understanding is we should likely always create the command runner? Only it won't run unless a user explicitly uses a 
docker_environment
target. It would always early return inside 
run()
f
I'd rather not the docker command runner know anything about any inner command runners.
Instead, create a 
DockerLocalSwitchCommandRunner
which owns both a 
docker::ComandRunner
and a 
local::CommandRunner
.
and it chooses which one to invoke based on the image being set or not in the 
Process
an issue you will see is that the nailgun command runner wants to access some fields and methods on 
local::CommandRunner
. I had a solution for that earlier in the development of the naive docker command runner PR, but took it out before landing after discussion with Stu.
h
I'd rather not the docker command runner know anything about any inner command runners.
for my understanding, why not?
f
separation of concerns -- it is a better architecture for these components to be reusable and, more importantly, composable in different ways
something like 
DockerLocalSwitchCommandRunner
is easy to write and is very straight-forward to understand since it would not be many lines
if you couple 
docker::CommandRunner
and 
local::CommandRunner
together, now it is necessary to understand how they interact in order to understand them
đź‘Ť 1
which complicates tests
and also complicates any experimentation we would want to do with composing them in different ways for production use
w
you could do it even higher: in 
src/nodes.rs
and 
src/context.rs
, and give it two CommandRunner stacks.
…but if the field will be consumed from the 
Process
struct, maybe it does make sense for it to be in the CommandRunner.
this was the question we were discussing earlier: does the 
Process
struct have this field, or is it maybe provided as one of the inputs to the 
Process->ProcessResult
intrinsic?
f
you could do it even higher: in 
src/nodes.rs
and 
src/context.rs
, and give it two CommandRunner stacks.
yes. still maintains a good separation of concerns vis-a-vis 
local::CommandRunner
and 
docker::CommandRunner
h
you could do it even higher: in src/nodes.rs and src/context.rs
I'm not following that. Something like change 
context.core.command_runners
so that we have two command_runners stacks?
w
Yes
... except how would the value actually be provided to the runner... unknown. At that point you still either need to change the Process struct, or add an additional argument to the runner I suppose.
ok, yea: i think that this does need to be entirely internal to the CommandRunner. having two stacks doesn’t work without a bunch of interface changes to CommandRunner this would likely be a good factoring, and could maybe even be generic (i.e. take a function which chooses the left or right instance): https://pantsbuild.slack.com/archives/C0D7TNJHL/p1662085572524529?thread_ts=1662078438.700529&amp;cid=C0D7TNJHL … but there is also prior-art in that the nailgun CommandRunner wraps a locally runner and disables itself: https://github.com/pantsbuild/pants/blob/8aa1b5f05e6369cefa9210034ebca0f0a57d2e17/src/rust/engine/process_execution/src/nailgun/mod.rs#L123-L132
h
but there is also prior-art in that the nailgun CommandRunner wraps a locally runner and disables itself:
That was how I already implemented it hehe. Is it fine, or we want me to do 
DockerLocalSwitchCommandRunner
?
w
i think it’s fine.
sorry… i was only half paged in on this and was confusing things.
đź‘Ť 1
h
Also I then got lost inside 
<http://context.rs|context.rs>
trying to figure out the ordering of the command runners. Do you know what order makes sense? With that direction, I can play with implementation For example, 
{RemoteExecutionRunner,LocalRunner -> NailgunRunner} -> LocalCacheRunner -> RemoteCacheRunner
w
for the foreseeable future, mixing nailgun and docker won’t make sense.
but between the two: if a docker image has been requested, that should take precedent, because it’s a request to cross-build, and nailgun is only an optimization
so Docker-wrapping-Nailgun
h
@witty-crayon-22786 is Docker a 
bounded::CommandRunner
?
w
it should be bounded, yea.
with the same shared local bound
so bounded above Docker
h
above
Docker-wrapping-Nailgun
the ordering is tripping me up. Is this right? What we have now: local runner -> (maybe) Nailgun Now: local runner -> (maybe) Nailgun -> Docker
w
if -> means “is called by”, then yes
local runner -> (maybe) Nailgun -> Docker -> bounded
đź‘Ť 1
Open in Slack
PreviousNext
Powered by