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

uv-tool/install: ignore existing environments on interpreter mismatch #7451

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Sep 17, 2024

This changes uv tool install behavior with regards to re-using existing environments.
In particular, this replaces the existing version-matching logic with a tighter one, enforcing
a same-interpreter match.
This allows to properly switch between system and managed interpreter, at the cost of
more eagerly invalidating existing environments every time there is an interpreter change.

Closes: #7320

@@ -204,7 +204,7 @@ impl InstalledTools {
match PythonEnvironment::from_root(&environment_path, cache) {
Ok(venv) => {
debug!(
"Using existing environment for tool `{name}`: {}",
"Found existing environment for tool `{name}`: {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: drive-by change to make debug logs more uniform. After this, the output now looks like this:

DEBUG Found existing environment for tool `poetry`: .local/share/uv/tools/poetry
DEBUG Found existing interpreter for tool `poetry`: /home/lucab/.local/share/uv/tools/poetry/bin/python3

@lucab lucab force-pushed the ups/tool-install-interp-mismatch branch 3 times, most recently from 3d19926 to 60ea656 Compare September 17, 2024 09:44
@konstin konstin added the enhancement New feature or improvement to existing functionality label Sep 17, 2024
@lucab lucab force-pushed the ups/tool-install-interp-mismatch branch from 60ea656 to 5721707 Compare September 17, 2024 10:04
@lucab
Copy link
Contributor Author

lucab commented Sep 17, 2024

It looks like I'm hitting some Windows-only different/buggy behavior, or some non-determinism in the CI there, which I'm trying to narrow down.

The symptom is that, on Windows runner only, running the same command twice results in the interpreter binary jumping around.
That is, this simple uv tool install black on the second run will trigger environment invalidation due to a different interpreter:

// Install `black` again
uv_snapshot!(context.filters(), context.tool_install()
.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
`black` is already installed
"###);

Snapshot: tool_install_already_installed-3
[...]
    4     4 │ ----- stderr -----
    5       │-`black` is already installed
          5 │+Ignored existing environment for `black` due to stale Python interpreter
          6 │+Interpreter mismatch: old='[TEMP_DIR]/tools/black/Scripts/python' vs new='[PYTHON-3.12]'

@lucab lucab force-pushed the ups/tool-install-interp-mismatch branch 2 times, most recently from 096d764 to c63df4d Compare September 17, 2024 10:23
@lucab
Copy link
Contributor Author

lucab commented Sep 17, 2024

I've abused the CI to run a few more tests, and found out that this failure on the Windows CI is specific to using a "system" interpreter.
The logic works and test is passing when forcing a managed interpreter via --python-preference only-managed, but fails in all other cases.

My hypothesis here is that there is indeed a difference in the OSes, when creating the venv and setting up the interpreter. On Unix this is done through symlinks (targeting the same binary file in the end) which Windows does not generally have, so there is some file duplication involved (thus failing the is_same_file() check).

@lucab lucab force-pushed the ups/tool-install-interp-mismatch branch 4 times, most recently from e548325 to b26e08f Compare September 17, 2024 15:36
@lucab lucab marked this pull request as ready for review September 17, 2024 15:54
@lucab
Copy link
Contributor Author

lucab commented Sep 17, 2024

For future reference, things we tried in this PR but didn't work due to false positive mismatches detected:

  • comparing interpreter binaries on-disk (via same-file): on Windows, interpreter files are copied around thus always mismatching
  • comparing interpreter paths: also on Windows, minor name differences and versioning (e.g. python vs python3) which result in mismatches

Eventually settled on comparing base_prefix as proxy, as suggested by @zanieb.

@lucab lucab changed the title uv-tool/install: ignore existing environment on interpreter mismatch uv-tool/install: ignore existing environments on interpreter mismatch Sep 17, 2024
let selected_base_prefix = interpreter.sys_base_prefix();
if old_base_prefix == selected_base_prefix {
debug!(
"Found existing interpreter for tool `{}`: {}",
Copy link
Member

@zanieb zanieb Sep 17, 2024

Choose a reason for hiding this comment

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

We'll always find an existing interpreter, maybe this should say something like... "Existing interpreter matches the requested interpreter"? I'd also be okay with lowering this to a trace log.

} else {
let _ = writeln!(
printer.stderr(),
"Ignored existing environment for `{from}` due to stale Python interpreter",
Copy link
Member

Choose a reason for hiding this comment

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

My intuition is that this should be "Ignoring" but I'm not sure if that's clear in our style guide. If you search though, we use "Ignoring" consistently but not "Ignored".

Is the meaning of "stale" clear here? I worry it's not clear to the user if that means something is wrong with the existing interpreter or not. We might want to say something like "Ignoring existing environment for {from}: the requested Python interpreter does not match the environment interpreter". Wdyt?

Comment on lines +2176 to +2255
.arg("--python-preference")
.arg("only-managed")
Copy link
Member

Choose a reason for hiding this comment

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

This isn't super obvious, but I think this can fetch a Python managed version during the test which we currently avoid (downstream packagers do not like it). I think this test may also be brittle depending on the developer's machine — the test context is happy with whatever Python versions it can find whether they are system or managed. I am not sure how to properly write a test for this though :/ it seems like a pain.

Copy link
Member

Choose a reason for hiding this comment

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

For the sake of your time, I'd recommend omitting test coverage and opening an issue to add it. We'll need to consider a comprehensive solution here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can split the new cases apart in a different test and mark it as ignored, if you like. That should still allow running the test manually if needed, but won't impact packagers.
I'll open a ticket about testing with system interpreters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucab lucab force-pushed the ups/tool-install-interp-mismatch branch from b26e08f to 9db055f Compare September 17, 2024 17:18
/// `--python-preference` parameters.
#[ignore = "https://github.com/astral-sh/uv/issues/7473"]
#[test]
fn tool_install_python_preference() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ cargo nextest run --run-ignored all tool_install_python_preference
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.29s
------------
 Nextest run ID 110712ea-a0a0-436a-ab95-4cdfbb384338 with nextest profile: default
    Starting 1 test across 86 binaries (1651 tests skipped)
        PASS [   6.088s] uv::tool_install tool_install_python_preference
------------
     Summary [   6.089s] 1 test run: 1 passed, 1651 skipped

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Nice work!

@lucab lucab merged commit 969b4a2 into astral-sh:main Sep 18, 2024
59 checks passed
@lucab lucab deleted the ups/tool-install-interp-mismatch branch September 18, 2024 06:37
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Sep 21, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.4.9` -> `0.4.13` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.4.13`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0413)

[Compare Source](astral-sh/uv@0.4.12...0.4.13)

##### Enhancements

-   Add `socks` support ([#&#8203;7503](astral-sh/uv#7503))
-   Avoid warning about bad Python interpreter links for empty project environment directories ([#&#8203;7527](astral-sh/uv#7527))
-   Improve invalid environment warning messages ([#&#8203;7544](astral-sh/uv#7544))
-   Use more verbose spelling of "virtualenv" during creation ([#&#8203;7523](astral-sh/uv#7523))
-   Do not use a user-facing warning for "Waiting to acquire lock..." message ([#&#8203;7502](astral-sh/uv#7502))

##### Performance

-   Use a single buffer for hints on resolver errors ([#&#8203;7497](astral-sh/uv#7497))

##### Bug fixes

-   Allow Python pre-releases to be used if they are first on the `PATH` ([#&#8203;7470](astral-sh/uv#7470))
-   Avoid deleting the project environment directory if it is not a virtual environment ([#&#8203;7522](astral-sh/uv#7522))
-   Do not error if the `CACHEDIR.TAG` file exists but cannot be written to ([#&#8203;7550](astral-sh/uv#7550))
-   Treat invalid platform as more compatible than invalid Python ([#&#8203;7556](astral-sh/uv#7556))
-   Use portable paths when serializing sources ([#&#8203;7504](astral-sh/uv#7504))
-   Compute resolver hints using the final reduced derivation tree ([#&#8203;7546](astral-sh/uv#7546))
-   Bump the wheel and sdist cache versions ([#&#8203;7560](astral-sh/uv#7560))
-   Heal cache entries with missing source distributions ([#&#8203;7559](astral-sh/uv#7559))

##### Rust libraries

-   Bump minimum supported Rust version from 1.80 -> 1.81

##### Documentation

-   Add `UV_LINK_MODE` to Docker caching example ([#&#8203;7510](astral-sh/uv#7510))
-   Clarify behavior of of overrides in CLI reference ([#&#8203;7537](astral-sh/uv#7537))

### [`v0.4.12`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0412)

[Compare Source](astral-sh/uv@0.4.11...0.4.12)

##### Enhancements

-   Allow users to provide pre-defined metadata for resolution ([#&#8203;7442](astral-sh/uv#7442))
-   Invalidate existing tool environments on Python interpreter mismatch ([#&#8203;7451](astral-sh/uv#7451))

##### Bug fixes

-   Avoid fatal error when searching for egg-info with missing directory ([#&#8203;7498](astral-sh/uv#7498))

##### Documentation

-   Add note on cache growth for self-hosted GitHub runners ([#&#8203;5757](astral-sh/uv#5757))

### [`v0.4.11`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0411)

[Compare Source](astral-sh/uv@0.4.10...0.4.11)

##### Enhancements

-   Add `--no-editable` support to `uv sync` and `uv export` ([#&#8203;7371](astral-sh/uv#7371))
-   Add support for `--only-dev` to `uv sync` and `uv export` ([#&#8203;7367](astral-sh/uv#7367))
-   Add support for remaining pip-supported file extensions ([#&#8203;7387](astral-sh/uv#7387))
-   Generate shell completion for `uvx` ([#&#8203;7388](astral-sh/uv#7388))
-   Include `uv export` command in `requirements.txt` output ([#&#8203;7374](astral-sh/uv#7374))
-   Prune unzipped source distributions in `uv cache prune --ci` ([#&#8203;7446](astral-sh/uv#7446))
-   Warn when trying to `uv sync` a package without build configuration ([#&#8203;7420](astral-sh/uv#7420))
-   Support requests for pre-releases in the `--python` option ([#&#8203;7335](astral-sh/uv#7335))

##### Bug fixes

-   Avoid erroneous version warning for `.dist-info` directories ([#&#8203;7444](astral-sh/uv#7444))
-   Avoid removing seed packages for `uv venv --seed` environments ([#&#8203;7410](astral-sh/uv#7410))
-   Avoid unnecessary progress bar initializations ([#&#8203;7412](astral-sh/uv#7412))
-   Error when `tool.uv.sources` contains duplicate package names ([#&#8203;7383](astral-sh/uv#7383))
-   Include `--branch` et al when resolving unnamed URLs in `uv add` ([#&#8203;7447](astral-sh/uv#7447))
-   Include `dev-dependencies` in `--no-sources` invocations ([#&#8203;7408](astral-sh/uv#7408))
-   Include the parent interpreter in Python discovery when `--system` is used ([#&#8203;7440](astral-sh/uv#7440))
-   Respect `--no-sources` in PEP 723 scripts ([#&#8203;7409](astral-sh/uv#7409))
-   Respect `pyproject.toml` credentials from user-provided requirements ([#&#8203;7474](astral-sh/uv#7474))
-   Use consistent PyPI cache bucket ([#&#8203;7443](astral-sh/uv#7443))
-   Use unambiguous relative paths in `uv export` ([#&#8203;7378](astral-sh/uv#7378))

##### Documentation

-   Add documentation on platform-specific dependencies ([#&#8203;7411](astral-sh/uv#7411))
-   Add documentation for passing installer options on Linux ([#&#8203;6839](astral-sh/uv#6839))
-   Separate project data from configuration settings ([#&#8203;7053](astral-sh/uv#7053))

##### Error messages

-   Hint at missing `project.name` ([#&#8203;6803](astral-sh/uv#6803))
-   Surface dedicated `project.name` error for workspaces ([#&#8203;7399](astral-sh/uv#7399))
-   Remove duplicate warning for settings discovery errors ([#&#8203;7384](astral-sh/uv#7384))

### [`v0.4.10`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0410)

[Compare Source](astral-sh/uv@0.4.9...0.4.10)

##### Enhancements

-   Allow `uv tool upgrade --all` to continue on individual upgrade failure ([#&#8203;7333](astral-sh/uv#7333))
-   Support globs as cache keys in `tool.uv.cache-keys` ([#&#8203;7268](astral-sh/uv#7268))
-   Add Python package (`__main__.py`) support to `uv run` ([#&#8203;7281](astral-sh/uv#7281))
-   Add zip application support to `uv run` ([#&#8203;7289](astral-sh/uv#7289))
-   Add `--token` option to `self update` command ([#&#8203;7279](astral-sh/uv#7279))

##### Performance

-   Use `globwalk` for `cache-keys` matching ([#&#8203;7337](astral-sh/uv#7337))

##### Bug fixes

-   Always treat archive-like requirements as local files ([#&#8203;7364](astral-sh/uv#7364))
-   Apply `--no-install` options when constructing resolution ([#&#8203;7277](astral-sh/uv#7277))
-   Avoid clobbering existing `py.typed` files contents in `uv init` ([#&#8203;7338](astral-sh/uv#7338))
-   Avoid enforcing platform compatibility when validating lockfile ([#&#8203;7305](astral-sh/uv#7305))
-   Avoid installing transitive dev dependencies ([#&#8203;7318](astral-sh/uv#7318))
-   Avoid selecting prerelease Python installations without opt-in ([#&#8203;7300](astral-sh/uv#7300))
-   Fix PPC64 page size in binary builds. ([#&#8203;7298](astral-sh/uv#7298))
-   Include pre-release Python versions in `uv python list` ([#&#8203;7290](astral-sh/uv#7290))
-   Make version ID optional for source builds ([#&#8203;7362](astral-sh/uv#7362))
-   Support relative paths in `uv add --script` ([#&#8203;7301](astral-sh/uv#7301))

##### Documentation

-   Fix documentation typos for `uv build --build-constraint` flag ([#&#8203;7330](astral-sh/uv#7330))
-   Fix grammatical error in CLI docs ([#&#8203;7353](astral-sh/uv#7353))

##### Error messages

-   Add dedicated lock errors for wheel-only distributions ([#&#8203;7307](astral-sh/uv#7307))
-   Avoid treating `.whl` sources as source distributions ([#&#8203;7303](astral-sh/uv#7303))
-   Clarify Python requirement source for script incompatibilities ([#&#8203;7339](astral-sh/uv#7339))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minor: uv tool install should be able to modify which Python a tool uses.
3 participants