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

run clean-all before switching pants python version runner scripts #7151

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Jan 25, 2019

Problem

When running ./pants vs ./pants2, or changing python interpreter version via PY=python3.6 ./pants to PY=python3.7 ./pants, it is currently necessary to run clean-all first before switching which python interpreter is used to run pants. Anyone testing with different python interpreter versions has to remember to run clean-all in between invocations. I find this difficult to remember.

Solution

  • Make ./pants write the python version string to a sentinel file .pants-python-version at the repo root containing the python version we're running with before doing anything else, and if the version differs from the python version we are currently running pants under, run a clean-all.
  • Add ONLY_USING_SINGLE_PYTHON_VERSION env var to CI to avoid running clean-all or needing to generate this sentinel file.

Result

Many weird errors no longer occur when switching between ./pants2 and ./pants, or using the $PY env var to switch between interpreter versions when running pants.

Followup Issues

  • #7355 -- fix reporting not picking up a run if clean-all is one of the goals, which would let us avoid having to make two separate pants invocations here.
@Eric-Arellano
Copy link
Contributor

left a comment

This is awesome!

I wanted to include this functionality when first working on ./pants3, but didn't know how to keep track of the prior interpreter used between processes. Good idea with making an untracked & hidden file.

Thanks for making the developer process this much simpler!

Show resolved Hide resolved .gitignore Outdated
Show resolved Hide resolved pants
Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:auto-clean-all-on-python-version-switch branch 2 times, most recently from b17e576 to 81e0895 Jan 28, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:auto-clean-all-on-python-version-switch branch from b94bbf1 to 963f7ed Feb 3, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:auto-clean-all-on-python-version-switch branch 3 times, most recently from ffb60df to 72aedbf Mar 10, 2019

Will re-review once this is green + some changes requested as discussed over Slack

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:auto-clean-all-on-python-version-switch branch from 72aedbf to 5cbeda8 Mar 10, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

This is so useful! Thank you!

There have been at least 5 times I remember being slowed down by forgetting to to run clean-all, and I know I'm not alone here.

Even once we drop Py2, this will be useful when switching between Py36 and Py37.

@@ -46,4 +46,8 @@ USER ${TRAVIS_USER}:${TRAVIS_GROUP}
# Our newly created user is unlikely to have a sane environment: set a locale at least.
ENV LC_ALL="en_US.UTF-8"

# TODO(#7152): this tells the ./pants runner script to avoid trying to clean the workspace when
# changing python versions. Remove all instances of this after the python 3 migration!
ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 10, 2019

Contributor

Note to other reviewers, we will always duplicate most of this Dockerfile from travis_ci because we don't want Py27 w/ UCS2 on most of our Travis shards. See https://github.com/pantsbuild/pants/blob/master/build-support/docker/travis_ci_py27_ucs2/Dockerfile#L4 for further explanation.

This duplication is frustrating, but necessary until we drop Py2.

Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated

cosmicexplorer added some commits Jan 25, 2019

cosmicexplorer added some commits Mar 10, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:auto-clean-all-on-python-version-switch branch from 8c3f991 to 2efeb79 Mar 12, 2019

Show resolved Hide resolved .gitignore Outdated

cosmicexplorer added some commits Mar 13, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:auto-clean-all-on-python-version-switch branch from 66d95ec to 0642281 Mar 14, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thanks Danny! The structure looks great.

All these comments are simple suggested renames to reflect the new insight we had that PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS is what causes the cache issue, not PY.

pants Outdated
@@ -2,6 +2,39 @@
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

REPO_ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd -P)

_interpreter_version_sentinel_file_path='.pants.d/.pants-python-version'

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

I recommend changing to .python-interpreter-constraints

Also because it's a local variable, I think you can remove the _ here and elsewhere. That's personal style preference though.

Suggested change
_interpreter_version_sentinel_file_path='.pants.d/.pants-python-version'
interpreter_constraints_sentinel_file_path='.pants.d/.python-interpreter-constraints'

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

Removed _ for consistency (thanks for pointing that out, will stick with lowercase in this repo!) and changed to .python-interpreter-constraints!

Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated
Show resolved Hide resolved pants Outdated
Show resolved Hide resolved build-support/travis/travis.yml.mustache Outdated
Show resolved Hide resolved build-support/docker/travis_ci_py36/Dockerfile Outdated
Show resolved Hide resolved build-support/docker/travis_ci_py27_ucs2/Dockerfile Outdated
Show resolved Hide resolved build-support/docker/travis_ci/Dockerfile Outdated
Show resolved Hide resolved .travis.yml Outdated
@Eric-Arellano
Copy link
Contributor

left a comment

Thanks Danny for iterating on this again! Everything looks good to me beyond the one suggested change.

I do also still prefer DISABLE_AUTO_CLEAN_ALL over ONLY_USING_SINGLE_PYTHON_VERSION as I think it's better to not program for some future unknown need. If we end up wanting to use DISABLE_AUTO_CLEAN_ALL for a purpose other than deciding to clean-all, we can rename the env var when it makes sense to do so. The other benefit of using DISABLE_AUTO_CLEAN_ALL is that we could kill off the comments in the Dockerfiles and .travis.yml as the env var is self-explaining—comments rot, so this would be good to be able to do so. I won't block on this though.

Show resolved Hide resolved pants Outdated
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

The other benefit of using DISABLE_AUTO_CLEAN_ALL is that we could kill off the comments in the Dockerfiles and .travis.yml as the env var is self-explaining—comments rot, so this would be good to be able to do so. I won't block on this though.

The env var self-explains "what" but not "why", and I would argue that using DISABLE_AUTO_CLEAN_ALL is significantly more mysterious than using ONLY_USING_SINGLE_PYTHON_VERSION if both are in the absence of comments. DISABLE_AUTO_CLEAN_ALL to me feels like it could eventually become unnecessary, and then become extremely mysterious, because it's not clear what might be relying on that behavior, which ONLY_USING_SINGLE_PYTHON_VERSION feels far less prone to. I don't consider that to be programming defensively for some future need. I'm also less concerned about comments rotting in this case because fewer edits happen to these files, especially Dockerfiles, which makes git blame more useful. Also, if we need to have other behavior based on ONLY_USING_SINGLE_PYTHON_VERSION, we don't have to change anything at all, which makes this mildly-complex bit of python version interaction much easier to follow and review -- I don't see the same being true for DISABLE_AUTO_CLEAN_ALL.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Also @stuhood I realized a reason we might not want to put this in .pants.d/ -- because then we would end up running a clean-all again after every manual clean-all, because clean-all would also delete the interpreter constraints file. I'll put this back in .gitignore -- this is a bit of a hack but I don't think it's necessarily more of a hack than e.g. the rust-toolchain file.

cosmicexplorer added some commits Mar 16, 2019

move interpreter constraints file out of .pants.d AND make interprete…
…r constraints a parameter to the clean-all check function

@cosmicexplorer cosmicexplorer merged commit 0672383 into pantsbuild:master Mar 18, 2019

1 check passed

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@Eric-Arellano the DISABLE_AUTO_CLEAN_ALL vs ONLY_USING_SINGLE_PYTHON_VERSION question should be considered as remaining open, pending any future changes that might cause us to favor one or the other.

cosmicexplorer added a commit that referenced this pull request Apr 9, 2019

avoid running a clean-all in integration tests, which are in a new bu…
…ildroot (#7522)

### Problem

Running the below command results in a `clean-all` being run before the integration test:
```
> ./pants2 test tests/python/pants_test/backend/python/tasks:: -- -s -k test_platform_defaults_to_config
...
10:32:53 00:06     [pytest]
                   Invalidated 21 targets.
                   scrubbed PYTHONPATH=/Users/dmcclanahan/tools/pants/src/python: from py.test environment
10:32:53 00:06       [run]
                     ============== test session starts ===============
                     platform darwin -- Python 2.7.10, pytest-3.6.4, py-1.7.0, pluggy-0.7.1
                     rootdir: /Users/dmcclanahan/tools/pants/.pants.d, inifile: /Users/dmcclanahan/tools/pants/.pants.d/test/pytest-prep/CPython-2.7.10/718d17b455ce5917535680e933c8a655f6334861/pytest.ini
                     plugins: timeout-1.2.1, cov-2.4.0
                     collected 142 items / 141 deselected

                     tests/python/pants_test/backend/python/tasks/test_python_binary_integration.py .python-interpreter-constraints not found in the buildroot.
                     Forcing an initial clean-all.
                     Different interpreter constraints were set than were used in the previous Pants run.
                     Clearing the cache and preparing a Python environment with these interpreter constraints:
                     ['CPython>=2.7,<3','CPython>=3.6,<4']
...
```
This is an artifact of the changes for #7151, which tries to be safe and clean-all if the `.python-interpreter-constraints` file doesn't exist to ensure subprocesses are invoked with the right python version. But we don't need to do this for integration tests, since they're invoked in a new buildroot.

### Solution

- Add `ONLY_USING_SINGLE_PYTHON_VERSION=true` to the env in all integration tests.

### Result

Pants will no longer run a clean-all before integration tests in a new buildroot.

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

Rerun `select-interpreter` if global Python interpreter constraints h…
…ave changed (#7586)

### Problem
Currently we only check for changes to the file's `compatibility` entry in its `BUILD` file, but really we should be also checking global constraints.

As it stands now, changing `--python-setup-interpreter-constraints` will have no impact until running `./pants clean-all`, which is not meant to be.

This closes #7510.

### Solution
Use `PythonSetup`'s `compatibility_or_constraints` in the fingerprint strategy, which first checks if the targets have `compatibility` marked in their BUILD entries, and then falls back to the global interpreter constraints.

#### Alternative solution: invalidate upon subsystem changes
Alternatively, we could add `self.fingerprint` to our file naming scheme, so that _any_ invalidation to `PythonSetup` or to `PythonInterpreterCache` would also invalidate this task. See d35e80c#diff-3d23a0b5af958441c095d5acd551ac16R101 for this implementation.

We did not go down this route because it would result in more invalidation than necessary. This task is only ever impacted by changes to `--python-setup-interpreter-constraints` and `compatibility` entries.

#### Also revert auto clean-all
#7151 and #7522 were workarounds to this underlying issue.

### Result
Calling `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.6.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil` the first time will cause an invalidation, then the interpreter selection task will not run upon subsequent invocations.

Running `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==2.7.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil` right after will properly cause an invalidation and result in Python 2.7 being used for the tests.
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.