-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
run clean-all before switching pants python version runner scripts #7151
Conversation
There was a problem hiding this 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!
b17e576
to
81e0895
Compare
b94bbf1
to
963f7ed
Compare
ffb60df
to
72aedbf
Compare
Will re-review once this is green + some changes requested as discussed over Slack
72aedbf
to
5cbeda8
Compare
There was a problem hiding this 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' |
There was a problem hiding this comment.
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.
This reverts commit 963f7ed.
8c3f991
to
2efeb79
Compare
66d95ec
to
0642281
Compare
…nv vars in the travis config
There was a problem hiding this 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' |
There was a problem hiding this comment.
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.
_interpreter_version_sentinel_file_path='.pants.d/.pants-python-version' | |
interpreter_constraints_sentinel_file_path='.pants.d/.python-interpreter-constraints' |
There was a problem hiding this comment.
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
!
There was a problem hiding this 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.
The env var self-explains "what" but not "why", and I would argue that using |
Also @stuhood I realized a reason we might not want to put this in |
…r constraints a parameter to the clean-all check function
@Eric-Arellano the |
…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.
…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.
Problem
When running
./pants
vs./pants2
, or changing python interpreter version viaPY=python3.6 ./pants
toPY=python3.7 ./pants
, it is currently necessary to runclean-all
first before switching which python interpreter is used to run pants. Anyone testing with different python interpreter versions has to remember to runclean-all
in between invocations. I find this difficult to remember.Solution
./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 aclean-all
.ONLY_USING_SINGLE_PYTHON_VERSION
env var to CI to avoid runningclean-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
clean-all
is one of the goals, which would let us avoid having to make two separate pants invocations here.