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

Improve error message when no valid Python interpreter can be resolved #7628

Merged

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Apr 26, 2019

Problem

Currently, when a valid interpreter cannot be found, such as by running PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.9.*']" ./pants test tests/python/pants_test/util:strutil, we will print something like this:

FAILURE: Unable to detect a suitable interpreter for compatibilities: CPython==3.9.* (Conflicting targets: tests/python/pants_test/util:strutil)

A user reported that they were very confused by this message. It seems the main issue was not explaining what to do to fix it, which led to them searching through GitHub and getting confused with our Python 3 migration efforts (e.g. pants_runtime_python_version) vs. setting constraints for your own code.

The other motivating factor is our setup repo failing to find a Python 3.6+ interpreter, despite us knowing for a fact that it is installed and on the PATH. https://travis-ci.org/pantsbuild/setup/jobs/524685486#L355. Here, it would be extremely helpful if we knew what Pants is resolving.

Solution

Improve the message to do two things:

  • Provide several suggestions to fix the issue. We do not settle on one, because any of these 3 suggestions may be relevant.
  • Print all the interpreters that are discovered by Pants.

Result

Now running PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.9.*']" ./pants test tests/python/pants_test/util:strutil results in this more helpful message:

FAILURE: Unable to detect a suitable interpreter for compatibilities: CPython==3.9.* (Conflicting targets: tests/python/pants_test/util:strutil)

Pants detected these interpreter versions on your system: CPython-2.7.10, CPython-2.7.15, CPython-3.6.8, CPython-3.7.1, CPython-3.7.3

Possible ways to fix this:
* Modify your Python interpreter constraints by following https://www.pantsbuild.org/python_readme.html#configure-the-python-version.
* Ensure the targeted Python version is installed and discoverable.
* Modify Pants' interpreter search paths via --pants-setup-interpreter-search-paths.

# NB: self.setup() requires filters to be passed, or else it will use the global interpreter
# constraints. We allow any interpreter other than CPython 3.0-3.3, which is known to choke
# with Pants.
for interpreter in self.setup(filters=("CPython<3", "CPython>=3.3", "PyPy"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewers, am I missing any other Python versions that Pants supports? IronPy or anything maybe?

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thank you!

While it would be great to have a test for this branch of the code itself (it's fairly large!), I'm fine with merging as is, since this is a huge improvement.

Would you mind following up to add an integration test covering this?

@stuhood stuhood merged commit 995f5ba into pantsbuild:master Apr 26, 2019
@stuhood
Copy link
Sponsor Member

stuhood commented Apr 26, 2019

I verified the case that you mentioned in the description, and merged. Thanks!

@Eric-Arellano
Copy link
Contributor Author

Would you mind following up to add an integration test covering this?

Happy to! Although I have no idea how to do so hermetically. The main functionality we should test is grabbing all of the interpreters on the system. I don't know how to do this in a way that works on multiple platforms.

One idea, use subprocess.run(['which', '-a', 'python'])? Although we would need some way to convert the results to the format CPython-2.7.10..

Any recommendations?

@Eric-Arellano Eric-Arellano deleted the better-interpreter-selection-failure branch April 27, 2019 00:52
@stuhood
Copy link
Sponsor Member

stuhood commented Apr 27, 2019

@Eric-Arellano : I would just do the same thing you did in the description: add an integration test with a(n even more) ridiculous python version, and confirm that you see a relevant snippet from your error message.

@Eric-Arellano
Copy link
Contributor Author

and confirm that you see a relevant snippet from your error message.

We wouldn't be testing that we are grabbing all interpreters, though. Is this okay?

What we would end up testing is

  1. this code branch ever triggers
  2. we output the target
  3. we output the requested compatibility
  4. we output at least some interpreters discovered on the system. Would not try to guess which are returned, just that "CPython" is in the output for example

Definitely value in this, just not testing the whole thing.

@stuhood
Copy link
Sponsor Member

stuhood commented Apr 27, 2019

We wouldn't be testing that we are grabbing all interpreters, though. Is this okay?

Yes, IMO. I'm more concerned with the whole codepath breaking due to an undefined variable or something.

Eric-Arellano added a commit that referenced this pull request Apr 29, 2019
…preter can be resolved (#7630)

While we currently do check that the branch for no valid interpreter being detected works, the check is not very comprehensive. 

In #7628, we made the logged error message even more complex, so it becomes even more important to check that we are logging the error message correctly.
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.

None yet

2 participants