Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local caching CommandRunner has default-on flag #8040

Conversation

@illicitonion
Copy link
Contributor

commented Jul 11, 2019

No description provided.

@stuhood
Copy link
Member

left a comment

Thanks!

@Eric-Arellano
Copy link
Contributor

left a comment

Woohoo!

@stuhood stuhood force-pushed the twitter:dwagnerhall/local-process-execution-cache/wireup branch from ebb33bc to 8904ae4 Jul 12, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Hm, the panic on the OSX bootstrap shard repros after a rebase.

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

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).

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)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 12, 2019

Contributor

Btw, cool Py3 only idiom to do this:

...check=True, env={**os.environ, 'RUST_BACKTRACE': '1'})

stuhood added a commit that referenced this pull request Jul 12, 2019

Add lmdb_store to travis cache (#8042)
This will allow local process execution to be cached across runs once #8040 lands.

See #8041 for discussion of keeping its size down.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/local-process-execution-cache/wireup branch from 1b14726 to a0fe0b8 Jul 15, 2019

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/local-process-execution-cache/wireup branch from a0fe0b8 to 02d0179 Jul 18, 2019

@blorente
Copy link
Contributor

left a comment

This is cool, thanks! The comments are all nits, feel free to ignore them.

@@ -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,

This comment has been minimized.

Copy link
@blorente

blorente Jul 18, 2019

Contributor

I can see us having more --process-execution-prefixed flags. It would maybe be worth adding a TODO like:

Suggested change
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,

This comment has been minimized.

Copy link
@blorente

blorente Jul 18, 2019

Contributor

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.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 18, 2019

Member

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.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Author Contributor

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,

This comment has been minimized.

Copy link
@blorente

blorente Jul 18, 2019

Contributor

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> =

This comment has been minimized.

Copy link
@blorente

blorente Jul 18, 2019

Contributor

Why not have an if/else here, instead of creating the mutable variable?

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Author Contributor

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.

@hrfuller
Copy link
Contributor

left a comment

Cool! LGTM.

@illicitonion illicitonion merged commit 4435b8e into pantsbuild:master Jul 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/local-process-execution-cache/wireup branch Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.