Navigation Menu

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

Integrate PEX interpreter selection based on target-level interpreter compatibility constraints #5160

Merged
merged 44 commits into from Dec 20, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
24bf2cf
WIP: First pass at integrating interpreter selection into Pants build…
Dec 5, 2017
9567613
Bump pex version and add python 3 binary/run/test example in testproj…
Dec 5, 2017
243522c
Cleanup docstrings/comments and minor refactoring
Dec 5, 2017
0f63542
Rename one variablefor clarity
Dec 5, 2017
ad3f714
Add guard for only working with targets that have python sources
Dec 5, 2017
7a45792
Refactoring based on kwlzn suggestions
Dec 6, 2017
ac28d60
Add integration testing for pants run and unit tests for interpreter …
Dec 7, 2017
b83a2f7
Add unit/integration tests and testing utils for interpreter selection
Dec 7, 2017
28cbdf7
Remove unused imports
Dec 8, 2017
c99b4d0
Fix test breakage due to interpreter cache filtering in setup method
Dec 8, 2017
50df988
Cleanup whitespace and copyright year
Dec 8, 2017
212f87c
Interpreter cache test modification for CI
Dec 8, 2017
b7120a0
Travis debug 1
Dec 11, 2017
7da186e
Travis debug 2
Dec 11, 2017
1d37a89
Travis debug 3
Dec 11, 2017
f69c5f0
Travis debug 4
Dec 11, 2017
82e6e74
Travis debug 5
Dec 11, 2017
a2eefe9
Remove debug statements
Dec 11, 2017
23e2417
Travis debug 5
Dec 11, 2017
4d1b606
Cleanup tests and refactor pexrc util function
Dec 11, 2017
b15dbe3
Changes to comments to manually rekick CI
Dec 12, 2017
71fd8c0
Remove unused imports
Dec 12, 2017
063bb0d
Remove inaccurate comments
Dec 12, 2017
95b102f
Testing util changes per kwlzn requests
Dec 14, 2017
6031d7a
Minor cleanup and removal of unused import.
Dec 14, 2017
a9fad7c
Fix 2 lint errors
Dec 14, 2017
8ac4b70
Debug failing CI tests and lint errors
Dec 15, 2017
da2890d
Add clean-all statement to debug flaky CI test
Dec 15, 2017
af61c1e
Change interpreter version and pexrc location for test
Dec 15, 2017
d987cc6
Refactor tests per jsirois requests and fix bug
Dec 15, 2017
9ea24e2
Fix python3 symlink issues to pass CI
Dec 15, 2017
7302859
Fix lint errors and debug flaky CI
Dec 16, 2017
1196098
Travis debug
Dec 16, 2017
4ef2aef
Travis debug 2
Dec 16, 2017
cbf0929
Travis Debug 3
Dec 16, 2017
a402d44
Travis debug 4
Dec 17, 2017
7910e9d
Travis debug 5
Dec 17, 2017
364c5f9
Travis debug 6
Dec 17, 2017
a6fcb24
Travis debug 7
Dec 17, 2017
0e5be87
Travis debug 8
Dec 17, 2017
1ed2438
Fix lint error
Dec 18, 2017
82e6788
Remove pyenv test dir from git ignore
Dec 18, 2017
bd71291
Fix classmethod bug
Dec 20, 2017
4e8baa8
Address a few test assertions and clean up helper method for interpre…
Dec 20, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion 3rdparty/python/requirements.txt
Expand Up @@ -13,7 +13,7 @@ packaging==16.8
pathspec==0.5.0
parameterized==0.6.1
pep8==1.6.2
pex==1.2.13
pex==1.2.15
psutil==4.3.0
pyflakes==1.1.0
Pygments==1.4
Expand Down
22 changes: 21 additions & 1 deletion src/python/pants/backend/python/interpreter_cache.py
Expand Up @@ -11,6 +11,7 @@
from pex.interpreter import PythonIdentity, PythonInterpreter
from pex.package import EggPackage, Package, SourcePackage
from pex.resolver import resolve
from pex.variables import Variables

from pants.backend.python.targets.python_target import PythonTarget
from pants.base.exceptions import TaskError
Expand Down Expand Up @@ -39,6 +40,22 @@ def _matching(cls, interpreters, filters):
if cls._matches(interpreter, filters):
yield interpreter

@memoized_property
def pex_python_paths(self):
"""A list of paths to Python interpreter binaries as defined by a
PEX_PYTHON_PATH defined in either in '/etc/pexrc', '~/.pexrc', or ./.pexrc'.
PEX_PYTHON_PATH defines a colon-seperated list of paths to interpreters
that a pex can be built and ran against.

:return: paths to interpreters as specified by PEX_PYTHON_PATH
:rtype: list
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pants docstring style standard should look like this (note the indentation):

"""A one line summary with punctuation.

Additional descriptions. Would go here. And might
wrap etc.

:return: A description of the thing it returns.
:rtype: list
"""

ppp = Variables.from_rc().get('PEX_PYTHON_PATH', '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just .get('PEX_PYTHON_PATH').

if ppp:
return ppp.split(os.pathsep)
else:
return []

def __init__(self, python_setup, python_repos, logger=None):
self._python_setup = python_setup
self._python_repos = python_repos
Expand Down Expand Up @@ -127,11 +144,14 @@ def setup(self, paths=(), filters=(b'',)):
:returns: A list of cached interpreters
:rtype: list of :class:`pex.interpreter.PythonInterpreter`
"""
pex_python_paths = self.pex_python_paths
# We filter the interpreter cache itself (and not just the interpreters we pull from it)
# because setting up some python versions (e.g., 3<=python<3.3) crashes, and this gives us
# an escape hatch.
filters = filters if any(filters) else self._python_setup.interpreter_constraints
if not any(filters):
filters = self._python_setup.interpreter_constraints
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like there's a tension here between the notion of global interpreter constraints defined on PythonSetup and target level constraints as it pertains to interpreter setup - and I'd expect to see something connecting the dots at least at a validation level.

what happens now if the global interpreter constraints conflict with target level constraints (e.g. if global constraints only ensure a python2.7 interpreter in the interpreter cache, but then a target has constraints pinning to python3.6)? what should happen?

seems to me like currently, this might entirely bypass the interpreter cache which could lead to breakage down the road. a test around that case (and any required fixes) would be good.

setup_paths = (paths
or pex_python_paths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would directly reference self.pex_python_paths here vs using the temporary pex_python_paths var once.

or self._python_setup.interpreter_search_paths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably include a note on the option for [python-setup] interpreter-search-paths that a pexrc config defining PPP will supersede it.

or os.getenv('PATH').split(os.pathsep))

Expand Down
4 changes: 3 additions & 1 deletion src/python/pants/backend/python/subsystems/python_setup.py
Expand Up @@ -64,7 +64,9 @@ def register_options(cls, register):
'If unspecified, a standard path under the workdir is used.')
register('--interpreter-search-paths', advanced=True, type=list, default=[],
metavar='<binary-paths>',
help='A list of paths to search for python interpreters.')
help='A list of paths to search for python interpreters. Note that if a PEX_PYTHON_PATH '
'variable is defined in a pexrc file, those interpreter paths will take precedence over '
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right precedence, and why? Should the paths be merged instead?

I don't have an opinion, just trying to understand the design choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use /etc/pexrc both at build-time and at (pex) run-time for a globally consistent view of "blessed" interpreters, so the original thinking was to align strictly to that or else fallback to pants' config.. but I could see merging being useful for testing purposes.

will leave it up to @CMLivingston to make the final decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it makes the most sense to give it precedence over the setup.interpreter_constraints so we know we are using the same interpreter across all tasks and the runtime of the pex in play. I could see a situation where an interpreter is selected for subsequent tasks because it is a lower version than all interpreters in PPP, and it is possible that this could lead to unexpected errors (although tbh I don't know what they would be exactly because the interpreter product is used in multiple tasks).

For now I will go with giving PPP precedence and writing a log.warn if interpreter-search-paths is passed to let the user know that they have a pexrc somewhere. To me, if someone defines a pexrc, they are saying that they want created pexes to use this config and as of right now I don't see any reason why the Pants case should be an exception.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

'this option.')

@property
def interpreter_constraints(self):
Expand Down
Expand Up @@ -103,6 +103,10 @@ def _create_binary(self, binary_tgt, results_dir):

builder = PEXBuilder(path=tmpdir, interpreter=interpreter, pex_info=pex_info, copy=True)

