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 #6387

Merged
merged 19 commits into from Aug 28, 2018

Conversation

Projects
None yet
3 participants
@CMLivingston
Copy link
Contributor

CMLivingston commented Aug 22, 2018

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)

@CMLivingston CMLivingston changed the title Clivingston/python setup interpreter override Override interpreter constraints if global option is passed down Aug 22, 2018

@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Aug 23, 2018

@stuhood this is ready for another review. Previous changes were accepted in the final state of Alan's PR, and the commits of interest in this one are the last two.

@stuhood stuhood requested review from stuhood , benjyw , jsirois and alanbato and removed request for stuhood and benjyw Aug 27, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks! The tests are appreciated.

One implementation tweak, and then this should be landable.

tgts_by_compatibilities[c].append(target)
filters.update(c)

if self._python_setup.get_options().is_flagged('interpreter_constraints'):

This comment has been minimized.

@stuhood

stuhood Aug 27, 2018

Member

Mentioned offline: this separate if/else block should not actually be necessary if the is_flagged logic were to move into compatibility_or_constraints... the effect when is_flagged=True would be that all targets ended up with the same config values.

This would also have the advantage of working anywhere else the compatibility_or_constraints method is used.

for compatibility in tgts_by_compatibilities.keys():
compatible_with_target = set(self._matching(allowed_interpreters, compatibility))
allowed_interpreters &= compatible_with_target
if not self._python_setup.get_options().is_flagged('interpreter_constraints'):

This comment has been minimized.

@stuhood

stuhood Aug 27, 2018

Member

This if would not be necessary with the implementation described above.

@@ -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 27, 2018

Member

I believe that this is also not necessary if the is_flagged logic moves into compatibility_or_constraints, because python_execution_task_base.py will do the right thing.

@@ -147,3 +147,47 @@ def test_pants_test_interpreter_selection_with_pexrc(self):
else:
print('Could not find both python {} and python {} on system. Skipping.'.format(py27, py3))
self.skipTest('Missing neccesary Python interpreters on system.')

def test_pants_test_interpreter_selection_with_option_2(self):
"""Test that the pants test goal with an interpreter constrainted by the

This comment has been minimized.

@stuhood

stuhood Aug 27, 2018

Member

This comment needs some cleanup.

self.assert_success(pants_run_2)

def test_pants_test_interpreter_selection_with_option_3(self):
"""Test that the pants test goal with an interpreter constrainted by the

This comment has been minimized.

@stuhood

stuhood Aug 27, 2018

Member

ditto

@stuhood
Copy link
Member

stuhood left a comment

Thanks! Will merge on green CI.

@@ -115,8 +115,9 @@ def create_pex(self, pex_info=None):
# Add the extra requirements first, so they take precedence over any colliding version
# in the target set's dependency closure.
pexes = [extra_requirements_pex] + pexes
constraints = {constraint for rt in relevant_targets if is_python_target(rt)
for constraint in PythonSetup.global_instance().compatibility_or_constraints(rt)}
constraints = set(pex_info.interpreter_constraints)

This comment has been minimized.

@stuhood

stuhood Aug 27, 2018

Member

This change should not be necessarry anymore... although it doesn't hurt.

@stuhood
Copy link
Member

stuhood left a comment

Argh... that was supposed to be an approval. Let's try that again.

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/python_setup_interpreter_override branch from 358dc6c to 19b0193 Aug 27, 2018

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/python_setup_interpreter_override branch from 19b0193 to 3e23885 Aug 27, 2018

@stuhood stuhood merged commit c0a64d0 into pantsbuild:master Aug 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment