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

Extract out `resolve_requirements` V2 rule for creating PEXes with requirements #7846

Merged

Conversation

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

commented Jun 4, 2019

Problem

python_test_runner.py does too much right now.

One of its main pieces of logic that should be factored out is creating a PEX with the desired requirements, entry point, and interpreter constraints. This will presumably be useful for other V2 rules, so should not be coupled to the Pytest runner.

Also, it was found in #7795 that this requirements PEX is causing ~75% of the performance issues for the V2 Pytest runner. Having it extracted out into its own rule will make it easier to develop, test, and optimize.

Solution

Create a generic rule, along with dedicated tests.

@Eric-Arellano Eric-Arellano changed the title WIP: Extract out `resolve_requirements` rule for creating PEXes WIP: Extract out `resolve_requirements` rule for creating PEXes with requirements Jun 4, 2019

@Eric-Arellano
Copy link
Contributor Author

left a comment

Final step before this will be ready to review is figuring out testing, both what to do with the original python_test_runner test for interpreter constraints and what/how to test this new file.

def test_interpreter_constraints_parsing(self):
python_setup = global_subsystem_instance(PythonSetup)
target_adaptors = [
# NB: This target will use the global --python-setup-interpreter-constraints.
PythonTargetAdaptor(compatibility=None),
PythonTargetAdaptor(compatibility=["CPython>=400"]),
]
self.assertEqual(
parse_interpreter_constraints(python_setup, target_adaptors),
[
"--interpreter-constraint", "CPython>=2.7,<3",
"--interpreter-constraint", "CPython>=3.6,<4",
"--interpreter-constraint", "CPython>=400"
]
)

I don't know how to meaningfully test this file. I think we would want ~integration tests that set up the entire PEX with dependencies, then confirm the PEX works? Maybe test the different permutations like setting an entry point and setting interpreter constraints?

Eric-Arellano added some commits Jun 4, 2019

Delete test_python_test_runner.py
No longer deals with interpreter constraints, plus the soon-to-be-newly moved over integration test already covers things.

Eric-Arellano added a commit that referenced this pull request Jun 5, 2019

Move V2 test runner integration test into proper location of `backend…
…/python` folder (#7847)

`test_test_integration.py` was really an integration test for the V2 Pytest runner, not for the generic V2 test runner. For example, these tests would not make sense in the context of running Java tests. Related code should live near each other to make things more discoverable.

Also, this changes the `BUILD` for `pants_test/rules` to create an entry for each test file and to properly include each test's dependencies, which were quite out of date. Having a target per test file makes it easier to keep dependencies up to date and makes it easier to run a specific test suite.

This is prework for #7846.

@Eric-Arellano Eric-Arellano changed the title WIP: Extract out `resolve_requirements` rule for creating PEXes with requirements Extract out `resolve_requirements` rule for creating PEXes with requirements Jun 5, 2019

@Eric-Arellano Eric-Arellano changed the title Extract out `resolve_requirements` rule for creating PEXes with requirements Extract out `resolve_requirements` V2 rule for creating PEXes with requirements Jun 5, 2019

@Eric-Arellano Eric-Arellano marked this pull request as ready for review Jun 5, 2019

@Eric-Arellano Eric-Arellano requested review from stuhood, benjyw and illicitonion Jun 5, 2019

Show resolved Hide resolved src/python/pants/backend/python/rules/python_test_runner.py
request = ExecuteProcessRequest(
argv=(python_binary, './{}'.format(output_pytest_requirements_pex_filename)),
argv=("python", './{}'.format(output_pytest_requirements_pex_filename)),

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 5, 2019

Contributor

I am not entirely sure this is kosher; I think it happens to work locally because of how we happen to have implemented $PATH suppression (

fn new<S: AsRef<OsStr>>(program: S) -> StreamedHermeticCommand {
let mut inner = Command::new(program);
inner
.env_clear()
// It would be really nice not to have to manually set PATH but this is sadly the only way
// to stop automatic PATH searching.
.env("PATH", "");
StreamedHermeticCommand { inner }
}
), but according to the remote execution API $PATH shouldn't be used to look up argv[0] even if it's set.

Sadly I suspect the technically correct answer here is a little wrapper shell script along the lines of:

exec "$@"

which we'd invoke lookup_on_path python ./output...

but I guess we can punt on this until it becomes a problem.

(We could also make our local process execution resemble the spec more closely by before running inspecting "does argv[0] contain a /, if not, error"?)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 6, 2019

Author Contributor

This works on remoting and local. I made this change in #7844 specifically to fix the remoting case.

The reason it works (I think) is that we explicitly set the env to include the entry PATH: interpreter_search_paths.

--

Are you proposing having this be something like /usr/bin/python? Or parametrizing it as an option so that you can set which you want?

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 6, 2019

Contributor

As per https://github.com/bazelbuild/remote-apis/blob/a5c577357528b33a4adff88c0c7911dd086c6923/build/bazel/remote/execution/v2/remote_execution.proto#L433-L436 the execution system shouldn't do $PATH lookup; in particular, it's totally valid for it to execv and not execvp... Our local execution happens to execvp because that's what rust conveniently exposes to us, but it shouldn't...

I'm surprised RBE does, if it does... Does it?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 7, 2019

Author Contributor

I'm surprised RBE does, if it does... Does it?

I haven't worked on remoting in two weeks and the context switch to verify this is going to take too much time to be able to quickly check, but I know when working on #7844 the change allowed me to get further than before. Originally, iirc it would complain it could not find the file /Users/eric/DocsLocal/code/projects/pants/build-support/pants_dev_deps.py36.venv/bin/python; with the change, the error was just that Pex could not resolve requirements.

Will hopefully be done with Py3 work soon and be able to go back to remoting!

Show resolved Hide resolved src/python/pants/backend/python/rules/python_test_runner.py
Show resolved Hide resolved src/python/pants/backend/python/rules/resolve_requirements.py
interpreter_constraint_args.extend(["--interpreter-constraint", text_type(constraint)])

# NB: we use the hardcoded and generic bin name `python`, rather than something dynamic like
# `sys.executable`, to ensure that the interpreter may be discovered both locally and in remote

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 5, 2019

Contributor

As above

@stuhood

stuhood approved these changes Jun 5, 2019

Copy link
Member

left a comment

Thanks!

As @illicitonion pointed out, there will be some issues here in the presence of remoting... would be great to get to a place where we are hitting that API and beginning to shake out those issues.

Eric-Arellano added some commits Jun 6, 2019

Remove requirements from ResolvedRequirementsPex
Instead, we store only the information that is newly unique. If the user wants to know the requirements, they can either look at what they passed to the Request or even open up PEX-INFO.
Fix Python 2 text_type issues.
So done with Python 2!
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Merging without another CI burn because I only changed a comment and we have a backlog. Ran pre-commit githook first.

@Eric-Arellano Eric-Arellano merged commit 530a06c into pantsbuild:master Jun 7, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:resolve-requirements-rule branch Jun 7, 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.