Add internal options for managing toolchain discovery preferences #4416

zanieb · 2024-06-19T16:55:41Z

Adds support for the toolchain discovery preferences outlined in #4198 but we don't expose this to users yet, I'll do that next to make it easier to review.

I've made some refactors in the toolchain discovery implementation to enable this behavior and move us towards clearer abstractions. There's still remaining work here, but I'd prefer tackle things in follow-ups instead of expanding this pull request. I plan on opening a couple before merging this.

I'd like to shift the public toolchain API to focus on discovering either an environment or a toolchain. The first would be used by commands that operate on an environment, while the latter would be used by commands that just need an interpreter to create environments. I haven't changed this here, but some of the refactors are in preparation for supporting this idea.

In brief:

We now allow different ordering of installed toolchain discovery based on a ToolchainPreference type. This is the type we will expose to users.
SystemPython was changed into an EnvironmentPreference which is used to determine if we should prefer virtual or system Python environments.
We drop the whole ToolchainSources selection concept, it was confusing and the error messages from it were awkward. Most of the functionality is now captured by the preference enums, but you can't do things like "only find a toolchain from the parent interpreter" as easily anymore.

zanieb · 2024-06-19T17:20:49Z

crates/uv/tests/venv.rs

-          × No interpreter found for Python 3.100 in search path
+          × No interpreter found for Python 3.100 in system toolchains


This is less clear but will address separately.

zanieb · 2024-06-19T17:21:23Z

crates/uv/tests/pip_sync.rs

-    success: false
-    exit_code: 2
+    success: true
+    exit_code: 0
    ----- stdout -----

    ----- stderr -----
-    error: No Python interpreters found in virtual environments
+    Resolved 1 package in [TIME]
+    Audited 1 package in [TIME]


Interesting regression... will investigate this.

These tests are not properly isolated from managed toolchains because they aren't following our TestContext pattern :sigh:

zanieb · 2024-06-19T17:23:29Z

crates/uv-toolchain/src/toolchain.rs

-        if let Some(request) = python {
-            Self::find_requested(&request, system, preview, cache)
-        } else if system.is_preferred() {
-            Self::find_default(preview, cache)
-        } else {
-            // First check for a parent interpreter
-            // We gate this check to avoid an extra log message when it is not set
-            if std::env::var_os("UV_INTERNAL__PARENT_INTERPRETER").is_some() {
-                match Self::find_parent_interpreter(system, cache) {
-                    Ok(env) => return Ok(env),
-                    Err(Error::NotFound(_)) => {}
-                    Err(err) => return Err(err),
-                }
-            }
-
-            // Then a virtual environment
-            match Self::find_virtualenv(cache) {
-                Ok(venv) => Ok(venv),
-                Err(Error::NotFound(_)) if system.is_allowed() => {
-                    Self::find_default(preview, cache)
-                }
-                Err(err) => Err(err),
-            }
-        }
+        let request = python.unwrap_or_default();
+        Self::find_requested(&request, environments, preference, cache)


This is a big plus. We don't need to do multiple discovery attempts to get a different ordering. We just say what we want.

zanieb · 2024-06-19T17:34:28Z

crates/uv/tests/pip_sync.rs

+        .env("UV_TEST_PYTHON_PATH", &context.python_path())
+        .env("UV_TOOLCHAIN_DIR", "")


Addresses #4416 (comment)

In preparation for changing the order dynamically in #4416

BurntSushi

Nice!

Was this the refactor you were talking about in DMs? I know you mentioned it being exploratory, but one thing that I find can help here is to put things like type/function renames into one commit separate from the other changes. The renames can touch a lot of code and add a lot of noise to the diff. When split out, the "more interesting" changes can be seen in a different commit.

(I know this isn't always feasible, especially in a more exploratory refactor. But I thought it might be valuable to throw it out there.)

BurntSushi · 2024-06-20T12:40:01Z

crates/uv-toolchain/src/toolchain.rs

-        if let Some(request) = python {
-            Self::find_requested(&request, system, preview, cache)
-        } else if system.is_preferred() {
-            Self::find_default(preview, cache)
-        } else {
-            // First check for a parent interpreter
-            // We gate this check to avoid an extra log message when it is not set
-            if std::env::var_os("UV_INTERNAL__PARENT_INTERPRETER").is_some() {
-                match Self::find_parent_interpreter(system, cache) {
-                    Ok(env) => return Ok(env),
-                    Err(Error::NotFound(_)) => {}
-                    Err(err) => return Err(err),
-                }
-            }
-
-            // Then a virtual environment
-            match Self::find_virtualenv(cache) {
-                Ok(venv) => Ok(venv),
-                Err(Error::NotFound(_)) if system.is_allowed() => {
-                    Self::find_default(preview, cache)
-                }
-                Err(err) => Err(err),
-            }
-        }
+        let request = python.unwrap_or_default();
+        Self::find_requested(&request, environments, preference, cache)


zanieb · 2024-06-20T13:17:13Z

Yeah this is a part of what I was mentioning. A lot of the exploration is figuring out how to add this setting while moving the API towards something that's easy to use correctly. I generally do try to split out renames, but yeah I didn't do a great job of that here :)

Restores the `PythonEnvironment::find` API which was removed a while back in favor of `Toolchain::find`. As mentioned in #4416, I'm attempting to separate the case where you want an active environment from the case where you want an installed toolchain in order to create environments. I wanted to drop `EnvironmentPreference` from `Toolchain::find` and just have us consistently consider (or not consider) virtual environments when discovering toolchains for creating environments. Unfortunately this caused a few things to break so I reverted that change and will explore it separately. Because I was exploring that change, there are some minor changes to the `Toolchain` API here.

…4424) Exposes the option added in #4416. Adds `--toolchain-preference` and `tool.uv.toolchain-preference` to configure if system or managed toolchains are preferred. Users can opt-out of managed toolchains or system toolchains entirely as well.

In preparation for changing the order dynamically in astral-sh#4416

…tral-sh#4416) Adds support for the toolchain discovery preferences outlined in astral-sh#4198 but we don't expose this to users yet, I'll do that next to make it easier to review. I've made some refactors in the toolchain discovery implementation to enable this behavior and move us towards clearer abstractions. There's still remaining work here, but I'd prefer tackle things in follow-ups instead of expanding this pull request. I plan on opening a couple before merging this. I'd like to shift the public toolchain API to focus on discovering either an **environment** or a **toolchain**. The first would be used by commands that operate on an environment, while the latter would be used by commands that just need an interpreter to create environments. I haven't changed this here, but some of the refactors are in preparation for supporting this idea. In brief: - We now allow different ordering of installed toolchain discovery based on a `ToolchainPreference` type. This is the type we will expose to users. - `SystemPython` was changed into an `EnvironmentPreference` which is used to determine if we should prefer virtual or system Python environments. - We drop the whole `ToolchainSources` selection concept, it was confusing and the error messages from it were awkward. Most of the functionality is now captured by the preference enums, but you can't do things like "only find a toolchain from the parent interpreter" as easily anymore.

Restores the `PythonEnvironment::find` API which was removed a while back in favor of `Toolchain::find`. As mentioned in astral-sh#4416, I'm attempting to separate the case where you want an active environment from the case where you want an installed toolchain in order to create environments. I wanted to drop `EnvironmentPreference` from `Toolchain::find` and just have us consistently consider (or not consider) virtual environments when discovering toolchains for creating environments. Unfortunately this caused a few things to break so I reverted that change and will explore it separately. Because I was exploring that change, there are some minor changes to the `Toolchain` API here.

zanieb added the internal A refactor or improvement that is not user-facing label Jun 19, 2024

zanieb changed the title ~~Add setting for managing toolchain discovery preferences~~ Add internal options for managing toolchain discovery preferences Jun 19, 2024

zanieb force-pushed the zb/toolchain-preference branch from 028ae18 to c0411bf Compare June 19, 2024 16:56

zanieb marked this pull request as draft June 19, 2024 16:56

zanieb force-pushed the zb/toolchain-preference branch from c0411bf to 0edfbad Compare June 19, 2024 17:12

zanieb commented Jun 19, 2024

View reviewed changes

zanieb force-pushed the zb/toolchain-preference branch from 0edfbad to 82af472 Compare June 19, 2024 17:34

zanieb commented Jun 19, 2024

View reviewed changes

zanieb mentioned this pull request Jun 19, 2024

Refactor Python executable chain into lazy variables #4318

Merged

zanieb marked this pull request as ready for review June 19, 2024 17:35

zanieb requested review from konstin and BurntSushi June 19, 2024 17:36

Base automatically changed from zb/discover-chain to main June 19, 2024 20:27

zanieb added a commit that referenced this pull request Jun 19, 2024

Refactor Python executable chain into lazy variables (#4318)

34c7bc5

In preparation for changing the order dynamically in #4416

zanieb force-pushed the zb/toolchain-preference branch from 82af472 to e722d0e Compare June 19, 2024 20:30

Add setting for managing toolchain discovery preferences

f971ee4

zanieb force-pushed the zb/toolchain-preference branch from e722d0e to f971ee4 Compare June 19, 2024 20:44

BurntSushi approved these changes Jun 20, 2024

View reviewed changes

zanieb merged commit 13e532c into main Jun 20, 2024
47 checks passed

zanieb deleted the zb/toolchain-preference branch June 20, 2024 13:57

This was referenced Jun 20, 2024

Add PythonEnvironment::find API #4423

Merged

Expose toolchain-preference as a CLI and configuration file option #4424

Merged

ChannyClaus pushed a commit to ChannyClaus/uv that referenced this pull request Jun 20, 2024

Refactor Python executable chain into lazy variables (astral-sh#4318)

8096845

In preparation for changing the order dynamically in astral-sh#4416

zanieb mentioned this pull request Jun 27, 2024

Include environment preferences in the toolchain not found errors #4587

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal options for managing toolchain discovery preferences #4416

Add internal options for managing toolchain discovery preferences #4416

zanieb commented Jun 19, 2024 •

edited

Loading

zanieb Jun 19, 2024

zanieb Jun 19, 2024

zanieb Jun 19, 2024

zanieb Jun 19, 2024

BurntSushi Jun 20, 2024

zanieb Jun 19, 2024

BurntSushi left a comment

BurntSushi Jun 20, 2024

zanieb commented Jun 20, 2024

		× No interpreter found for Python 3.100 in search path
		× No interpreter found for Python 3.100 in system toolchains

		.env("UV_TEST_PYTHON_PATH", &context.python_path())
		.env("UV_TOOLCHAIN_DIR", "")

Add internal options for managing toolchain discovery preferences #4416

Add internal options for managing toolchain discovery preferences #4416

Conversation

zanieb commented Jun 19, 2024 • edited Loading

zanieb Jun 19, 2024

Choose a reason for hiding this comment

zanieb Jun 19, 2024

Choose a reason for hiding this comment

zanieb Jun 19, 2024

Choose a reason for hiding this comment

zanieb Jun 19, 2024

Choose a reason for hiding this comment

BurntSushi Jun 20, 2024

Choose a reason for hiding this comment

zanieb Jun 19, 2024

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

BurntSushi Jun 20, 2024

Choose a reason for hiding this comment

zanieb commented Jun 20, 2024

zanieb commented Jun 19, 2024 •

edited

Loading