-
Notifications
You must be signed in to change notification settings - Fork 662
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
Resolve non-determistic behavior in preferences due to site-packages ordering #2780
Conversation
crates/uv/tests/pip_install.rs
Outdated
|
||
// Request the second anyio version again | ||
// Should remove both previous versions and reinstall the second one | ||
uv_snapshot!(context1.filters(), context1.install().arg("anyio==4.0.0"), @r###" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do not request 4.0.0, 3.7.0 is reinstalled without panic on my machine 🤯
Cherry-picked over to #2779 — we can discuss the weirdness of this test here if necessary but I'll merge the clear fix anyway. |
# Conflicts: # crates/uv/tests/pip_install.rs
let mut entries = site_packages.collect::<Result<Vec<_>, std::io::Error>>()?; | ||
// TODO(zanieb): Consider filtering to just directories to reduce the size of the sort | ||
// Sort for determinism, `read_dir` is different per-platform | ||
entries.sort_by_key(fs_err::DirEntry::path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure out how to write this as a single iterator operation because it's weird working with nested results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean, not collecting the entries
here at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must collect them to sort them afaik. I just couldn't filter to directories here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. You would need to filter prior to the collect
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I tried that haha it's awkward because of the nested Result
types, it's beyond my understanding of iterators in Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @snowsignal maybe you have a suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zanieb Here's one way you could do it:
let mut entries: Vec<_> = site_packages
.filter(|read_dir| match read_dir {
Ok(entry) => entry
.file_type()
.map(|file_type| file_type.is_dir())
.unwrap_or_default(),
Err(_) => true,
})
.collect::<std::io::Result<_>>()?;
Also, if you tweak the code to collect into a BTreeSet<PathBuf>
, the sorting will happen as part of collect()
and you don't have to call sort_by_key
afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's how you could do that:
let paths: BTreeSet<_> = site_packages
.filter_map(|read_dir| match read_dir {
Ok(entry) => {
entry.file_type().ok()?.is_dir().then_some(Ok(entry.path()))
}
Err(err) => Some(Err(err)),
})
.collect::<Result<_>>()?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool thank you!
@@ -60,7 +66,6 @@ impl<'a> SitePackages<'a> { | |||
|
|||
// Index all installed packages by name. | |||
for entry in site_packages { | |||
let entry = entry?; | |||
if entry.file_type()?.is_dir() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the call we'd move into the iterator chain above
// Request the anyio without a version specifier | ||
// This is loosely a regression test for the ordering of the installation preferences | ||
// from existing site-packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before sorting the site-packages, this preferred anyio==3.7.0
on macOS and anyio==4.0.0
on Ubuntu
Originally a regression test for #2779 but we found out that there's some weird behavior where different
anyio
versions were preferred based on the platform.