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

Add `--[no-]remote-execution` flag #7991

Merged
merged 5 commits into from Jul 3, 2019

Conversation

Projects
None yet
5 participants
@Eric-Arellano
Copy link
Contributor

commented Jul 1, 2019

Problem

We want to allow users to permanently configure all remoting options in their pants.ini, without that config triggering Pants always using remoting. Tangibly, we are trying to land #7990 to setup our own usage and can't get green CI because Pants is trying to remote things it shouldn't.

Instead, users should be able to turn on remoting at the per-invocation level, just like we have --enable-pantsd.

Solution

Introduce --[no-]remote-execution. For now, this defaults to False as this is an alpha feature and it does not work without other remoting config.

Result

Users can have all their remoting config setup and still be able to entirely run locally. Also now more explicit when you are and are not remoting.

@stuhood

stuhood approved these changes Jul 2, 2019

Copy link
Member

left a comment

Thanks. Please wait for a sanity check from @illicitonion before merging.

Show resolved Hide resolved src/python/pants/option/global_options.py Outdated
Show resolved Hide resolved src/rust/engine/src/context.rs Outdated

@Eric-Arellano Eric-Arellano changed the title Add `--enable-remoting` flag Add `--[no-]remote-execution` flag Jul 2, 2019

@illicitonion
Copy link
Contributor

left a comment

This works for me, but it may be worth thinking about whether this is really a boolean, or will be an enum in the future? In particular, when speculation is a thing, will we want:
--remote-execution --speculate-execution
or will we want
--execution-strategy={local,remote,speculative,...}
?

Show resolved Hide resolved src/rust/engine/src/lib.rs Outdated
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

In particular, when speculation is a thing, will we want: ... --execution-strategy={local,remote,speculative}

That approach sounds great to me! Default to local.

Though, this assumes that there are instances where users would only want to remote and not speculate? Do we anticipate that being the case?

--

Closely related, in Slack Stu mentioned the option for the speculate timeout threshold. I think that is should be separate from this option here, but this is one possible way of doing this:

./pants --remoting --speculation-local-timeout=0 to mean always remote, don't speculate.

--

cc @stuhood @hrfuller

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This works for me, but it may be worth thinking about whether this is really a boolean, or will be an enum in the future? In particular, when speculation is a thing, will we want:
--remote-execution --speculate-execution
or will we want
--execution-strategy={local,remote,speculative,...}
?

There was some discussion of this in slack (here), and my takeaway was that if remoting is enabled, we should have a good set of defaults for that case which will include speculation (by default). So I leaned toward the boolean flag.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

my takeaway was that if remoting is enabled, we should have a good set of defaults for that case which will include speculation (by default). So I leaned toward the boolean flag.

Agreed. If you have remoting, by default it makes sense to have speculation also enabled.

So long as we have some mechanism to turn off speculation, which we can do via something like --speculation-local-timeout=0.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:enable-remoting branch from 7e36967 to 0ddf3af Jul 2, 2019

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I'm largely indifferent on the boolean vs enum question, happy to do whatever other people prefer :)

@jsirois

jsirois approved these changes Jul 3, 2019

@Eric-Arellano Eric-Arellano merged commit 53d42df into pantsbuild:master Jul 3, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:enable-remoting branch Jul 3, 2019

Eric-Arellano added a commit that referenced this pull request Jul 4, 2019

Don't use remote store when --no-remote-execution specified (#8010)
#7991 added the flag `--[no-]remote-exeuction` to toggle on and off remote behavior. There, out of error, it was only used to toggle the remote execution runner, not also the remote store.

This is causing #7990 to continue to fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.