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

Set up pants.remote.ini for remoting Python unit tests #7990

Merged

Conversation

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

commented Jul 1, 2019

Per #7649, we are working to remote unit tests in CI.

This implements step #2, which allows us to run unit tests locally (several, but not all tests). Here, we set up pants.remote.ini so that users only must point to it and provide the authentication token.

pants.ini Outdated
# TODO(#7735): We need to add this entry for remoting to be able to discover a valid
# interpreter, because <PATH> will refer to the host PATH and not the remote value. This value
# was found by inspecting the docker image for remoting.
"/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/python3.6/bin:/usr/local/go/bin",

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 1, 2019

Author Contributor

I did not comment this out because it should not cause failures, at least in most instances. If any of these entries do not exist on the machine, then Pex / Pants will just try the other entries in <PEXRC> and <PATH> (which is the host PATH).

By leaving it in, it's one less thing that users have to change to run remoting.

I see why we might want to comment it out by default, though, and am happy to do so if you think it's worth it.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 2, 2019

Member

IMO, we should either resolve the platform issue, or (if this is really a good default) submit it as the default to pex.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 2, 2019

Author Contributor

or (if this is really a good default) submit it as the default to pex.

What do you mean?

we should either resolve the platform issue

I'd prefer we don't let the platform issue block this PR, because we seem to still be at least a few days away from a good solution. Agreed that it will be great to remove this hack asap.

pants.ini Outdated
remote_execution_extra_platform_properties: [
# This allows network requests, e.g. to resolve dependencies with Pex.
"dockerNetwork=standard",
"container-image=docker://marketplace.gcr.io/google/rbe-ubuntu16-04@sha256:da0f21c71abce3bbb92c3a0c44c3737f007a82b60f8bd2930abc55fe64fc2729",

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 1, 2019

Author Contributor

Stu and I discussed instead using our custom Centos7 image. For now, we'll stick with this because it's Ubuntu 16 so closer to what we have in CI. It's possible that we will need to revisit this sometime soon.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 2, 2019

Member

We should use whatever is closest to our travis environment probably... the Centos6/7 would primarily be relevant to wheel building, but our unit tests successfully run PEX resolves directly in the travis environment currently, and it would probably reduce complexity a bit to not change the platform we're running unit/integration tests on right now.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Blocked by #7991.

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Works for me, as long as @stuhood's concerns are addressed :)

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

Add `--[no-]remote-execution` flag (#7991)
### 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.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:remoting-local-unit-tests branch from 38fac31 to 713f24c Jul 3, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

This is now ready for review.

Show resolved Hide resolved pants.ini Outdated
@stuhood

stuhood approved these changes Jul 4, 2019

@Eric-Arellano Eric-Arellano changed the title Set up pants.ini for remoting Python unit tests Set up pants.remote.ini for remoting Python unit tests Jul 4, 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.

@Eric-Arellano Eric-Arellano merged commit b2602ea into pantsbuild:master Jul 4, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:remoting-local-unit-tests branch Jul 4, 2019

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.