-
Notifications
You must be signed in to change notification settings - Fork 531
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
[Core] Fix wheel creation when multiple versions are installed #3866
Conversation
sky/backends/wheel_utils.py
Outdated
if (last_wheel_modification_time < last_modification_time) or not any( | ||
WHEEL_DIR.glob(_WHEEL_PATTERN)): |
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.
Should we instead check WHEEL_DIR.glob(f'**/{_WHEEL_PATTERN}')
instead, similar to https://github.com/skypilot-org/skypilot/blob/master/sky/backends/wheel_utils.py#L43?
Also, since we are checking the whether the current wheel exists, it might be fine to just use the current wheel for the later latest_wheel
, i.e. it can become _get_current_wheel_and_remove_older_others
?
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.
Ah good idea. Refactored _get_latest_wheel_and_remove_all_others
into separate methods to get the latest wheel and remove others.
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.
Thanks for the fix @romilbhardwaj ! LGTM
# Only build wheels if the wheel is outdated or wheel does not exist | ||
# for the requested version. | ||
if (last_wheel_modification_time < last_modification_time) or not any( | ||
WHEEL_DIR.glob(f'**/{_WHEEL_PATTERN}')): |
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.
Nit: do we need any
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.
Yes, glob()
returns a generator that we must check with any(). Checking just glob(f'**/{_WHEEL_PATTERN}')
would always return true since a generator is always returned.
If a user has multiple Sky versions installed (e.g., in different conda envs),
sky launch
would fail with:This is because we cleanup all except the latest wheels:
skypilot/sky/backends/wheel_utils.py
Lines 51 to 57 in bca709b
This PR updates our wheel creation behavior to check if a wheel for the version exists and create it if it doesn't already exist.
Tested (run the relevant ones):
bash format.sh
Manual tests: