Only python_binary's constraint should be included in a built pex

[stuhood]

pex-tool/pex#655 changed the default operator for lists of constraints to OR.

But for a long time now PythonBinaryCreate has been using something like the following snippet to collect constraints from all targets in the closure:

pants/src/python/pants/backend/python/tasks/python_binary_create.py

Lines 131 to 145 in 58706c1

	# Find which targets provide sources and which specify requirements.
	source_tgts = []
	req_tgts = []
	constraint_tgts = []
	for tgt in binary_tgt.closure(exclude_scopes=Scopes.COMPILE):
	if has_python_sources(tgt) or has_resources(tgt):
	source_tgts.append(tgt)
	elif has_python_requirements(tgt):
	req_tgts.append(tgt)
	if is_python_target(tgt):
	constraint_tgts.append(tgt)
	# Add interpreter compatibility constraints to pex info. This will first check the targets for any
	# constraints, and if they do not have any will resort to the global constraints.
	pex_builder.add_interpreter_constraints_from(constraint_tgts)

---

For a usecase like:

python_library(
  compatibility=['CPython>=2.7,<4'],
  ..
)

python_binary(
  compatibility=['CPython>=3.6'],
  ..
)

... the resulting pex will have constraints:

['CPython>=2.7,<4', 'CPython>=3.6']

...which when ORd, do not have the intended effect.

---

It looks like what this code wants to be doing is:

1. Including only the python_binary target's constraint,
2. ... but still validating that the context is compatible.

Doing 1 is straightforward, but I'll need a pointer on 2. cc @jsirois , @Eric-Arellano

[Eric-Arellano]

Agreed that the desired behavior is to only use the python_binary's constraints for the PEX's interpreter constraints. If those constraints are missing, then we should fallback to the global --python-setup-interpreter-constraints. In the above example, it should be ['CPython>=3.6'] and nothing else.

As pitched over Slack, I think your idea to do an interpreter cache lookup with all constraints first to ensure is a good one. I recommend using this function:

pants/src/python/pants/backend/python/interpreter_cache.py

Lines 79 to 80 in 58706c1

	def select_interpreter_for_targets(self, targets):
	"""Pick an interpreter compatible with all the specified targets."""

Otherwise, we would have to build a Pex twice—the first time with all constraints and the second with just the binary_target constraints—and then execute the first Pex to see if it fails at runtime.

--

It would be great to see a test added for this. I think we want to test where a binary's compatibility conflicts with one of its library's compatibility, to ensure that the build fails.

[jsirois]

Although select_interpreter_for_targets sort of does the right thing, its not completely correct. I think using it may introduce no breaks over current broken-ness though. In particular, the broken cases are those where we have no local interpreter appropriate, but we have all needed wheels. For example, building a 2.7 pex on a 3.6 only machine. Should work, but won't. As noted, this is already broken, but the correct future would probably parse constraints using packaging and form the intersection directly from math on components of the parsed constraint never involving any real local interpreter.

[Eric-Arellano]

Good call, John. Making this resolution not depend on the local interpreters available also reduces the risk of Pex interpreter resolution diverging from Pants interpreter resolution. Finally, I suspect it will make it easier for us to port ./pants binary to V2 and remoting.

Stu, feel free to assign this ticket to me.

[jsirois]

The necessary bits:

$ pex packaging
Python 3.7.3 (default, Mar 26 2019, 21:43:19) 
[GCC 8.2.1 20181127] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from packaging.requirements import Requirement
>>> req1 = Requirement('CPython>=3.6')
>>> req2 = Requirement('CPython>=2.7,<4')
>>> req1.specifier
<SpecifierSet('>=3.6')>
>>> req2.specifier
<SpecifierSet('<4,>=2.7')>
>>> all_specs = list(req1.specifier) + list(req2.specifier)
>>> all_specs
[<Specifier('>=3.6')>, <Specifier('<4')>, <Specifier('>=2.7')>]
>>> components = [(spec.operator, spec.version) for spec in all_specs]
>>> components
[('>=', '3.6'), ('<', '4'), ('>=', '2.7')]
>>> 

[Eric-Arellano]

@stuhood Here's an initial diff:

diff --git a/src/python/pants/backend/python/subsystems/python_setup.py b/src/python/pants/backend/python/subsystems/python_setup.py
index 695b1def7..2e41aa07d 100644
--- a/src/python/pants/backend/python/subsystems/python_setup.py
+++ b/src/python/pants/backend/python/subsystems/python_setup.py
@@ -133,6 +133,22 @@ class PythonSetup(Subsystem):
       return tuple(self.interpreter_constraints)
     return tuple(compatibility or self.interpreter_constraints)
 
+  # TODO: this function should probably directly take interpreter constraints, and not call
+  # self.compatibility_or_constraints(), because it would be a simpler function that makes less
+  # assumptions and it would also become a pure function. Would make it much easier to test.
+  def has_no_compatibility_constraint_conflicts(self, *compatibility_entries):
+    """Confirm that all targets have compatible constraints.
+
+    :param compatibility_entries: List[Optional[List[str]]], e.g. *[None, ['CPython>3']].
+    """
+    resolved_constraints = {
+      constraint
+      for compatibility_entry in compatibility_entries
+      for constraint in self.compatibility_or_constraints(compatibility_entry)
+    }
+    # TODO: somehow calculate if any of the constraints fail, likely using packaging.requirements.
+    # TODO: add various unit tests for this this function.
+
   @classmethod
   def expand_interpreter_search_paths(cls, interpreter_search_paths, pyenv_root_func=None):
     special_strings = {
diff --git a/src/python/pants/backend/python/tasks/python_binary_create.py b/src/python/pants/backend/python/tasks/python_binary_create.py
index 805dfa8a5..2b98a550d 100644
--- a/src/python/pants/backend/python/tasks/python_binary_create.py
+++ b/src/python/pants/backend/python/tasks/python_binary_create.py
@@ -140,9 +140,19 @@ class PythonBinaryCreate(Task):
         if is_python_target(tgt):
           constraint_tgts.append(tgt)
 
-      # Add interpreter compatibility constraints to pex info. This will first check the targets for any
-      # constraints, and if they do not have any will resort to the global constraints.
-      pex_builder.add_interpreter_constraints_from(constraint_tgts)
+      # Check that all dependencies have valid compatibility constraints that do not conflict
+      # with the binary's constraints.
+      # TODO: somehow reference global instance of PythonSetup
+      # TODO: fail if not compatible
+      PythonSetup().has_no_compatibility_constraint_conflicts(*[
+        getattr(tgt, 'compatibility', None) for tgt in constraint_tgts
+      ])
+
+
+      # Add interpreter compatibility constraints to pex info from the binary target. This will
+      # first check if the target has `compatibility` defined, then will resort to the global
+      # constraints.
+      pex_builder.add_interpreter_constraints_from([binary_tgt])
 
       # Dump everything into the builder's chroot.
       for tgt in source_tgts:

I'm going to move on to other work for now, but can pick back up tomorrow if you'd like.

[stuhood]

So... while I agree that not requiring a matching local interpreter would be good, we're currently trying to unblock the 1.16.x release, which unblocks deleting python 2 (among other things), which unblocks Eric dying a happy man.

I'd like to avoid doing anything algorithmically fiddly in that path, and hopefully hand off a working patch to @illicitonion in a few hours that he can land, cherry-pick, and ship.

So I'm leaning toward just doing the select_interpreter_for_targets thing right now.

[Eric-Arellano]

So I'm leaning toward just doing the select_interpreter_for_targets thing right now.

So long as we have a good TODO to make this more robust, I'm fine with that given the context.

which unblocks Eric dying a happy man.

😀

[stuhood]

It looks like there is an equivalent issue with PythonRun... 823ae84 fixed this for pytest, but it seems like this is basically just: "all pex callsites need updating post 1.6.7 upgrade to account for ANDd constraints".

[illicitonion]

It looks like there is an equivalent issue with PythonRun... 823ae84 fixed this for pytest, but it seems like this is basically just: "all pex callsites need updating post 1.6.7 upgrade to account for ANDd constraints".

Can we instead factor this out so that PythonBinary does things right, and PythonTests and PythonRun just depend on a synthetic PythonBinary, and then run them?

That would avoid this problem recurring, and would also mean that resources are treated uniformly across ./pants binary and ./pants run (which I'm about to file a bug about).