-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Omit wheels from lockfile based on --exclude-newer
#12299
base: main
Are you sure you want to change the base?
Conversation
161cd1d
to
36f5ae7
Compare
Is there going to be a user-facing consequence here for people with |
No. |
f637ee8
to
082065c
Compare
There's a panic in the benchmark script setup that looks real. |
04ec269
to
8a825af
Compare
As a reminder (partly for myself), I think the reason we don't filter these out entirely (which would be way simpler) is because we want to be able to show them in error messages (i.e., we want to be able to say "There were wheels, but they were too new"). |
8a825af
to
0964b22
Compare
@@ -145,7 +145,7 @@ mod resolver { | |||
let concurrency = Concurrency::default(); | |||
let config_settings = ConfigSettings::default(); | |||
let exclude_newer = Some( | |||
jiff::civil::date(2024, 8, 8) | |||
jiff::civil::date(2024, 9, 1) |
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 depends on https://pypi.org/project/methodtools/0.4.7/#files, which added a wheel on August 23, 2024. So we can bump this to avoid building it from source.
CodSpeed Performance ReportMerging #12299 will improve performances by 20.51%Comparing Summary
Benchmarks breakdown
|
The benchmark change is pretty funny. I'll review this tomorrow, thanks for the quick turnaround. |
(oops) |
I can reproduce a speedup in the right range by bumping the cutoff date:
|
pub fn is_excluded(&self) -> bool { | ||
matches!(self, Self::Incompatible(IncompatibleWheel::ExcludeNewer(_))) | ||
} |
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.
Is there a reason to not consider all Incompatible()
instances "excluded"?
Summary
We respect
--exclude-newer
during resolution, but we weren't applying it to individual files when writing the lockfile. As a result, if wheels were added to a distribution after its initial release, we'd end up including them in the lockfile, even if they were uploaded after the--exclude-newer
date.Closes #12296.