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

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer 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

Eric-Arellano
Eric-Arellano previously approved these changes Jan 25, 2019
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

.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 Show resolved Hide resolved
@cosmicexplorer cosmicexplorer force-pushed the auto-clean-all-on-python-version-switch branch 2 times, most recently from b17e576 to 81e0895 Compare January 29, 2019 18:05
@cosmicexplorer cosmicexplorer force-pushed the auto-clean-all-on-python-version-switch branch from b94bbf1 to 963f7ed Compare February 3, 2019 03:46
@cosmicexplorer cosmicexplorer force-pushed the auto-clean-all-on-python-version-switch branch 3 times, most recently from ffb60df to 72aedbf Compare March 10, 2019 05:51
@Eric-Arellano Eric-Arellano dismissed their stale review March 10, 2019 15:33

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

@cosmicexplorer cosmicexplorer force-pushed the auto-clean-all-on-python-version-switch branch from 72aedbf to 5cbeda8 Compare March 10, 2019 19:19
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pants Outdated Show resolved Hide resolved
pants Outdated Show resolved Hide resolved
pants Outdated Show resolved Hide resolved
@cosmicexplorer cosmicexplorer force-pushed the auto-clean-all-on-python-version-switch branch from 8c3f991 to 2efeb79 Compare March 12, 2019 01:24
.gitignore Outdated Show resolved Hide resolved
@cosmicexplorer cosmicexplorer force-pushed the auto-clean-all-on-python-version-switch branch from 66d95ec to 0642281 Compare March 14, 2019 04:37
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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
Copy link
Contributor Author

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 cosmicexplorer merged commit 0672383 into pantsbuild:master Mar 18, 2019
@cosmicexplorer
Copy link
Contributor Author

@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
…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
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants