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.

Sign up for GitHub

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

Jump to bottom

Deduplicate when install or uninstall python #4841

Merged
merged 3 commits into from
Jul 6, 2024
Merged

Deduplicate when install or uninstall python #4841

merged 3 commits into from
Jul 6, 2024

Conversation

j178
Copy link
Contributor

@j178 j178 commented Jul 5, 2024 •
edited
Loading

When specifying the same argument multiple times, the same version will be downloaded multiple times:

$ cargo run -- python install --preview --force 3.12.3 cpython-3.12 3.12.3 3.12
Looking for installation Python 3.12.3 (any-3.12.3-any-any-any)
Looking for installation cpython-3.12-any-any-any (cpython-3.12-any-any-any)
Looking for installation Python 3.12.3 (any-3.12.3-any-any-any)
Looking for installation Python 3.12 (any-3.12-any-any-any)
Found 4/4 versions requiring installation
Downloading cpython-3.12.3-windows-x86_64-none
Downloading cpython-3.12.3-windows-x86_64-none
Downloading cpython-3.12.3-windows-x86_64-none
Downloading cpython-3.12.3-windows-x86_64-none

This PR deduplicates the ManagedPythonDownload before install or uninstall:

$ cargo run -q -- python install --preview --force 3.12.3 cpython-3.12 3.12.3 3.12
Looking for installation Python 3.12 (any-3.12-any-any-any)
Looking for installation Python 3.12.3 (any-3.12.3-any-any-any)
Looking for installation cpython-3.12-any-any-any (cpython-3.12-any-any-any)
Downloading cpython-3.12.3-windows-x86_64-none
Installed Python 3.12.3 to C:\Users\nigel\AppData\Roaming\uv\data\python\cpython-3.12.3-windows-x86_64-none
Installed 1 version in 6s

$ cargo run -q -- python uninstall --preview  3.12.3 cpython-3.12 3.12.3 3.12
Looking for Python installations matching Python 3.12 (any-3.12-any-any-any)
Found installation `cpython-3.12.3-windows-x86_64-none` that matches Python 3.12
Looking for Python installations matching Python 3.12.3 (any-3.12.3-any-any-any)
Looking for Python installations matching cpython-3.12-any-any-any (cpython-3.12-any-any-any)
Uninstalled `cpython-3.12.3-windows-x86_64-none`
Removed 1 Python installation

@j178 j178 marked this pull request as ready for review July 5, 2024 22:24
charliermarsh
charliermarsh reviewed Jul 5, 2024
View reviewed changes
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.

IIRC dedup only deduplicates adjacent elements. Should we collect into a BTreeSet?

@zanieb
Copy link
Member

zanieb commented Jul 5, 2024

We probably need to de-duplicate the keys we're going to download too

@j178 j178 marked this pull request as draft July 5, 2024 23:10
@j178 j178 changed the title Dedup arguments when install or uninstall python Dedupulicates when install or uninstall python Jul 6, 2024
@j178 j178 changed the title Dedupulicates when install or uninstall python Deduplicates when install or uninstall python Jul 6, 2024
@j178 j178 marked this pull request as ready for review July 6, 2024 00:18
@j178 j178 changed the title Deduplicates when install or uninstall python Deduplicate when install or uninstall python Jul 6, 2024
@zanieb zanieb added bug Something isn't working preview Experimental behavior labels Jul 6, 2024
zanieb
zanieb approved these changes Jul 6, 2024
View reviewed changes
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.

Thank you!

@zanieb zanieb merged commit bcb2568 into astral-sh:main Jul 6, 2024
47 checks passed
charliermarsh
charliermarsh reviewed Jul 6, 2024
View reviewed changes
crates/uv/src/commands/python/uninstall.rs
@@ -83,7 +84,7 @@ pub(crate) async fn uninstall(
return Ok(ExitStatus::Failure);
}

let tasks = futures::stream::iter(matching_installations.iter())
let tasks = futures::stream::iter(matching_installations.iter().unique())
Copy link
Member

Choose a reason for hiding this comment

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

Can we collect into a BTreeSet beforehand or no?

Copy link
Member

Choose a reason for hiding this comment

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

Hm matching_installations is BTreeSet already for deterministic ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I did not realize that, I've raised #4842 to revert this.

@charliermarsh
Copy link
Member

Thanks, your contributions are great.

j178 added a commit to j178/uv that referenced this pull request Jul 6, 2024
@j178
Revert uninstall deduplication changes from astral-sh#4841
02c8937
@j178 j178 mentioned this pull request Jul 6, 2024
Merged
@j178 j178 deleted the dedup branch July 6, 2024 05:25
charliermarsh pushed a commit that referenced this pull request Jul 6, 2024
@j178
Revert uninstall deduplication changes from #4841 (#4842)
1f454f3 
`matching_installations` is BTreeSet already, no need to deduplicate it.

#4841 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charliermarsh charliermarsh charliermarsh left review comments

@zanieb zanieb zanieb approved these changes

Assignees
No one assigned
Labels
bug Something isn't working preview Experimental behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@j178 @zanieb @charliermarsh