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

Default to pants devs using Python 3 by renaming `./pants3`->`./pants` and `./pants`->`./pants2` #7257

Merged
merged 8 commits into from Mar 1, 2019

Conversation

Projects
6 participants
@Eric-Arellano
Copy link
Contributor

commented Feb 19, 2019

Problem

We should internally default to using Python 3, as we will in two major releases require Python 3 to run.

Further, we want to make this change before officially releasing a Python 3 wheel so that we can internally dogfood the migration before others consume it.

Solution

Rename ./pants3 -> ./pants and ./pants -> ./pants2. This means ./pants will default to using Python 3, and ./pants2 will default to using Python 2.

Also update ci.sh to properly set PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS. We now allow the caller of the script to pre-set the value (along with $PY), and default to always setting the constraint to the exact Python interpreter used for $PY. This allows us to ensure we always use the correct interpreter, while allowing certain tests to override the values, such as the Py2 blacklist shard to use a Py2 Pex with Py3 interpreter constraints.

Result

  • Running ./pants now defaults to unconstrained Python 3, i.e. Python 3.6 or 3.7.
  • Running PY=python3.7 ./pants runs constrained Python 3.7. Same with PY=python3.6 running Python 3.6.
  • Running ./pants2 defaults to running Python 2.7 with interpreter constraints allowing for either Python 2 or Python 3 for spawned subprocesses such as unit tests.

Not impacted by change

Note this does not impact end consumers of Pants, only Pants developers working directly on this repository.

Eric-Arellano added some commits Feb 19, 2019

Rename pants3->pants and pants->pants2
We want to default to running with Python 3, as this will be the only way we can run Pants in 2 major releases.

We are making this change now for Pants developers—before we release Python 3 to the public—as it will help us to discover issues before then.
Improve wording
* Avoid the word "it".
* Clarify why we need to set exact constraints for PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS.
@benjyw

benjyw approved these changes Feb 19, 2019

Copy link
Contributor

left a comment

EPIC!

@illicitonion
Copy link
Contributor

left a comment

🎉

@Eric-Arellano Eric-Arellano added this to In progress in Adding Python 3 support Feb 19, 2019

Eric-Arellano added some commits Feb 27, 2019

Ignore Pex deprecation warnings
I've been waiting to land this PR for the upgrade to Pex 1.6.* as to avoid a regex DeprecationWarning anytime you run with Py37. That upgrade ran into an issue that needs to be broken out into a new Pex release.

In the meantime, we can simply ignore the DeprecationWarning and remove this new line as a part of the Pex upgrade PR.

Will merge this PR after a green CI run!
Cherry-pick CI changes from #7235
I realized the original ci.sh would not work as intended! We would always hardcode PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS with `-2` to be >2.7,<4. This means our blacklist shard would not be able to properly constrain to >=3.6, and would actually be running with 2.7. The tests were all green, so we didn't notice this.

Instead, we copy the code from #7235 (py2 abi specified), as I've iterated on ci.sh a couple distinct times there until finally discovering this as the best version. These changes make sense to have here, because we do need to change how we configure the interpreter constraints, so might as well do it right the first time here.

@Eric-Arellano Eric-Arellano requested review from stuhood, jsirois, illicitonion and benjyw and removed request for stuhood Feb 27, 2019

pants Outdated
# Pants will try to use Python 2 for subprocesses. Without setting minor version constraints, when using Python 3
# we could end up using a different Python minor version for subprocesses than we do for Pants.
py_major_minor=$(${PY} -c 'import sys; print(".".join(map(str, sys.version_info[0:2])))')
export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor}.*']}"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Feb 27, 2019

Author Contributor

Note: here we allow any patch version, but in ci.sh we determined we must specify the patch version.

This was discovered while working on #7235. There, we have system Python 2.7 already, but must install a new version of Python 2.7 to get different unicode settings (UCS2 vs UCS4). We were running into issues with the interpreter changing between what was used to run Pants and what we used for subprocesses.

Should we default to constraining to the exact patch version too? Because this constraint is determined dynamically at runtime, I would advocate yes. The dynamic determination means that my computer could be using 3.7.1 and another's could be using 3.7.2, for example, without any issues. All it ensures is that that Pants run uses the exact same interpreter for subprocesses that it used for $PY / the virtualenv.

This comment has been minimized.

Copy link
@stuhood

stuhood Feb 27, 2019

Member

It doesn't feel like requiring a precise version in the pants script is strictly necessary... since the tests themselves don't require a precise version, and since the tests should run under some stableish version, even if pants is running under something else.

But it doesn't seem like it hurts to force them to match, so if it's caused you issues in that regard, it seems fine to align them. It's also possible that requiring a precise version only in CI (via the ci.sh/travis.yml) would be sufficient.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Feb 27, 2019

Author Contributor

I ended up making the change to require the exact patch version because while it's most often not important which interpreter it uses for tests—so long as they're the same minor version—there is a distinct case this would matter: interpreters with different UCS settings (i.e. how they store unicode).

For example, assume we have Python 2.7.13 built with UCS4 and have 2.7.15 built with UCS2. There is a meaningful difference between 2.7.13 vs. 2.7.15 here, due to the unicode setting. So, here, it does matter that we always consistently use 2.7.13 completely or 2.7.15 completely, and do not mix the two. This is what is going on in our ci.sh for #7235.

I can't think of a situation where it would be wrong to not have this guarantee that $PY interpreter is the same interpreter for subprocesses. Especially because we have the safety valve of letting the user pre-define PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS, I think this is the more correct implementation.

Constrain interpreter to patch version
See https://github.com/pantsbuild/pants/pull/7257/files#r260840111. This ensures the message "set interpreter constraints to exactly match the interpreter used for the venv" is actually true.

@Eric-Arellano Eric-Arellano merged commit df7385d into pantsbuild:master Mar 1, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:default-pants3 branch Mar 1, 2019

Eric-Arellano added a commit that referenced this pull request Mar 7, 2019

Fix ./pants2 overriding preset interpreter constraints (#7334)
### Problem
`./pants2` would always override `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS`. This is not always desired, e.g. when we want to specify you must use Python 2.7.15 instead of 2.7.13.

This fixes an issue introduced by #7257.

### Solution
Allow the env var to be pre-set, else default to allowing Py2.7 or 3.6+.

Eric-Arellano added a commit that referenced this pull request Mar 7, 2019

Fix regression to python interpreter selection test from #7257 (#7333)
### Problem
#7257 resulted in `test_pytest_run.py::test_target_constraints_with_no_sources` failing because `ci.sh` now sets interpreter constraints for Py2 runs whereas it earlier did not set any. So, the test is pulling in the global interpreter constraints and [failing to find an acceptable interpreter](https://travis-ci.org/pantsbuild/pants/jobs/502998214#L1310).

### Solution
Set the test to be hermetic, so that external dependencies aren't pulled in.

### Result
`PY=python2.7 PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==2.7.15']" ./pants clean-all test tests/python/pants_test/backend/python/tasks:integration -- -k test_target_constraints_with_no_sources` now passes.

Hence, the cron job should now be fixed.

@Eric-Arellano Eric-Arellano moved this from In progress to Done in Adding Python 3 support Mar 8, 2019

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.