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

When python target compatibility is not set, use interpreter constraints. #6284

Merged
merged 6 commits into from Aug 1, 2018

Conversation

Projects
None yet
7 participants
@stuhood
Copy link
Member

stuhood commented Jul 31, 2018

Problem

Currently, interpreter_constraints are not used as the "default" value of compatibility for a python target, which means that:

./pants --python-setup-interpreter-constraints='["CPython>=3.5"]' test tests/python/pants_test/java:nailgun_io

...fails in an odd half-configured fashion (where the interpreter is bounded in some way, but not the dep resolution).

Solution

Use interpreter_constraints as the default value of compatibility for a target. This aligns with #6250, and that patch should likely land as well.

Result

./pants --python-setup-interpreter-constraints='["CPython>=3.5"]' test tests/python/pants_test/java:nailgun_io
...works, and uses python 3.

Fixes #5082.


Before this patch, in a repository with:

--python-setup-interpreter-constraints='["CPython>=2.7,CPython<3"]'

...individual targets were able to claim to be compatible with CPython>3 while depending on targets with no declared compatibility, and things would just work.

After this patch, the default compatibility for a repository is specified via the constraints, and so rather than being completely unconstrained, a target with no compatibility argument would pick up the default. In the case above, that would cause a failure.

There are a few potential fixes for consumers:

  1. don't pin interpreters in pants.ini, and only pin compatibility on binaries, or on targets where it is relevant
  2. pin interpreters in pants.ini, but for a range wide enough to cover all of your binary and test target's compatibilities
  3. pin interpreters for a "default" compatibility (ie, default to 2), and then explicitly clear the default on libraries to make them "agnostic": ie, python_library(.., compatibility=[]) (or declare them to be wide enough to cover all consumers).
tgts_with_compatibilities.append(target)
filters.update(target.compatibility)
if isinstance(target, PythonTarget):
c = self._python_setup.compatibility_or_constraintstar(target)

This comment has been minimized.

@jsirois

jsirois Jul 31, 2018

Member

typo: _python_setup.compatibility_or_constraintstar

This comment has been minimized.

@stuhood

stuhood Jul 31, 2018

Member

Blargh, thanks. Extracted the helper after getting things working.

if not targets:
targets = tgts_by_compatibilities[c] = []
targets.append(target)
filters.update(c)

allowed_interpreters = set(self.setup(filters=filters))

This comment has been minimized.

@jsirois

jsirois Jul 31, 2018

Member

This call already follows the logic: if filters - use them, else use the PythonSetup.interpreter_constraints as the filters. Are you sure PIC needs modification?

This comment has been minimized.

@stuhood

stuhood Jul 31, 2018

Member

I am not sure that PIC needs modification, as I expect it was the edit in python_execution_task_base.py that had the effect.

But "if no targets specify compatibility values, then use the default" is not quite the same: if you have one target with no compatibility (that should thus use the default) and one target with, you should (attempt to) merge those.

This does make me wonder whether this logic needs graph awareness of some sort though (hopefully not quite as far as partitioning).

This comment has been minimized.

@jsirois

jsirois Jul 31, 2018

Member

Agreed they're not the same, but this did read as a somewhat thrashy change trying a few things at once. It would be nice to revert without thorough tests if not needed.

@benjyw

benjyw approved these changes Jul 31, 2018

filters.update(target.compatibility)
if isinstance(target, PythonTarget):
c = self._python_setup.compatibility_or_constraintstar(target)
targets = tgts_by_compatibilities.get(c)

This comment has been minimized.

@benjyw

benjyw Jul 31, 2018

Contributor

Could you not streamline all this with a defaultdict(list)? Then tgts_by_compatibilities[c] will create, assign and return an empty list if none exists at that key.

@@ -80,8 +80,7 @@ def register_options(cls, register):

@property
def interpreter_constraints(self):
return (self.get_options().interpreter_constraints or self.get_options().interpreter or
[self.get_options().interpreter_requirement or b''])
return tuple(self.get_options().interpreter_constraints)

This comment has been minimized.

@benjyw

benjyw Jul 31, 2018

Contributor

So was the previous version simply broken? There is no --interpreter-requirement option.

This comment has been minimized.

@benjyw

benjyw Jul 31, 2018

Contributor

And since you're here and fixing this, can you also delete the stale comment on line 22 above?

This comment has been minimized.

@stuhood

stuhood Jul 31, 2018

Member

I believe so, yes. Guessing it was left behind after a deprecation.

@benjyw

benjyw approved these changes Jul 31, 2018

@alanbato
Copy link
Member

alanbato left a comment

LGTM 👍

@stuhood stuhood force-pushed the twitter:stuhood/intepreter-constraints-provide-compatibility branch from 4dfb60f to bc06e1c Aug 1, 2018

@stuhood stuhood force-pushed the twitter:stuhood/intepreter-constraints-provide-compatibility branch from c837a48 to 99d31a1 Aug 1, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 1, 2018

There is one failing integration test to act as a reminder that this represents a major behavior change.

Currently, in a repository with:

--python-setup-interpreter-constraints='["CPython>=2.7,CPython<3"]'

...individual targets are able to claim to be compatible with CPython>3 while depending on targets with no declared compatibility, and things will just work.

After this patch, the default compatibility for a repository would be specified via the constraints, and so rather than being completely unconstrained, a target with no compatibility argument would pick up the default. In the case above, that would cause a failure.


How do we feel about this? There are a few potential deployment strategies for a mixed repository:

  1. don't pin interpreters in pants.ini, and only pin compatibility on binaries, or on targets where it is relevant
  2. pin interpreters in pants.ini, but for a range wide enough to cover all of your binary's/test's compatibilities
  3. pin interpreters for a "default" compatibility (ie, default to 2), and then explicitly clear the default on libraries to make them "agnostic": ie, python_library(.., compatibility=[]), or wide enough to cover all consumers

@benjyw , @jsirois , @CMLivingston , @kwlzn : Thoughts?

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 1, 2018

I'm happy with the behavior change, see #5082. It seems to me 1 & 2 above are sane. 3 is not today since the option says nothing about default in its name. We should probably remove the default constaints in PythonSetup and or rename the option.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 1, 2018

There are also solutions that involve a bit of graph awareness (but don't go all the way to partitioning the graph): for example, default compatibility could apply only to roots (binaries/tests), but not to inner libraries.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 1, 2018

This sounds worth the break for the consistency.

To talk a little about Foursquare's use case, we right now only have Python 2, but are migrating next few weeks. Our ideal is to make Python 3 be the default, and within BUILD files you would have to explicitly specify it's Py2 only or Py2 + Py3, which sounds like strategy 3.

I'm confused on one scenario. If your pants.ini only specifies Py3, but your BUILD specifies Py2, which interpreter will be used?

cc @mateor if I got anything wrong.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Aug 1, 2018

I think the behavior change is fine, as long as there's a workaround, and there are several.

@mateor

This comment has been minimized.

Copy link
Member

mateor commented Aug 1, 2018

Thanks Eric, you represented it well

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 1, 2018

I'm confused on one scenario. If your pants.ini only specifies Py3, but your BUILD specifies Py2, which interpreter will be used?

@Eric-Arellano : The effect of this change is that interpreter_constraints is the default, and compatibility on a target overrides the default. Unless there are other codepaths that are using interpreter_constraints but not compatibility (not sure...), a target could override compatibility outside of the constraints just fine... it just might have to do it transitively in its dependencies as well.

@stuhood stuhood merged commit d1f9001 into pantsbuild:master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/intepreter-constraints-provide-compatibility branch Aug 1, 2018

@mateor

This comment has been minimized.

Copy link
Member

mateor commented Aug 1, 2018

@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Aug 2, 2018

I'm late, but I agree that the result of this change is the right direction.

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

When python target compatibility is not set, use interpreter constrai…
…nts. (pantsbuild#6284)

### Problem

Currently, `interpreter_constraints` are not used as the "default" value of `compatibility` for a python target, which means that:
```
./pants --python-setup-interpreter-constraints='["CPython>=3.5"]' test tests/python/pants_test/java:nailgun_io
```

...fails in an odd half-configured fashion (where the interpreter is bounded in some way, but not the dep resolution).

### Solution

Use `interpreter_constraints` as the default value of `compatibility` for a target. This aligns with pantsbuild#6250, and that patch should likely land as well.

### Result

```./pants --python-setup-interpreter-constraints='["CPython>=3.5"]' test tests/python/pants_test/java:nailgun_io```
...works, and uses python 3.

Fixes pantsbuild#5082.

----

Before this patch, in a repository with:
```
--python-setup-interpreter-constraints='["CPython>=2.7,CPython<3"]'
```
...individual targets were able to claim to be compatible with `CPython>3` while depending on targets with no declared compatibility, and things would just work.

After this patch, the default compatibility for a repository is specified via the constraints, and so rather than being completely unconstrained, a target with no compatibility argument would pick up the default. In the case above, that would cause a failure.

There are a few potential fixes for consumers:
1. don't pin interpreters in pants.ini, and only pin compatibility on binaries, or on targets where it is relevant
2. pin interpreters in pants.ini, but for a range wide enough to cover all of your binary and test target's compatibilities
3. pin interpreters for a "default" compatibility (ie, default to 2), and then explicitly clear the default on libraries to make them "agnostic": ie, `python_library(.., compatibility=[])` (or declare them to be wide enough to cover all consumers).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment