-
Notifications
You must be signed in to change notification settings - Fork 547
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
Replace deprecated azure.common usages #3466
Replace deprecated azure.common usages #3466
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.
Thanks for identifying this problem @Programmerino and welcome to SkyPilot! 🚀 This is exciting. Left some discussions 🫡
@cblmemo Thank you for the comments, I think this should fix the issues you mentioned. The only dubious part I see is I'm not sure if it's OK to be importing from the adapter in the skylet code, or if I should move that code somewhere else. |
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 quick fix @Programmerino ! It looks mostly good to me. Left one nit about error handling. For importing adaptors in node provider, I think it should be fine but cc @Michaelvll for a double-check here :)
sky/adaptors/azure.py
Outdated
def _get_account(): | ||
return json.loads( | ||
run('az account show -o json', | ||
stdout=subprocess.PIPE, | ||
stderr=subprocess.DEVNULL).stdout.decode()) |
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 expose the stderr here and add proper error handling? maybe raise an runtime error if json parse error and print out the error message
ac81b26
to
4f5d934
Compare
4f5d934
to
03abe9b
Compare
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! Left several nits 🫡
sky/adaptors/azure.py
Outdated
import threading | ||
|
||
from sky.adaptors import common | ||
from sky.utils.subprocess_utils import run |
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.
from sky.utils.subprocess_utils import run | |
from sky.utils import subprocess_utils |
nit: import package and modules only; https://google.github.io/styleguide/pyguide.html#2144-decision
sky/adaptors/azure.py
Outdated
|
||
if result.returncode != 0: | ||
error_message = result.stderr.decode() | ||
print(f'Error executing command: {error_message}') |
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: could we include those error message into the error raised instead of print it?
sky/adaptors/azure.py
Outdated
raise RuntimeError( | ||
'Failed to execute az account show -o json command.') |
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: we usually use the following context manager to disable traceback for simplicity of the output.
skypilot/sky/utils/ux_utils.py
Lines 26 to 40 in 5a0ecc7
def print_exception_no_traceback(): | |
"""A context manager that prints out an exception without traceback. | |
Mainly for UX: user-facing errors, e.g., ValueError, should suppress long | |
tracebacks. | |
If SKYPILOT_DEBUG environment variable is set, this context manager is a | |
no-op and the full traceback will be shown. | |
Example usage: | |
with print_exception_no_traceback(): | |
if error(): | |
raise ValueError('...') | |
""" |
7bc174e
to
e69d905
Compare
…ling and output handling
e69d905
to
b0adc77
Compare
This issue seems not happening with the latest |
This PR is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
This PR was closed because it has been stalled for 10 days with no activity. |
azure.common
is deprecated and throws the following error on newer version:This PR replaces the relevant code (very few places), using new APIs and CLI parsing instead (unfortunately the recommended route)
I'm not able to run smoke tests on my machine, so if someone wants to make sure the changes to the skylet code are OK, that would be good.
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh