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 toolchain and environment missing error messages #4596

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jun 27, 2024

The journey here can be seen in:

I collapsed all the commits here because only the last one in the stack got us to a "correct" error message.

There are a few architectural changes:

  • We have a dedicated MissingEnvironment and EnvironmentNotFound type for PythonEnvironment::find allowing different error messages when searching for environments
  • ToolchainNotFound becomes a struct with the ToolchainRequest which greatly simplifies missing toolchain error formatting
  • ToolchainNotFound tracks the EnvironmentPreference so it can accurately report the locations checked

The messages look like this now, instead of the bland (and often incorrect): "No Python interpreter found in system toolchains".

❯ cargo run -q -- pip sync requirements.txt
error: No virtual environment found
❯ UV_TEST_PYTHON_PATH="" cargo run -q -- pip sync requirements.txt --system
error: No system environment found
❯ UV_TEST_PYTHON_PATH="" cargo run -q -- pip sync requirements.txt --python 3.12
error: No virtual environment found for Python 3.12
❯ UV_TEST_PYTHON_PATH="" cargo run -q -- pip sync requirements.txt --python 3.12 --system
error: No system environment found for Python 3.12
❯ UV_TEST_PYTHON_PATH="" cargo run -q -- toolchain find 3.12 --preview
error: No toolchain found for Python 3.12 in system path
❯ UV_TEST_PYTHON_PATH="" cargo run -q -- pip compile requirements.in
error: No toolchain found in virtual environments or system path

I'd like to follow this with hints, suggesting creating an environment or using system in some cases.

@zanieb zanieb added the error messages Messaging when something goes wrong label Jun 27, 2024
#[derive(Clone, Debug, Error)]
pub enum ToolchainNotFound {
Copy link
Member Author

Choose a reason for hiding this comment

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

The enum is now a struct which contains the ToolchainRequest instead of de-structuring the request here.

Comment on lines -133 to -136
/// The requested directory path does not exist.
DirectoryNotFound(PathBuf),
/// No Python executables could be found in the requested directory.
ExecutableNotFoundInDirectory(PathBuf, PathBuf),
Copy link
Member Author

Choose a reason for hiding this comment

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

We lose the ability to distinguish between these cases in error messages, but I don't think it's particularly critical.

Copy link
Member

Choose a reason for hiding this comment

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

By "these cases", do you mean file vs. directory vs. executable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically whether its a missing directory or a missing executable in the directory. We'll always raise the latter now.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would also be harder to encode things like "this exists but is not an executable" in the future. In practice, I don't think we were even using the variant right now.

@zanieb zanieb marked this pull request as ready for review June 27, 2024 18:40
@zanieb zanieb force-pushed the zb/toolchain-errors branch 2 times, most recently from 24c9f0d to 1e84c62 Compare June 27, 2024 18:49
@zanieb zanieb requested a review from BurntSushi June 27, 2024 19:03
@@ -264,7 +264,7 @@ fn create_venv_unknown_python_minor() {
----- stdout -----

----- stderr -----
× No interpreter found for Python 3.100 in system toolchains
× No toolchain found for Python 3.100 in system path or `py` launcher
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on "toolchain" as user-facing copy vs. interpreter? I feel like interpreter is more familiar to folks but seeing that you proactively changed it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, but I'd like to be consistent. We're going to talk about managed toolchains a lot going forward so I think we might want to move in that direction.

A nuance is that we look for an interpreter in order to discover a toolchain. I guess that leans towards using "interpreter" here.

Copy link
Member

Choose a reason for hiding this comment

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

I weakly prefer interpreter I think. It seems likely to land better with Python users. But the inconsistency with using "toolchain" in written docs does seem unfortunate. How crazy would it be to switch to "interpreter" everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well.. I talked about that in the initial design and @konstin did clarify that there's much more than the interpreter involved e.g. the managed toolchains include the standard library.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this change, I'll use "interpreter" because I want to get these changes out asap.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah I hear ya. If y'all already dug into the naming here then I defer to you. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have yet to be convinced that "toolchain" is the best user-facing concept :) we should all chat more.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let sources = match self.environment_preference {
EnvironmentPreference::Any => {
format!("virtual environments or {}", self.toolchain_preference)
Copy link
Member

Choose a reason for hiding this comment

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

Would this be "virtual environments or managed or system toolchains"? I feel like it should be Oxford comma'd but I know that's tedious...

Copy link
Member Author

Choose a reason for hiding this comment

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

😬 I know right. I considered doing an extra match to fix this.... but I didn't have it in me. Think it's worth it? I might want to construct a test covering this so I know it matters if I do it.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

So the basic idea here is that rather than creating a ToolchainNotFound variant, we base the error message on the request itself? Makes sense to me.

What ToolchainRequest was being returned such that I saw that "error: No Python interpreters found in system toolchains" message in my ruff-lsp PR?

@zanieb
Copy link
Member Author

zanieb commented Jun 27, 2024

So the basic idea here is that rather than creating a ToolchainNotFound variant, we base the error message on the request itself? Makes sense to me.

Yep, though that kind of just fell out of me making some other changes to actually propagate the information for correct error messages.

What ToolchainRequest was being returned such that I saw that "error: No Python interpreters found in system toolchains" message in my ruff-lsp PR?

This was because the ToolchainNotFound didn't track EnvironmentPreference so when constructing the error message we only had the ToolchainPreference and did something dumb. In your case, it was probably ToolchainRequest::Any, EnvironmentPreference::ExplicitSystem, and ToolchainPreference::OnlySystem (because we don't allow mutation of managed toolchains).

@@ -264,7 +264,7 @@ fn create_venv_unknown_python_minor() {
----- stdout -----

----- stderr -----
× No interpreter found for Python 3.100 in system toolchains
× No toolchain found for Python 3.100 in system path or `py` launcher
Copy link
Member

Choose a reason for hiding this comment

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

I weakly prefer interpreter I think. It seems likely to land better with Python users. But the inconsistency with using "toolchain" in written docs does seem unfortunate. How crazy would it be to switch to "interpreter" everywhere?

@zanieb zanieb enabled auto-merge (squash) June 28, 2024 15:10
@zanieb zanieb merged commit 3a62ba3 into main Jun 28, 2024
47 checks passed
@zanieb zanieb deleted the zb/toolchain-errors branch June 28, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Messaging when something goes wrong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants