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

distribution-filename: speed up is_compatible #367

Merged
merged 5 commits into from
Nov 9, 2023
Merged

distribution-filename: speed up is_compatible #367

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d06e32a
distribution-filename: add note about dropping build tags
BurntSushi Nov 8, 2023
90cdee2
bench: add micro-benchmarks for wheel-related operations
BurntSushi Nov 8, 2023
00959ae
distribution-filename: add tests for WheelFilename parsing
BurntSushi Nov 8, 2023
0abbafb
distribution-filename: reject wheel filenames with too many components
BurntSushi Nov 8, 2023
dc27d94
distribution-filename: speed up is_compatible
BurntSushi Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
distribution-filename: add note about dropping build tags 
I added this comment because I was trying to figure out where and
how the build tags were used before modifying the code. But it looks
like they aren't. I also checked and, at least the `flyte.in`
benchmark does have some wheel names that contain a build number.
There is perhaps a latent bug here, but I'm not certain.
  • Loading branch information
@BurntSushi
BurntSushi committed Nov 8, 2023
commit d06e32aba4facfc257c271743376169b737b43e4
6 changes: 6 additions & 0 deletions crates/distribution-filename/src/wheel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ impl FromStr for WheelFilename {
// The wheel filename should contain either five or six entries. If six, then the third
// entry is the build tag. If five, then the third entry is the Python tag.
// https://www.python.org/dev/peps/pep-0427/#file-name-convention
//
// 2023-11-08(burntsushi): It looks like the code below actually drops
// the build tag if one is found. According to PEP 0427, the build tag
// is used to break ties. This might mean that we generate identical
// `WheelName` values for multiple distinct wheels, but it's not clear
// if this is a problem in practice.
Comment on lines +35 to +39
Copy link
Member

Choose a reason for hiding this comment

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

I dropped it for the initial version to add it later when a case comes up but so far i didn't hit any case where there the build tag mattered (i haven't actually seen any build tag in the wild yet)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are lots in the flyte.in benchmark. For example, Pillow-9.4.0-2-pp39-pypy39_pp73-macosx_10_10_x86_64 has a build tag of 2.

let mut parts = basename.split('-');

let Some(distribution) = parts.next() else {
Expand Down