Local caching CommandRunner has default-on flag #8040
Conversation
illicitonion
requested review from
stuhood,
hrfuller,
blorente and
Eric-Arellano
stuhood
force-pushed
the
dwagnerhall/local-process-execution-cache/wireup
branch
from
ebb33bc to
8904ae4
Compare
|
Hm, the panic on the OSX bootstrap shard repros after a rebase. |
|
Pushed an edit which exposed the issue. Looks like we need to increase the file handle count on the OSX bootstrap shard. It looks like we only do on the test shard(s). |
build-support/bin/ci.py
Outdated
| subprocess.run(["./pants", "binary", "src/python/pants/bin:pants_local_binary"], check=True) | ||
| env = dict(os.environ) | ||
| env['RUST_BACKTRACE'] = "1" | ||
| subprocess.run(["./pants", "binary", "src/python/pants/bin:pants_local_binary"], check=True, env=env) |
There was a problem hiding this comment.
Btw, cool Py3 only idiom to do this:
...check=True, env={**os.environ, 'RUST_BACKTRACE': '1'})
illicitonion
force-pushed
the
dwagnerhall/local-process-execution-cache/wireup
branch
from
1b14726 to
a0fe0b8
Compare
illicitonion
force-pushed
the
dwagnerhall/local-process-execution-cache/wireup
branch
from
a0fe0b8 to
02d0179
Compare
| @@ -435,6 +438,8 @@ def register_bootstrap_options(cls, register): | |||
| 'and fall back to the local host if remote calls take longer than the speculation timeout.\n' | |||
| '`none`: Do not speculate about long running processes.', | |||
| advanced=True) | |||
| register('--process-execution-use-local-cache', type=bool, default=True, advanced=True, | |||
There was a problem hiding this comment.
I can see us having more --process-execution-prefixed flags. It would maybe be worth adding a TODO like:
| register('--process-execution-use-local-cache', type=bool, default=True, advanced=True, | |
| # TODO Make a subsystem to namespace `--process-execution`-prefixed flags. | |
| register('--process-execution-use-local-cache', type=bool, default=True, advanced=True, |
There was a problem hiding this comment.
Also, I don't think it's relevant here, but passing daemon=True will make this option invalidate the daemon, in case that's something we want to do.
There was a problem hiding this comment.
We have this TODO further up in this file. But currently it's not possible to have a "bootstrap" subsystem.
Also, I don't think it's relevant here, but passing daemon=True will make this option invalidate the daemon, in case that's something we want to do.
daemon=True is the default.
There was a problem hiding this comment.
We currently need this to be daemon=True (the default) because we statically create CommandRunners early in the daemon lifecycle, and have no way to re-create them, or pass their config around dynamically. We may want to change that at some point :) (cc @hrfuller)
| if process_execution_use_local_cache { | ||
| let process_execution_store = ShardedLmdb::new( | ||
| local_store_dir2.join("processes"), | ||
| 5 * 1024 * 1024 * 1024, |
There was a problem hiding this comment.
nit: name this magical number.
| cache_key_gen_version: remote_execution_process_cache_namespace.clone(), | ||
| platform_properties: remote_execution_extra_platform_properties.clone(), | ||
| }; | ||
|
|
||
| let mut command_runner: Box<dyn process_execution::CommandRunner> = |
There was a problem hiding this comment.
Why not have an if/else here, instead of creating the mutable variable?
There was a problem hiding this comment.
I think my instinct here is because this is more clear in the face of multiple wrappers:
let mut val = foo;
if wrap {
val = do_wrap(val);
}
if wrap_in_other {
val = do_other_wrap(val);
}but I don't have a strong preference either way, if you do.
illicitonion
deleted the
dwagnerhall/local-process-execution-cache/wireup
branch
No description provided.