Skip to content

Commit

Permalink
don't match the interpreter patch version for .ipex files! (#9285)
Browse files Browse the repository at this point in the history
A followup to #8793, after realizing that there are actually many separate patch versions of python interpreters going around. `git tag | wc -l` in the [cpython repo](https://github.com/python/cpython/) results in 421 separate tags, many just differing by the patch version.

The terminology being used here is: `CPython==2.7.5` has a *major version* of 2, *minor version* of 7, and a *patch version* of 5.

It is my impression that interpreter patch versions do not affect the libraries that can be used by the interpreter, just the major and minor versions (as in, pip will resolve the same libraries for Python `2.7.5` as `2.7.13`).

When `.ipex` files are created (see #8793 for background), they need to specify a *precise* interpreter version, in order to ensure that the pip resolve that runs when the .ipex is first executed matches exactly the resolve that occurred at build time. However, specifying an interpreter constraint *including* the patch version doesn't affect this invariant, and *does* make .ipex files impossible to run on machines that don't have the exact right patch version of the specified interpreter!

- Make the `PEX-INFO` of a .ipex file point to a single interpreter constraint `{major}.{minor}.*`, which matches any patch version for the given major and minor versions.
  - `ipex_launcher.py` will use this interpreter directly to resolve 3rdparty requirements when the .ipex is first executed, so the interpreter constraint in `PEX-INFO` is how .ipex files ensure they have the correct interpreter to resolve for.

.ipex files are easier to deploy!!!

In addition to multi-platform PEX files, could we also consider multi-interpreter PEX files? Alternatively (and this seems like a *fantastic* longer-term goal) we could follow up on #7369 and wrap the interpreter up with the PEX!
  • Loading branch information
cosmicexplorer committed May 4, 2020
1 parent 2d3bafa commit 7070e46
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 4 deletions.
14 changes: 14 additions & 0 deletions src/python/pants/python/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ python_library(

python_tests(
name='tests',
sources=['*_test.py', '!*_integration.py'],
dependencies=[
'3rdparty/python:pex',
':python',
Expand All @@ -33,3 +34,16 @@ python_tests(
],
tags = {'partially_type_checked'},
)


python_tests(
name='integration',
sources=['*_integration.py'],
dependencies=[
'3rdparty/python:pex',
'src/python/pants/util:contextutil',
'src/python/pants/testutil:int-test',
'testprojects/src/python:python_targets_directory',
],
tags = {'integration', 'partially_type_checked'},
)
20 changes: 16 additions & 4 deletions src/python/pants/python/pex_build_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from typing import Callable, Dict, List, Optional, Sequence, Set, Tuple

from pex.fetcher import Fetcher
from pex.interpreter import PythonInterpreter
from pex.interpreter import PythonIdentity, PythonInterpreter
from pex.pex_builder import PEXBuilder
from pex.pex_info import PexInfo
from pex.platforms import Platform
Expand Down Expand Up @@ -449,14 +449,23 @@ def _prepare_inits(self) -> Set[str]:
def set_emit_warnings(self, emit_warnings):
self._builder.info.emit_warnings = emit_warnings

def _set_major_minor_interpreter_constraint_for_ipex(
self, info: PexInfo, identity: PythonIdentity,
) -> PexInfo:
interpreter_name = identity.requirement.name
major, minor, _patch = identity.version
major_minor_only_constraint = f"{interpreter_name}=={major}.{minor}.*"
return ipex_launcher.modify_pex_info(
info, interpreter_constraints=[str(major_minor_only_constraint)]
)

def _shuffle_underlying_pex_builder(self) -> Tuple[PexInfo, Path]:
"""Replace the original builder with a new one, and just pull files from the old chroot."""
# Ensure that (the interpreter selected to resolve requirements when the ipex is first run) is
# (the exact same interpreter we used to resolve those requirements here). This is the only (?)
# way to ensure that the ipex bootstrap uses the *exact* same interpreter version.
self._builder.info = ipex_launcher.modify_pex_info(
self._builder.info,
interpreter_constraints=[str(self._builder.interpreter.identity.requirement)],
self._builder.info = self._set_major_minor_interpreter_constraint_for_ipex(
self._builder.info, self._builder.interpreter.identity
)

orig_info = self._builder.info.copy()
Expand All @@ -465,6 +474,9 @@ def _shuffle_underlying_pex_builder(self) -> Tuple[PexInfo, Path]:

# Mutate the PexBuilder object which is manipulated by this subsystem.
self._builder = PEXBuilder(interpreter=self._builder.interpreter)
self._builder.info = self._set_major_minor_interpreter_constraint_for_ipex(
self._builder.info, self._builder.interpreter.identity
)

return (orig_info, Path(orig_chroot.path()))

Expand Down
50 changes: 50 additions & 0 deletions src/python/pants/python/pex_build_util_test_integration.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import json
import os
import subprocess

from pex.interpreter import PythonInterpreter

from pants.testutil.pants_run_integration_test import PantsRunIntegrationTest
from pants.util.collections import assert_single_element
from pants.util.contextutil import open_zip, temporary_dir


class PexBuildUtilIntegrationTest(PantsRunIntegrationTest):

binary_target_address = "testprojects/src/python/python_targets:test"

def test_ipex_gets_imprecise_constraint(self) -> None:
cur_interpreter_id = PythonInterpreter.get().identity
interpreter_name = cur_interpreter_id.requirement.name
major, minor, patch = cur_interpreter_id.version

# Pin the selected interpreter to the one used by pants to execute this test.
cur_interpreter_constraint = f"{interpreter_name}=={major}.{minor}.{patch}"

# Validate the the .ipex file specifically matches the major and minor versions, but allows
# any patch version.
imprecise_constraint = f"{interpreter_name}=={major}.{minor}.*"

with temporary_dir() as tmp_dir:
self.do_command(
"--binary-py-generate-ipex",
"binary",
self.binary_target_address,
config={
"GLOBAL": {"pants_distdir": tmp_dir},
"python-setup": {"interpreter_constraints": [cur_interpreter_constraint],},
},
)

pex_path = os.path.join(tmp_dir, "test.ipex")
assert os.path.isfile(pex_path)
pex_execution_result = subprocess.run([pex_path], stdout=subprocess.PIPE, check=True)
assert pex_execution_result.stdout.decode() == "test!\n"

with open_zip(pex_path) as zf:
info = json.loads(zf.read("PEX-INFO"))
constraint = assert_single_element(info["interpreter_constraints"])
assert constraint == imprecise_constraint

0 comments on commit 7070e46

Please sign in to comment.