uv python install and uninstall now account for the UV_PYTHON env var by Choudhry18 · Pull Request #10222 · astral-sh/uv

Choudhry18 · 2024-12-29T23:55:11Z

Summary

Checks UV_PYTHON for version when uv python install or uv python uninstall ran

Test Plan

I added new snapshot tests:
uv/tests/it/python_install::python_install_with_uv_python_env that sets UV_PYTHON to and tests if that version is installed and not the latest. UV_PYTHON=3.12 uv python install would install 3.12 and not the latest version

uv/tests/it/python_install::python_install_with_uv_python_env_and_arg that sets the env variable and the target and tests if the target takes precedence. UV_PYTHON=3.12 uv python install 3.8 would install python 3.8

Requirement Concern

The order of precedence on the python install command is : target specified through cli -> environment var UV_PYTHON -> .python-versions if none other specified.

The order of precedence on the python uninstall command is: target specified through cli (required unless UV_PYTHON set) -> environment var UV_PYTHON (required unless target provided in cli). As the --all arg conflicts with target it also conflicts UV_PYTHON env variable

closes #10128

…iable and added a test for that

crates/uv/src/settings.rs

crates/uv/tests/it/python_install.rs

zanieb · 2025-01-23T23:25:07Z

Hey @Choudhry18 I fixed some merge conflicts and expanded the test cases.

I have one comment; otherwise this looks good to me!

zanieb · 2025-01-23T23:25:31Z

crates/uv/tests/it/python_install.rs

+    // We should ignore `UV_PYTHON` here
+    uv_snapshot!(context.filters(), context.python_uninstall().arg("--all").env(EnvVars::UV_PYTHON, "3.11"), @r###"
+    success: false
+    exit_code: 2
+    ----- stdout -----
+
+    ----- stderr -----
+    error: the argument '--all' cannot be used with '<TARGETS>...'
+
+    Usage: uv python uninstall --all --install-dir <INSTALL_DIR> <TARGETS>...
+
+    For more information, try '--help'.
+    "###);


This test failure is problematic — this needs to work still.

I see you mentioned this at

As the --all arg conflicts with target it also conflicts UV_PYTHON env variable

We might need to implement this differently to work around this. Perhaps --all should just override the targets?

I do agree that -all should override targets as I don't see a case where a user would add -all argument without intending to delete all the version. So all should override targets set through argument or environment. Do we need to give the user a warning (I personally don't think it's needed) like:

Setting targets with -all is redundant and just result in all managed python versions being uninstalled

Yeah I don't think a warning is needed, it's a weird case but it seems fine to just ignore the target silently unless someone reports a confusing interaction.

Ah are you suggesting that we should remove the conflict entirely and let someone do uv python uninstall foo --all? I think that could be too confusing :/

yes that is what I had in mind that if --all is set target shouldn't matter . I do agree invalid targets with --all not giving an error can be a bit confusing. Currently uv python uninstall foo -all doesn't really tell the user that their target is invalid it just tells them of the conflict of target with --all

So what I understand is you suggest that --all should override target but if it target is invalid it should throw an error

Oh sorry I mean uv python uninstall 3.11 --all would be confusing too — just used foo as a silly example.

oh so you suggest the target set as an argument should behave differently in case of use with --all (throw an error citing a conflict) vs target set through the env variable should be ignored silently. Apologies that took me a minute to understand :/

That makes sense for usage here the only concern I have is are there any other env variable that behave like this in uv. Most of the env variable I have seen they are sort of a replacement for just having to type to argument again and again and behave exactly the same as the cli argument. So this behaving differently might be not be very idiomatic. I also might just be overthinking this one.

oh so you suggest the target set as an argument should behave differently in case of use with --all (throw an error citing a conflict) vs target set through the env variable should be ignored silently. Apologies that took me a minute to understand :/

Yeah, that's the suggestion. No problem on the confusion.

Most of the env variable I have seen they are sort of a replacement for just having to type to argument again and again and behave exactly the same as the cli argument. So this behaving differently might be not be very idiomatic. I also might just be overthinking this one.

I think it's okay in this case. Though I'm not sure of the implications implementation-wise.

I couldn't find a suitable way to implement it using clap's derive API. My workaround was writing a custom validation check that is ran after the parsing and before the uninstall Settings are resolved. The implementation does get a bit ugly in order to replicate the format of the error message without using the derive API

… through custom validation that is called before resolving settings

zanieb · 2025-02-13T18:36:17Z

I found the early-validation a bit awkward (as you noted) and moved it into the implementation.

I'm finding myself a little hesitant about changing the uninstall behavior at all now? I feel like this change would definitely be simpler if we just handled UV_PYTHON on install and left uninstall explicit?

zanieb · 2025-02-13T18:51:53Z

Exploring that in #11487

Unlike #10222, this does not respect `UV_PYTHON` in `uv python uninstall` (continuing to require an explicit target there) which I think is simpler and matches our `.python-version` file behavior. --------- Co-authored-by: Choudhry Abdullah <[email protected]> Co-authored-by: Choudhry Abdullah <[email protected]> Co-authored-by: Aria Desires <[email protected]>

Unlike astral-sh#10222, this does not respect `UV_PYTHON` in `uv python uninstall` (continuing to require an explicit target there) which I think is simpler and matches our `.python-version` file behavior. --------- Co-authored-by: Choudhry Abdullah <[email protected]> Co-authored-by: Choudhry Abdullah <[email protected]> Co-authored-by: Aria Desires <[email protected]>

Choudhry18 added 2 commits December 30, 2024 04:29

uv python install and uninstall now account for the UV_PYTHON env var…

8b34200

…iable and added a test for that

fixed lint and clippy

3e30d22

zanieb self-assigned this Dec 30, 2024

samypr100 reviewed Jan 1, 2025

View reviewed changes

crates/uv/src/settings.rs Outdated Show resolved Hide resolved

samypr100 reviewed Jan 1, 2025

View reviewed changes

crates/uv/tests/it/python_install.rs Outdated Show resolved Hide resolved

Choudhry18 added 5 commits January 2, 2025 20:02

using clap to use UV_PYTHON env variable

4d0ff98

added env UV_PYTHON to the help message

ae0e134

another tweak to the help test

85957d5

changed pillow version back to 11.0.0

d818ded

reverting the pillow version

3dc6e0c

Choudhry18 requested a review from samypr100 January 3, 2025 10:04

zanieb added the breaking A breaking change label Jan 23, 2025

zanieb added this to the v0.6.0 milestone Jan 23, 2025

Merge branch 'main' into env_var_python_install

5ef1186

zanieb reviewed Jan 23, 2025

View reviewed changes

Choudhry Abdullah and others added 5 commits February 6, 2025 00:05

target set through UV_PYTHON doesn't conflict with --all. Implemented…

63aeec9

… through custom validation that is called before resolving settings

updated snapshot for python_install_default_from_env

f7feded

Merge branch 'astral-sh:main' into env_var_python_install

e8ac642

updated python version in snapshot test

428a191

updated python version to 3.12

bf276c3

zanieb self-requested a review February 8, 2025 02:38

Validate in the uninstall implementation instead of the CLI layer

c4536cc

zanieb changed the base branch from main to tracking/060 February 13, 2025 18:30

zanieb mentioned this pull request Feb 13, 2025

Respect UV_PYTHON in uv python install #11487

Merged

zanieb force-pushed the tracking/060 branch from 1e58434 to 438f10b Compare February 13, 2025 18:47

zanieb force-pushed the tracking/060 branch from 438f10b to f00317b Compare February 13, 2025 19:08

zanieb force-pushed the tracking/060 branch from 697ada2 to 80959bc Compare February 13, 2025 22:04

zanieb deleted the branch astral-sh:tracking/060 February 13, 2025 22:17

zanieb closed this Feb 13, 2025

Comments

Conversation

Choudhry18 commented Dec 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Requirement Concern

Uh oh!

Uh oh!

Uh oh!

zanieb commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanieb commented Feb 13, 2025

Uh oh!

zanieb commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Choudhry18 commented Dec 29, 2024 •

edited

Loading

zanieb commented Jan 23, 2025 •

edited

Loading