Skip to content
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

Determine default Python interpreter based on the pants_version being used #49

Merged
merged 12 commits into from Apr 13, 2019

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 29, 2019

Problem

Now that using Python 2.7 is ~deprecated and support will be removed soon, we need to stop defaulting to 2.7.

Defaulting to Python 3 will make for a less stark transition. Specifically, users will be able to leave out pants_runtime_python_version from their pants.ini, which is good because we will deprecate this option once we drop Py2.

However, we do not want to break backwards compatibility. A user should be able to downgrade to 1.14.0 without issues.

Solution

Determine which pants_version the user has pinned, and set the default interpreter from there, as follows:

  • <= 1.14.0: Python 2.7
  • == 1.15.0: Python 3.6->2.7
  • == 1.16.0: Python 3.6->3.7->2.7
  • == 1.17.0: Python 3.6->3.7->3.8->3.9
  • > 1.17.0: Python 3.9->3.8->3.7->3.6

Notably, we first default to Python 3 and only fall back to Python 2 when necessary. We do this because we want most people using Python 3 so that we can completely stress test it before dropping Python 2.

For versions > 1.17.0, we default to using the max interpreter possible and only fall back to 3.6 as a last resort. This allows us to make no assumptions about Pants supporting 3.6 in the future.

If the user has not pinned pants_version, such as happens with first time users, we use the default interpreters for the current major release. With 1.15.0, this means we would use 3.6 or 2.7. Until 1.18.0 is released and our migration is stabilized, we will update the default case in the ./pants script to match that new release. This should only impact new users before they run the ./pants generate-pants-ini command.

We also avoid the ambiguous python3 bin name to avoid accidentally trying to use Python 3.5, for example.

@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

I've held off review since the CI failures look legit and I can't suss the bug reading the diff.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Ah my bad - should have mentioned I expect CI to fail until 1.15.0 is released. Pants doesn't work with Python 3 when using 1.14.0, which is what an unconfigured pants_version defaults to.

Note everything passes except for --pants-version unspecified --python-version unspecified.

--

My intent in requesting a review was so that this is ready for me to merge once 1.15.0 is released and we get green CI.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Important clarification with this PR. Users who already have the script won't be impacted by the change, unless they curl the script again. This is a good thing, so that we don't change their behavior for 1.15.0 overnight.

Now, thanks to this PR, we will be able to simplify our instructions for current users to upgrade to Python 3 to simply:

  1. curl -L -O https://pantsbuild.github.io/setup/pants for the changes we made to ./pants to default to using Python 3.6 and then to fall back to Python 2.7.
  2. Change the pants_version in pants.ini to 1.15.0.
  3. Run ./pants.
  4. Save your changes to ./pants and pants.ini to your organization's version control.

Eric-Arellano added a commit to pantsbuild/pants that referenced this pull request Apr 1, 2019

No longer pin `pants_runtime_python_version` with `./pants generate-p…
…ants-ini` (#7469)

In pantsbuild/setup#49, we're changing the setup repo's `./pants` script to default to Python 3.6, and then fall back to Python 2.7. Thanks to that, we no longer need to use `pants_runtime_python_version` to get the script to use Python 3.

In fact, we should not set `pants_runtime_python_version`. The Python version used is an implementation detail we don't want users to have to worry about, and we will be deprecating the option in 1.17.0.dev0. So, us auto-generating the option when it is not even necessary is asking for confusion and a worse experience for users.

stuhood added a commit to pantsbuild/pants that referenced this pull request Apr 1, 2019

No longer pin `pants_runtime_python_version` with `./pants generate-p…
…ants-ini` (#7469)

In pantsbuild/setup#49, we're changing the setup repo's `./pants` script to default to Python 3.6, and then fall back to Python 2.7. Thanks to that, we no longer need to use `pants_runtime_python_version` to get the script to use Python 3.

In fact, we should not set `pants_runtime_python_version`. The Python version used is an implementation detail we don't want users to have to worry about, and we will be deprecating the option in 1.17.0.dev0. So, us auto-generating the option when it is not even necessary is asking for confusion and a worse experience for users.
@stuhood
Copy link
Member

left a comment

Is it possible (and/or worthwhile) to choose the default based on which side of the 1.15.0 split you are on? Presumably that would require the script to parse pants version numbers though.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@stuhood good idea! I would propose not only checking against 1.15.0, but also 1.16.0 and 1.17.0 splits, as follows:

if pants_version <= 1.14.0.x:
  python 2.7 -> die
elif pants_version == 1.15.0.x:
  python 3.6 -> python2.7 -> die
elif pants_version == 1.16.0.x:
  python 3.6 -> python 3.7 -> python2.7 -> die
elif pants_version >= 1.17.0.x:
  python3.6 -> python3.7 -> python3.8 -> python3 -> die
else:
  python 3.6 -> python2.7 -> die

At the cost of more complex code, we would have significantly less churn in this repo. Because people rarely update their ./pants script, I think that is a major benefit.

For example, with this scheme, new users who download the script at 1.16.0 will no longer have the risk of the ./pants script trying to use Python 2.7 when they upgrade to 1.17.0.

--

Foil here though, what to do when pants_version is not defined? When undefined, Pip determines which pants_version to use and we definitely don't want to rely on that result to dynamically change the default interpreter.

With this question, note that we no longer pin pants_runtime_python_version when running ./pants generate-pants-ini, so the interpreter we use that first time is a bit less relevant.

--

@jsirois is this getting too complex, do you think? Or is the reduction in churn + allowing users to downgrade versions without issues worth it?

@stuhood

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

When undefined, Pip determines which pants_version to use and we definitely don't want to rely on that result to dynamically change the default interpreter.

Hm... I suspect that we do? Pip defaults to "the latest stable version", which is a good default, and a reasonable way to choose the default python as well.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

When undefined, Pip determines which pants_version to use and we definitely don't want to rely on that result to dynamically change the default interpreter.

Hm... I suspect that we do? Pip defaults to "the latest stable version", which is a good default, and a reasonable way to choose the default python as well.

The issue is technical with the execution order.

setup/pants

Lines 74 to 79 in 3363481

# The high-level flow:
# 1.) Resolve the Python interpreter, first reading from the env var $PYTHON,
# then reading from pants.ini, then defaulting to `python2.7`.
# 2.) Grab pants version from pants.ini or default to latest.
# 3.) Check for a venv via a naming/path convention and execute if found.
# 4.) Otherwise create venv and re-exec self.

Pip requires the virtual env to already be set up, and pip corresponds to a specific Python version. Let's say 1.17.0 is now the stable release—then trying to use pip2 to install Pants and subsequently to determine which pants_version we're on will fail.

--

Instead, what I propose is based on the assumption that the majority of users will have pants_version pinned. In both our old and new documentation, the very first thing we say to do is to pin pants_version.

Given this assumption, the only time this undefined pants_version case matters is for first time users before they run ./pants generate-pants-ini. After that first run, they will have pants_version defined so above behavior means Pants will always work regardless of their pants_version.

So, for the undefined pants_version case, I propose that we change the defaults as we release each stable major version. That means in 1.15.0, we change it to python3.6->python2.7->die, then in 1.16.0 we change the script again to python3.6->python3.7->python2.7->die, and so on.

While this has some churn in the script, the key detail is pre-existing users have no need to re-curl the script. These updates are only expected to impact first-time users.

Eric-Arellano added some commits Apr 2, 2019

Extend ci.py to allow testing 1.14.0, 1.15.0, 1.16.0, and 1.17.0.
Each of these introduce significant churn in terms of which Python interpreters are supported. Now that the `./pants` script will set its default interpreter based on each of these pants_version, we should be testing each of them distinctly.

We also still test that `unspecified` works. This should be in the same .travis.yml entry as the most recent stable release.

@Eric-Arellano Eric-Arellano changed the title Default to using Python 3.6 then 2.7 Determine default Python interpreter based on the pants_version being used Apr 2, 2019

@Eric-Arellano Eric-Arellano requested review from jsirois and stuhood Apr 2, 2019

@stuhood
Copy link
Member

left a comment

Thanks!

Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated
@cosmicexplorer
Copy link

left a comment

This is an inspired change which is thoughtfully implemented!

Show resolved Hide resolved pants
Show resolved Hide resolved pants Outdated

Eric-Arellano added some commits Apr 2, 2019

Add 2.0 logic and address concern about Py39
* Check if pants_version is 2.*.*. Stu pointed out that the prior implementation would default to Python2 for those releases, which is a non-starter.
* For >= 1.17.0, add 3.9 to the list of accepted interpreters. It's been stated that Python 3.9 will be the last in the 3.x series, so by including 3.8 and 3.9 we cover all our bases.
* Add comment explaining else statement means <= 1.14.0.
Show resolved Hide resolved pants
Show resolved Hide resolved pants
For versions > 1.17.x, no longer assume Py36 support
We keep in in the list of possible interpreters, but go from max to min, i.e. 3.9->3.8->3.7->3.6. This is because we will always have a lower limit, but claim to not have an upper limit (beyond <4). If the user does not have 3.8 or 3.9 on their system, e.g. they aren't even released, we'll simply fall back to the prior version.

This reduces the risk of us dropping Py36 and then the `./pants` script being outdated for all our users as a result.

Eric-Arellano added some commits Apr 2, 2019

Invert version check order
By going from 1.14->...->1.17, we are able to else to anything >= 1.17.0. This allows us to de-duplicate how we treat >= 1.17 and == 2.*.*, and to thus de-dent the clauses.
@stuhood

stuhood approved these changes Apr 3, 2019

Copy link
Member

left a comment

Thank you Eric.

<= 1.14.0 only supports Py27
Issue introduced when chhanging what was the default case and what wasn't.

@Eric-Arellano Eric-Arellano merged commit 8fca2f4 into pantsbuild:gh-pages Apr 13, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:default-py3 branch Apr 13, 2019

Eric-Arellano added a commit that referenced this pull request Apr 25, 2019

Dynamically determine the Pants version when not specified (#51)
### Problem
When following our install guide https://www.pantsbuild.org/install.html#recommended-installation, we end up bootstrapping *two* virtual environments and logging this to the user, which is confusing for your very first interaction with Pants.

This is because the script names the venv folder as `unspecified_py*` when `pants.ini` does not have a version pinned: https://github.com/pantsbuild/setup/blob/8fca2f4a51ddc65c931d4aba4ec3fbc1acf27234/pants#L154-L158.

### Solution
PyPi has a JSON API that we can ping to determine dynamically what the most recent stable release is. This is the same version `pip` will resolve to, so we simply name the venv folder this.

This also allows us to change our execution flow, so that we first determine the Pants version and _then_ determine the Python version, rather than the reverse. The benefit here is that the Python function solely depends on the Pants version, as of #49, and now the execution flow reflects this properly.

#### Testing
We add tests to ensure this all works.

In doing so, we add a new infrastructure for hermitic tests. Rather than acting directly on this repo's `pants.ini`, we use temporary directories to create a new, clean environment every run. This infrastructure will be used to refactor and improve the rest of this repo's tests in followup PRs.

### Result
If the `pants_version` is left off of `pants.ini`, Pants will setup the venv folder to use the most recent stable release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.