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

Override interpreter constraints if global option is passed down #6250

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@alanbato
Copy link
Member

alanbato commented Jul 27, 2018

Problem

The original issue is documented at #6081, an attempt to solve this was #6234, but we've decided to take another path instead.

Solution

Instead of relying on interpreter constraints when building a pex in the repl task, we're checking if the --python-setup-interpreter-constraints is flagged. If so, it overrides the interpreter filters at interpreter selection time.

Result

Users should now be able to further filter the python interpreter to use even if the task does not declare an option to do so (like in the repl task)

@@ -151,7 +151,10 @@ def setup(self, paths=(), filters=(b'',)):
# 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 self._python_setup.get_options().is_flagged('interpreter_constraints'):

This comment has been minimized.

@CMLivingston

CMLivingston Jul 27, 2018

Contributor

This logic will need to move out of this method and up into select_interpreter_for_targets. We will want something like this:

  def select_interpreter_for_targets(self, targets):
    """Pick an interpreter compatible with all the specified targets."""
    tgts_with_compatibilities = []
    filters = set()

    if self._python_setup.get_options().is_flagged('interpreter_constraints'):
      # Reverse to prioritize flagged option over default value.
      filters = reversed(self._python_setup.get_options().interpreter_constraints)
    else:
      for target in targets:
        if isinstance(target, PythonTarget) and target.compatibility:
          tgts_with_compatibilities.append(target)
          filters.update(target.compatibility)

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

    if not self._python_setup.get_options().is_flagged('interpreter_constraints'):
      # Constrain allowed_interpreters based on each target's compatibility requirements.
      for target in tgts_with_compatibilities:
        compatible_with_target = set(self._matching(allowed_interpreters, target.compatibility))
        allowed_interpreters &= compatible_with_target

Essentially what is happening is that this method filters selected interpreters with target constraints. We need to disable that by checking the flag.

The reversing I did here is so that we do not use the default constraints, but rather the CLI-specified ones. This may be a case for a new python-setup option altogether. I am open to suggestions of cleaner ways of doing this as well. An alternative could be to use a boolean that gets set once at the top and reused, however this way will be fine as well. I will leave the imp details up to you.

python_binary(
name='main_py23',
source='main_py23.py',
compatibility=['CPython>2.7.6,<4'],

This comment has been minimized.

@CMLivingston

CMLivingston Jul 27, 2018

Contributor

CPython>=2.7,<4 should be sufficient.

This comment has been minimized.

@alanbato

alanbato Aug 15, 2018

Member

Should I change the other instances of 2.7.6 too then? I followed the convention of the other targets

@@ -0,0 +1,21 @@
# coding=utf-8

This comment has been minimized.

@CMLivingston

CMLivingston Jul 27, 2018

Contributor

Remind me why you created new files for this? I would prefer if you just reworked the existing files to fit your testing requirements rather than creating these new files. Then you could just add more targets.

This route would require you to update the test scripts here (test_py{2, 3}) to ensure they are passing and checking for the proper return values ("Python3" instead of ascii, or whatever).

This comment has been minimized.

@alanbato

alanbato Jul 27, 2018

Member

Because the original use case was working with libraries that are Python2/3 compatible, and the targets and files already there are specific to each version.
Arguably, we could convert either the 2 or 3 version to be 2/3 compatible, but I didn't want to modify any existing tests and behaviors.

Why would the tests need to be changed? They still use the same files and targets, the reason I created new files was to avoid that. Maybe there's something I'm missing?

I rather have Python2, Python3, and Python2/3 files and targets, so we can be sure we're testing against every use case.

This comment has been minimized.

@CMLivingston

CMLivingston Jul 27, 2018

Contributor

Sounds good to me! I was conflating these files with Twitter-internal files that support this testing.


if self._python_setup.get_options().is_flagged('interpreter_constraints'):
# Reverse to prioritize flagged option over default value
filters.update(reversed(self._python_setup.get_options().interpreter_constraints))

This comment has been minimized.

@CMLivingston

CMLivingston Jul 30, 2018

Contributor

We want to get the last constraint in the list here (the CLI-supplied constraint) via:
filters.add(self._python_setup.get_options().interpreter_constraints[-1])
We need to add the element here so the set object does not update the set with each character of the constraint string.

filters.update(target.compatibility)

if self._python_setup.get_options().is_flagged('interpreter_constraints'):
# Reverse to prioritize flagged option over default value

This comment has been minimized.

@CMLivingston

CMLivingston Jul 30, 2018

Contributor

Nit: Add a period at the end of this comment please.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 31, 2018

I like the idea behind this patch. There is a small amount of prior art that could be lifted up to make this more natural: see https://github.com/pantsbuild/pants/blob/master/src/python/pants/build_graph/bundle_mixin.py#L39-L56

That logic slots a target's field value in "between" the value from the command line and the value from pants.ini.


But I think you'll need to update a few more locations, as I discovered in #6284 while trying to do something similar. Sorry, I didn't realize until afterward that this patch is relevant.

stuhood added a commit that referenced this pull request Aug 1, 2018

When python target compatibility is not set, use interpreter constrai…
…nts. (#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 #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).

@alanbato alanbato force-pushed the alanbato:python_setup_interpreter_override branch from 0f7e735 to c420abd Aug 2, 2018

@alanbato alanbato force-pushed the alanbato:python_setup_interpreter_override branch from c420abd to 4f1b9e7 Aug 6, 2018

@@ -48,6 +49,9 @@ def setup_repl_session(self, targets):
entry_point = 'code:interact'
pex_info = PexInfo.default()
pex_info.entry_point = entry_point
python_setup = PythonSetup.global_instance()
setup_constraints = python_setup.interpreter_constraints
pex_info.add_interpreter_constraint(setup_constraints[-1])

This comment has been minimized.

@CMLivingston

CMLivingston Aug 7, 2018

Contributor

We will want to wrap lines 53-54 in an
if python_setup.get_options().is_flagged('interpreter_constraints'):
because we only want to pluck the last element off in that case (the one set by the CLI). When the CLI arg is not flagged, we will be getting either compatibility or config constraints as seen in line 118 of python_execution_task_base.py.

filters.update(c)

if self._python_setup.get_options().is_flagged('interpreter_constraints'):
# Add the last option passed down in the option constraints.

This comment has been minimized.

@CMLivingston

CMLivingston Aug 7, 2018

Contributor

I think this comment wording was my suggestion and it can be improved. Maybe something like:
# Pants will append the CLI-supplied constraints after the pants.ini constraints and we want to take only the CLI-supplied constraints in this case.

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

Looking good! I requested some changes to clean it up a bit but this looks ready for some testing!


@ensure_daemon
def test_run_repl_with_2(self):
# Run a Python 2 repl on a Python 2/3 library target.

This comment has been minimized.

@CMLivingston

CMLivingston Aug 14, 2018

Contributor

indentation on this comment

program = 'from interpreter_selection.echo_interpreter_version import say_hello; say_hello()'
pants_run = self.run_pants(command=command, stdin_data=program)
version = pants_run.stdout_data
self.assertRegexpMatches(pants_run.stdout_data, r'3\.\d\.\d')

This comment has been minimized.

@CMLivingston

CMLivingston Aug 15, 2018

Contributor

add a newline please :)


@ensure_daemon
def test_run_repl_with_3(self):
# Run a Python 3 repl on a Python 2/3 library target. Avoid some known-to-choke-on interpreters.

This comment has been minimized.

@CMLivingston

CMLivingston Aug 15, 2018

Contributor

indent this comment

This comment has been minimized.

@alanbato

alanbato Aug 15, 2018

Member

Actually it was the other lines the ones that were over-indented

self.assert_success(pants_run_3)

def test_run_2_by_option(self):
if self.skip_if_no_python('3'):

This comment has been minimized.

@CMLivingston

CMLivingston Aug 15, 2018

Contributor

should this be '2'?

This comment has been minimized.

@alanbato
def test_die(self):
command = ['run',
'{}:die'.format(self.testproject),
'--python-setup-interpreter-constraints=CPython>=2.7,<3',
'--python-setup-interpreter-constraints=CPython>=3.3',
'--python-setup-interpreter-constraints=["CPython>=2.7,<3", ">=3.3"]',

This comment has been minimized.

@CMLivingston

CMLivingston Aug 15, 2018

Contributor

Did this fix the local test for you? Mine is still failing.

This comment has been minimized.

@alanbato

alanbato Aug 15, 2018

Member

No, it was an attempt do so, though.

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

If it's failing in an attempt to build cryptography, you might be seeing the pexrc issue I opened that ticket about. But also, see #6352, which @nsaechao opened recently.

This comment has been minimized.

@CMLivingston

CMLivingston Aug 16, 2018

Contributor

We can revert this change. This only fails locally and we can follow up separately. I suspect this is unrelated to this change and has to do with the crypto build.

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

This looks awesome! Great job and the readability is on point here. It would be good to add a 2 and a 3 case for the pytest run integration as well, but after that we should be good to go to merge this (with a green CI).

@alanbato
Copy link
Member

alanbato left a comment

Replied to some of the code comments


@ensure_daemon
def test_run_repl_with_3(self):
# Run a Python 3 repl on a Python 2/3 library target. Avoid some known-to-choke-on interpreters.

This comment has been minimized.

@alanbato

alanbato Aug 15, 2018

Member

Actually it was the other lines the ones that were over-indented

self.assert_success(pants_run_3)

def test_run_2_by_option(self):
if self.skip_if_no_python('3'):

This comment has been minimized.

@alanbato
def test_die(self):
command = ['run',
'{}:die'.format(self.testproject),
'--python-setup-interpreter-constraints=CPython>=2.7,<3',
'--python-setup-interpreter-constraints=CPython>=3.3',
'--python-setup-interpreter-constraints=["CPython>=2.7,<3", ">=3.3"]',

This comment has been minimized.

@alanbato

alanbato Aug 15, 2018

Member

No, it was an attempt do so, though.

python_binary(
name='main_py23',
source='main_py23.py',
compatibility=['CPython>2.7.6,<4'],

This comment has been minimized.

@alanbato

alanbato Aug 15, 2018

Member

Should I change the other instances of 2.7.6 too then? I followed the convention of the other targets

alanbato added some commits Aug 15, 2018

@alanbato alanbato changed the title [WIP] Override interpreter constraints if global option is passed down Override interpreter constraints if global option is passed down Aug 16, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!


if self._python_setup.get_options().is_flagged('interpreter_constraints'):
# Pants will append the CLI-supplied constraints after the pants.ini constraints by default
# and we want to take only the CLI-supplied constraints in this case.

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

This comment is stale I think.

This comment has been minimized.

@CMLivingston
@@ -48,6 +49,11 @@ def setup_repl_session(self, targets):
entry_point = 'code:interact'
pex_info = PexInfo.default()
pex_info.entry_point = entry_point
python_setup = PythonSetup.global_instance()
if python_setup.get_options().is_flagged('interpreter_constraints'):

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

Consider moving the call to is_flagged into a helper on PythonSetup... something like: interpreter_contraints_if_flagged, which returns None if not flagged?

# Run a Python 2 repl on a Python 2/3 library target.
command = ['repl',
'testprojects/src/python/interpreter_selection:echo_interpreter_version_lib',
'--python-setup-interpreter-constraints=CPython<3',

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

If this is supposed to be overriding the default (rather than adding to it), it would need to use something like: --python-setup-interpreter-constraints='["CPython<3"]' ... see https://www.pantsbuild.org/options.html#list-options

This comment has been minimized.

@CMLivingston

CMLivingston Aug 16, 2018

Contributor

+1 to this, however you need to omit the ' ' around the list when supplying this via string in a test:
--python-setup-interpreter-constraints=["CPython<3"]

# Run a Python 3 repl on a Python 2/3 library target. Avoid some known-to-choke-on interpreters.
command = ['repl',
'testprojects/src/python/interpreter_selection:echo_interpreter_version_lib',
'--python-setup-interpreter-constraints=CPython>=3.3',

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

ditto

pants_ini_config = {'python-setup': {'interpreter_constraints': ["CPython>=2.7,<4"]}}
pants_run_3 = self.run_pants(
command=['run', '{}:echo_interpreter_version_3'.format(self.testproject),
'--python-setup-interpreter-constraints=CPython>=3'],

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

ditto

def test_die(self):
command = ['run',
'{}:die'.format(self.testproject),
'--python-setup-interpreter-constraints=CPython>=2.7,<3',
'--python-setup-interpreter-constraints=CPython>=3.3',
'--python-setup-interpreter-constraints=["CPython>=2.7,<3", ">=3.3"]',

This comment has been minimized.

@stuhood

stuhood Aug 16, 2018

Member

If it's failing in an attempt to build cryptography, you might be seeing the pexrc issue I opened that ticket about. But also, see #6352, which @nsaechao opened recently.

':main_py23'
],
compatibility=['CPython>2.7,<4']
)

This comment has been minimized.

@CMLivingston

CMLivingston Aug 16, 2018

Contributor

newline here

command=[
'test',
'{}:test_py3'.format(os.path.join(self.testproject,'python_3_selection_testing')),
'--python-setup-interpreter-constraints=CPython>=3',

This comment has been minimized.

@CMLivingston

CMLivingston Aug 16, 2018

Contributor

This will likely break CI. Needs to be '--python-setup-interpreter-constraints=["CPython>=3"]'

@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Aug 16, 2018

@stuhood We should delete thrift and pants.pex binaries before merging this, correct? I think they are here only as a workaround for some broken build support scripting on Alan's laptop.

@alanbato

This comment has been minimized.

Copy link
Member

alanbato commented Aug 16, 2018

I already deleted them on my last commit I think?

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).

stuhood added a commit that referenced this pull request Aug 28, 2018

Override interpreter constraints if global option is passed down (#6387)
I commandeered @alanbato's PR at #6250

## Problem
The original issue is documented at #6081, an attempt to solve this was #6234, but we've decided to take another path instead.

## Solution
Instead of relying on interpreter constraints when building a pex in the repl task, we're checking if the --python-setup-interpreter-constraints is flagged. If so, it overrides the interpreter filters at interpreter selection time.

## Result
Users should now be able to further filter the python interpreter to use even if the task does not declare an option to do so (like in the repl task)
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 29, 2018

@alanbato : This landed as #6387. Thanks a lot for your contribution!

@stuhood stuhood closed this Aug 29, 2018

@alanbato

This comment has been minimized.

Copy link
Member

alanbato commented Aug 30, 2018

My pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment