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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Programmerino
Copy link

azure.common is deprecated and throws the following error on newer version:

The public API of azure-cli-core has been deprecated starting 2.21.0, and this method can no longer return a valid credential. If you need to still use this method, you need to install 'azure-cli-core<2.21.0'. You may corrupt data if you use current CLI and old azure-cli-core.

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):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

Copy link
Collaborator

@cblmemo cblmemo left a 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 🫡

sky/adaptors/azure.py Outdated Show resolved Hide resolved
sky/skylet/providers/azure/config.py Outdated Show resolved Hide resolved
sky/skylet/providers/azure/config.py Outdated Show resolved Hide resolved
@Programmerino
Copy link
Author

@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.

Copy link
Collaborator

@cblmemo cblmemo left a 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 :)

Comment on lines 24 to 28
def _get_account():
return json.loads(
run('az account show -o json',
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL).stdout.decode())
Copy link
Collaborator

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

@Programmerino Programmerino force-pushed the new-azure-cli-loading branch 2 times, most recently from ac81b26 to 4f5d934 Compare May 2, 2024 20:24
Copy link
Collaborator

@cblmemo cblmemo left a 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 🫡

import threading

from sky.adaptors import common
from sky.utils.subprocess_utils import run
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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


if result.returncode != 0:
error_message = result.stderr.decode()
print(f'Error executing command: {error_message}')
Copy link
Collaborator

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?

Comment on lines 32 to 33
raise RuntimeError(
'Failed to execute az account show -o json command.')
Copy link
Collaborator

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.

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('...')
"""

@Programmerino Programmerino force-pushed the new-azure-cli-loading branch 3 times, most recently from 7bc174e to e69d905 Compare May 24, 2024 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants