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 42 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion 3rdparty/python/requirements.txt
Original file line number Diff line number Diff line change
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
18 changes: 18 additions & 0 deletions src/python/pants/backend/python/interpreter_cache.py
Original file line number Diff line number Diff line change
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

@classmethod
Copy link
Member

Choose a reason for hiding this comment

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

should be able to use @memoized_method here to avoid doing this lookup 2x+ per run

def pex_python_paths(cls):
"""A list of paths to Python interpreter binaries as defined by a
PEX_PYTHON_PATH defined in either in '/etc/pexrc', '~/.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')
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 @@ -132,6 +149,7 @@ def setup(self, paths=(), filters=(b'',)):
# an escape hatch.
filters = filters if any(filters) else self._python_setup.interpreter_constraints
setup_paths = (paths
or self.pex_python_paths()
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,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.
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
Original file line number Diff line number Diff line change
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)
3 changes: 3 additions & 0 deletions src/python/pants/backend/python/tasks2/select_interpreter.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ def execute(self):
python_tgts = self.context.targets(lambda tgt: isinstance(tgt, PythonTarget))
fs = PythonInterpreterFingerprintStrategy()
with self.invalidated(python_tgts, fingerprint_strategy=fs) as invalidation_check:
if PythonSetup.global_instance().interpreter_search_paths and PythonInterpreterCache.pex_python_paths:
Copy link
Contributor

Choose a reason for hiding this comment

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

PythonInterpreterCache.pex_python_paths is defined above as an instance method, here you access it as a class attribute. Need some fixup in one place or the other, but I expect you want to fix PythonInterpreterCache.pex_python_paths to be a memoized classmethod and call PythonInterpreterCache.pex_python_paths() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it to be a class method call, however I tried memoizing with multiple decorators from pants.util.memo (memoized, memoized_method) in tandem with @classmethod and none were able to function properly in testing and manual runs. I would prefer to follow up in a separate PR to address this if the workaround is not clear here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still broken:

if PythonSetup.global_instance().interpreter_search_paths and PythonInterpreterCache.pex_python_paths:

Did you forget to push a diff?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the secret sauce is the following ordering:

@classmethod
@memoized_method
def foo(cls):
  ...

A git grep -n -C1 "@memoized_method" | grep -C1 "@classmethod" will turn up a bunch of examples. I'm not hung up on this however for this PR.

self.context.log.warn("Detected both PEX_PYTHON_PATH and --python-setup-interpreter-search-paths. "
"Ignoring --python-setup-interpreter-search-paths.")
# If there are no relevant targets, we still go through the motions of selecting
# an interpreter, to prevent downstream tasks from having to check for this special case.
if invalidation_check.all_vts:
Expand Down
2 changes: 1 addition & 1 deletion testprojects/src/python/interpreter_selection/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ python_library(
sources = ['echo_interpreter_version.py'],
dependencies = [],
# Play with this to test interpreter selection in the pex machinery.
compatibility = ['CPython>=2.7,<3', 'CPython>=3.3']
compatibility = ['CPython>=2.7,<4']
)

python_binary(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# 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 and
# to serve as a set of test cases for interpreter selection based on different targets.
# 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_py3',
source='main_py3.py',
compatibility=['CPython>3'],
dependencies=[
':lib_py3'
]
)

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

python_tests(
name='test_py3',
sources=[
'test_py3.py',
],
dependencies=[
':main_py3'
],
compatibility=['CPython>3']
)

python_binary(
name='main_py2',
source='main_py2.py',
compatibility=['CPython>2.7.6,<3'],
dependencies=[
':lib_py2'
]
)

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

python_tests(
name='test_py2',
sources=[
'test_py2.py',
],
dependencies=[
':main_py2'
],
compatibility=['CPython>2.7.6,<3']
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# 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)


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

def say_something():
print('I am a python 2 library method.')
# Note that ascii exists as a built-in in Python 3 and
# does not exist in Python 2.
try:
ret = ascii
except NameError:
ret = None
assert ret is None
return ret
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

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

def say_something():
print('I am a python 3 library method.')
return ascii
Original file line number Diff line number Diff line change
@@ -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_selection_testing.lib_py2 import say_something


# A simple example to test building/running/testing a python 2 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()
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import sys

from interpreter_selection.python_3_selection_testing.lib_py3 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()
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# 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_selection_testing.main_py2 import main


def test_main():
print(sys.executable)
# Note that ascii exists as a built-in in Python 3 and
# does not exist in Python 2
ret = main()
assert ret == None
Original file line number Diff line number Diff line change
@@ -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).

import sys

from interpreter_selection.python_3_selection_testing.main_py3 import main


def test_main():
print(sys.executable)
# Note that ascii exists as a built-in in Python 3 and
# does not exist in Python 2
ret = main()
assert ret == ascii
1 change: 1 addition & 0 deletions tests/python/pants_test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ python_library(
'src/python/pants/util:dirutil',
'src/python/pants/util:process_handler',
'tests/python/pants_test/testutils:file_test_util',
'tests/python/pants_test/testutils:pexrc_util',
]
)

Expand Down
4 changes: 4 additions & 0 deletions tests/python/pants_test/backend/python/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,9 @@ python_tests(
'src/python/pants/python',
'src/python/pants/util:contextutil',
'tests/python/pants_test:base_test',
'tests/python/pants_test:int-test',
'tests/python/pants_test/testutils:git_util',
'tests/python/pants_test/testutils:pexrc_util',
],
timeout=1200
)
3 changes: 2 additions & 1 deletion tests/python/pants_test/backend/python/tasks2/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ python_tests(
'src/python/pants/util:contextutil',
'src/python/pants/util:process_handler',
'tests/python/pants_test:int-test',
'3rdparty/python:pex',
],
tags={'integration'},
timeout=600
timeout=2400
)
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@
unicode_literals, with_statement)

import os
import sys
import time

from pants.util.contextutil import temporary_dir
from pants_test.pants_run_integration_test import PantsRunIntegrationTest
from pants_test.testutils.pexrc_util import setup_pexrc_with_pex_python_path


class PytestRunIntegrationTest(PantsRunIntegrationTest):
testproject = 'testprojects/src/python/interpreter_selection'

def test_pytest_run_timeout_succeeds(self):
pants_run = self.run_pants(['clean-all',
'test.pytest',
Expand Down Expand Up @@ -95,3 +99,30 @@ def test_pytest_with_profile(self):
# We won't see a profile at prof itself because PANTS_PROFILE wasn't set when the
# current process started.
self.assertTrue(os.path.exists('{}.0'.format(prof)))

def test_pants_test_interpreter_selection_with_pexrc(self):
"""Test the pants test goal with intepreters selected from a PEX_PYTHON_PATH
defined in a pexrc file on disk.

"""
py27 = '2.7'
py3 = '3'
if self.has_python_version(py27) and self.has_python_version(py3):
print('Found both python {} and python {}. Running test.'.format(py27, py3))
py27_path, py3_path = self.python_interpreter_path(py27), self.python_interpreter_path(py3)
with setup_pexrc_with_pex_python_path(os.path.join(os.path.dirname(sys.argv[0]), '.pexrc'), [py27_path, py3_path]):
with temporary_dir() as interpreters_cache:
pants_ini_config = {'python-setup': {'interpreter_cache_dir': interpreters_cache}}
pants_run_27 = self.run_pants(
command=['test', '{}:test_py2'.format(os.path.join(self.testproject, 'python_3_selection_testing'))],
config=pants_ini_config
)
self.assert_success(pants_run_27)
pants_run_3 = self.run_pants(
command=['test', '{}:test_py3'.format(os.path.join(self.testproject, 'python_3_selection_testing'))],
config=pants_ini_config
)
self.assert_success(pants_run_3)
else:
print('Could not find both python {} and python {} on system. Skipping.'.format(py27, py3))
self.skipTest('Missing neccesary Python interpreters on system.')
Loading