Lower noop wheel install overhead. #2315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, installing a wheel that was already installed incurred the cost of hashing the installed wheel chroot every time. The overhead of this wasted work for a warm cache was egregious for large distributions like PyTorch, with gigabytes of files to hash taking seconds. Work towards pex-tool#2312.
|
This drops one of the two ~3s timings in #2312 ("Installing 22 distributions ...") to ~13ms for a warm cache: |
jsirois
commented
Jan 5, 2024
| # We support legacy chroots below by calculating the chroot fingerprint just in time. | ||
| pass | ||
| else: | ||
| cached_fingerprint = installed_wheel.fingerprint |
There was a problem hiding this comment.
@zmanji so this represents the avoidance of ~3s to hash PyTorch 1 of 2 times. It only works for new caches though (rm -rf ~/.pex/installed_wheels). I wanted to keep the status quo here with the chroot symlinking and just get this easy win for now. I do have some notes drawn up revisiting using a separate chroot hash, so this may all go away later, but with the same net effect of skipping the expensive chroot hashing.
The new installed wheel chroot `.layout.json` `fingerprint` field was missing.
jsirois
commented
Jan 5, 2024
| @@ -732,7 +732,7 @@ def iter_map_parallel( | |||
| # least two slots to ensure we process input items in parallel. | |||
| pool_size = max(2, min(len(input_items) // min_average_load, _sanitize_max_jobs(max_jobs))) | |||
|
|
|||
| perform_install = functools.partial(_apply_function, function) | |||
| apply_function = functools.partial(_apply_function, function) | |||
There was a problem hiding this comment.
This was, in the end, an unrelated re-name. I was poking around in the resolve / wheel install machinery looking at timings and narrowing in on the double-hashing going on.
huonw
approved these changes
Jan 5, 2024
Comment on lines
+492
to
+496
| except LoadError: | ||
| # We support legacy chroots below by calculating the chroot fingerprint just in time. | ||
| pass | ||
| else: | ||
| cached_fingerprint = installed_wheel.fingerprint |
There was a problem hiding this comment.
Totally stylistic (no need to change), but is it possible to calculate wheel_dir_hash in these two branches directly, potentially saving some slight fiddle:
Suggested change
| except LoadError: | |
| # We support legacy chroots below by calculating the chroot fingerprint just in time. | |
| pass | |
| else: | |
| cached_fingerprint = installed_wheel.fingerprint | |
| except LoadError: | |
| # We support legacy chroots below by calculating the chroot fingerprint just in time. | |
| wheel_dir_hash = fingerprint_path(self.install_chroot) | |
| else: | |
| wheel_dir_hash = installed_wheel.fingerprint |
There was a problem hiding this comment.
That doesn't work out nice like you think since installed_wheel.fingerprint can be None, necessitating repeating the except clause calculation.
Yes. That's the thrust of the last sentence of my comment to @zmanji here: #2315 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, installing a wheel that was already installed incurred the
cost of hashing the installed wheel chroot every time. The overhead of
this wasted work for a warm cache was egregious for large distributions
like PyTorch, with gigabytes of files to hash taking seconds.
Work towards #2312.