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

Allow users to request a fixed python interpreter #4930

Merged
merged 1 commit into from Oct 5, 2017

Conversation

Projects
None yet
4 participants
@copumpkin
Copy link
Contributor

copumpkin commented Oct 2, 2017

Problem

In some situations, the automatic python picker logic picks the wrong version of Python and we don't have much control over that beyond being able to specify version constraints (which sometimes aren't sufficient).

Solution

I added an option to explicitly specify a list of paths to search.

Result

Users can now pass in --python-setup-interpreter-search-paths=<search-paths> and have Pants search in the specified paths. You can even pass in the full path of the interpreter (e.g., which python) and it'll figure that out!

if fixed_interpreter:
interpreter = PythonInterpreter.from_binary(fixed_interpreter)
else:
python_tgts = self.context.targets(lambda tgt: isinstance(tgt, PythonTarget))

This comment has been minimized.

@copumpkin

copumpkin Oct 2, 2017

Contributor

Use GitHub's secret whitespace ignore option to see that I haven't actually changed any of this: https://github.com/pantsbuild/pants/pull/4930/files?w=1

This comment has been minimized.

@benjyw

benjyw Oct 2, 2017

Contributor

Neat! Didn't know that existed. Although I'm not sure what that does exactly, esp. where whitespace is meaningful.

Anyway, might be better to split this branch off into a separate method, for readability.

@stuhood stuhood requested review from benjyw and kwlzn Oct 2, 2017

@copumpkin copumpkin force-pushed the copumpkin:fixed-python branch from 46bbe64 to 8689dda Oct 2, 2017

@copumpkin copumpkin changed the base branch from 1.3.x to master Oct 2, 2017

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 2, 2017

I'm torn between calling the option fixed vs. fixed-interpreter. The fixed version reads more nicely on the command line, so you can write --pyprep-interpreter-fixed=<interp>, but in pants.ini, the fixed-interpreter seems a bit clearer.

@benjyw
Copy link
Contributor

benjyw left a comment

Thanks for the contribution! See comments inline.

@@ -41,6 +41,11 @@ class SelectInterpreter(Task):
"""Select an Python interpreter that matches the constraints of all targets in the working set."""

@classmethod
def register_options(cls, register):
super(SelectInterpreter, cls).register_options(register)
register('--fixed-interpreter', type=str, help='Use this Python interpreter instead of searching based on constraints', default=None)

This comment has been minimized.

@benjyw

benjyw Oct 2, 2017

Contributor

Maybe this option should be in python_setup.py along so it's next to --interpreter-constraints ?

Also, if I understand correctly the value here should be a path to a python binary? Let's update the help text to clarify this (i.e., what "this Python interpreter" means...)

This comment has been minimized.

@copumpkin

copumpkin Oct 2, 2017

Contributor

Sure! I thought of doing that but then I don't know how to get the constraints option from there to here. I haven't figured out yet how the current top-level constraints propagate yet.

This comment has been minimized.

@copumpkin

copumpkin Oct 2, 2017

Contributor

Ah, I need to use the global_instance I think. Okay, I'll push an update doing all that!

if fixed_interpreter:
interpreter = PythonInterpreter.from_binary(fixed_interpreter)
else:
python_tgts = self.context.targets(lambda tgt: isinstance(tgt, PythonTarget))

This comment has been minimized.

@benjyw

benjyw Oct 2, 2017

Contributor

Neat! Didn't know that existed. Although I'm not sure what that does exactly, esp. where whitespace is meaningful.

Anyway, might be better to split this branch off into a separate method, for readability.

@copumpkin copumpkin force-pushed the copumpkin:fixed-python branch from 8689dda to 6768dad Oct 2, 2017

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 2, 2017

@benjyw I pushed a new version that (I think) addresses your comments. I haven't run the tests or anything on it yet, but it seems to work fine on my local installation

@@ -62,13 +62,19 @@ def register_options(cls, register):
register('--artifact-cache-dir', advanced=True, default=None, metavar='<dir>',
help='The parent directory for the python artifact cache. '
'If unspecified, a standard path under the workdir is used.')
register('--fixed-interpreter', advanced=True, default=None, metavar='<interpreter-binary>',

This comment has been minimized.

@kwlzn

kwlzn Oct 2, 2017

Member

this should probably be an interpreter search path that's checked against the constraints vs a single interpreter.. in order to support e.g. 2 + 3. we've learned this the hard way with PEX_PYTHON (vs e.g. PEX_PYTHONS).

This comment has been minimized.

@copumpkin

copumpkin Oct 2, 2017

Contributor

So I'd pass those paths into interpreter_cache.setup()? I can do that

@copumpkin copumpkin force-pushed the copumpkin:fixed-python branch from 6768dad to 6a4082b Oct 2, 2017

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 2, 2017

@kwlzn how's this one?

@@ -62,13 +62,19 @@ def register_options(cls, register):
register('--artifact-cache-dir', advanced=True, default=None, metavar='<dir>',
help='The parent directory for the python artifact cache. '
'If unspecified, a standard path under the workdir is used.')
register('--interpreter-search-paths', advanced=True, type=list, default=[], metavar='<binary-paths>',
help='A list of paths in which to search for python')

This comment has been minimized.

@benjyw

benjyw Oct 3, 2017

Contributor

"To search for python interpreters."

Tiny nit: End the help sentence with a period. For grammar and for consistency...

This comment has been minimized.

@copumpkin

copumpkin Oct 3, 2017

Contributor

@benjyw fixed 😄

Allow users to tell pants where to look for python interpreters
By default it looks in PATH (the old behavior) but you can now tell it
to look elsewhere

@copumpkin copumpkin force-pushed the copumpkin:fixed-python branch from 6a4082b to 17d65ee Oct 3, 2017

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 5, 2017

@kwlzn @benjyw anything else I should change? I've been using this change locally on my 1.3 version and it seems to work fine

@kwlzn

kwlzn approved these changes Oct 5, 2017

Copy link
Member

kwlzn left a comment

this LGTM - how about a quick integration test or two to keep it from regressing later tho?

@benjyw

benjyw approved these changes Oct 5, 2017

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 5, 2017

@kwlzn I wasn't sure how best to test it, since I can't really assume multiple python versions will be present on the test box (or can I?) so it's hard for me to throw together a sample project that would make sure the resolution logic uses the python I want. I could set it to an empty search path and ensure that it complains that it can't find any pythons, but that feels kinda weak.

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented Oct 5, 2017

@copumpkin fair enough - tho there are some tests that target 2.7 + 3.x so I imagine it'd be possible.. but perhaps a couple too many hoops to jump through. can circle back after #4859 affords us better control over the test execution environment.

@kwlzn kwlzn merged commit 6bc4ce9 into pantsbuild:master Oct 5, 2017

1 check passed

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

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 6, 2017

Cool, I'll keep an eye on #4859 then. Thanks again!

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 6, 2017

Oh also, should I backport this to the 1.3 branch? That's what I've been using it on in my team, but unsure if you want to put it into an official release.

@stuhood stuhood added this to the 1.3.1 milestone Oct 9, 2017

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 9, 2017

Thanks @stuhood !

@copumpkin

This comment has been minimized.

Copy link
Contributor

copumpkin commented Oct 17, 2017

@kwlzn @benjyw @stuhood one thing I'm not sure about is how this should interact with the .pants.d cache of the python interpreter. If I change pants.ini to look for a different python version, I assume the cached python path will get updated, but I don't understand the cache more broadly to figure out how it should know about this option getting passed in. Any of you have any advice?

stuhood added a commit that referenced this pull request Nov 3, 2017

Allow users to tell pants where to look for python interpreters (#4930)
Problem

In some situations, the automatic python picker logic picks the wrong version of Python and we don't have much control over that beyond being able to specify version constraints (which sometimes aren't sufficient).

Solution

I added an option to explicitly specify a list of paths to search.

Result

Users can now pass in --python-setup-interpreter-search-paths=<search-paths> and have Pants search in the specified paths. You can even pass in the full path of the interpreter (e.g., which python) and it'll figure that out!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment