-
Notifications
You must be signed in to change notification settings - Fork 510
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
[AWS] Failover Optimization from Quota #1953
[AWS] Failover Optimization from Quota #1953
Conversation
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.
Awesome work @shethhriday29, and welcome to SkyPilot! Took a quick glance and left some high-level comments. Will take a closer look later. Also recommend running ./format.sh
to automatically format your code to keep the style checker happy.
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.
Awesome work @shethhriday29! Left some comments.
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
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 looks ready to go! Thanks for the awesome work @shethhriday29 - this will make failover much faster for AWS! 🚀
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 adding this feature @shethhriday29! It is exciting to see the optimization. Left several nits about code style and robustness. The PR should be ready to go, after the comments are fixed and the tests are passed.
sky/clouds/aws.py
Outdated
region: str, | ||
instance_type: str, | ||
use_spot: bool = False) -> bool: | ||
""" |
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.
style nit: Having a single line description of this function in the first line.
""" | |
"""Check if the quota is available for the requested instance_type | |
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.
Google styleguide: First line should be on the same line as the triple quotes. https://google.github.io/styleguide/pyguide.html#383-functions-and-methods
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
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 @shethhriday29! Left a final comment about the importing. Otherwise, it looks good to me.
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
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 update @shethhriday29! LGTM.
This is awesome @shethhriday29. A few UX notes:
However, removing the --use-spot flag I could successfully provision. A few ideas to improve this logging:
|
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.
Awesome @shethhriday29! LGTM.
sky/clouds/aws.py
Outdated
region: str, | ||
instance_type: str, | ||
use_spot: bool = False) -> bool: | ||
""" |
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.
Google styleguide: First line should be on the same line as the triple quotes. https://google.github.io/styleguide/pyguide.html#383-functions-and-methods
sky/clouds/cloud.py
Outdated
instance_type: str, | ||
use_spot: bool = False) -> bool: | ||
""" | ||
Checks to ensure that a particular accelerator has a nonzero quota |
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.
ditto (styleguide)
Thanks @shethhriday29! Looks like this is ready to go in. Merging now! |
Description:
Tested (run the relevant ones):
test_aws_zero_quota_failover
, that ensures that the zero quota checker works