# Add binary target's interpreter compatibility to pex info
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Period at end of sentence.

for constraint in binary_tgt.compatibility:
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can scrap this:

The closure iterated over a few lines down includes binary_tgt itself. And has_python_sources is true for PythonBinary targets. So as far as I can tell binary_tgt's constraints will be added there, and there's no need to do so here.

builder.add_interpreter_constraint(constraint)

if binary_tgt.shebang:
self.context.log.info('Found Python binary target {} with customized shebang, using it: {}'
.format(binary_tgt.name, binary_tgt.shebang))
Expand All @@ -116,6 +120,10 @@ def _create_binary(self, binary_tgt, results_dir):
for tgt in binary_tgt.closure(exclude_scopes=Scopes.COMPILE):
if has_python_sources(tgt) or has_resources(tgt):
source_tgts.append(tgt)
# Add target's interpreter compatibility constraints to pex info
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, and throughout.

if has_python_sources(tgt):
for constraint in tgt.compatibility:
builder.add_interpreter_constraint(constraint)
elif has_python_requirements(tgt):
req_tgts.append(tgt)

Expand Down
Expand Up @@ -16,6 +16,7 @@
from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
from pants.backend.python.targets.python_target import PythonTarget
from pants.backend.python.tasks2.gather_sources import GatherSources
from pants.backend.python.tasks2.pex_build_util import has_python_sources
from pants.backend.python.tasks2.resolve_requirements import ResolveRequirements
from pants.backend.python.tasks2.resolve_requirements_task_base import ResolveRequirementsTaskBase
from pants.build_graph.address import Address
Expand Down Expand Up @@ -141,6 +142,11 @@ def create_pex(self, pex_info=None):

with safe_concurrent_creation(path) as safe_path:
builder = PEXBuilder(safe_path, interpreter, pex_info=pex_info)
# Add target interpreter compatibilities to pex info
for rt in relevant_targets:
if has_python_sources(rt):
for constraint in rt.compatibility:
builder.add_interpreter_constraint(constraint)
builder.freeze()

return WrappedPEX(PEX(os.path.realpath(path), interpreter), interpreter)
@@ -0,0 +1,42 @@
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

# This BUILD file is intended to be an example of python 3 compatible targets.
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

# To successfully build and run this binary target, the user will need to define a
# .pexrc file in /etx/pexrc, ~/.pexrc, or in a .pexrc in the pants root dir. The pexrc
# will need to contain a PEX_PYTHON_PATH variable containing an absolute path to a
# python 3 interpreter.
#
# An example of a PEX_PYTHON_PATH variable in a pexrc:
# PEX_PYTHON_PATH=/path/to/python2.7:/path/to/python3.6
#
# Note that the basename of the python binaries specified must either be 'python' or
# 'pythonX.Y'. Specifing only major version (i.e. python3) will be ignored by Pants
# interpreter filtration.
#

python_binary(
name='main',
source='main.py',
compatibility=['CPython>3'],
dependencies=[
':lib'
]
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest to add a CPython<3 target here and run pants test python_3_binary:

python_library(
name='lib',
sources=['lib.py'],
compatibility=['CPython>3']
)

python_tests(
name='test',
sources=[
'test.py',
],
dependencies=[
':main'
],
compatibility=['CPython>3']
)
@@ -0,0 +1,19 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from __future__ should no longer be needed with python3 code afaict.


import sys


# A simple example to include in a python 3 binary target

def say_something():
print('I am a python 3 library method.')
assert 1/2 == 0.5

# Return 1/2 for testing the `./pants run` task
# Note that 1/2 = 0 in python 2 and 1/2 = 0.5 in python 3
return 1/2
@@ -0,0 +1,23 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

import sys

from interpreter_selection.python_3_binary.lib import say_something


# A simple example to test building/running/testing a python 3 binary target


def main():
v = sys.version_info
print(sys.executable)
print('%d.%d.%d' % v[0:3])
return say_something()

if __name__ == '__main__':
main()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

@@ -0,0 +1,15 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

import sys

from interpreter_selection.python_3_binary.main import main

def test_main():
print(sys.executable)
# Note that 1/2 = 0 in python 2 and 1/2 = 0.5 in python 3
assert main() == 0.5