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

Show DEVICE_MEMORY in show-gpus for AWS & Lambda. #1825

Merged
merged 4 commits into from
Apr 3, 2023
Merged

Conversation

concretevitamin
Copy link
Member

@concretevitamin concretevitamin commented Mar 30, 2023

Partially address #1764: only AWS & Lambda catalogs have the relevant info for now.

Azure / GCP's offerings will show a - under the DEVICE_MEM column.

Tested (run the relevant ones):

  • Any manual or new tests for this PR (please specify below)
    • sky show-gpus -a
  • AWS smoke tests: pytest tests/test_smoke.py --aws
  • 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

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

LGTM. Left minor comments.

sky/cli.py Outdated Show resolved Hide resolved
sky/cli.py Outdated Show resolved Hide resolved
sky/clouds/service_catalog/common.py Outdated Show resolved Hide resolved
Comment on lines +3036 to +3037
device_memory_str = (f'{item.device_memory:.0f}GB' if
not pd.isna(item.device_memory) else '-')
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the definition of device memory? IIRC, it is the amount of memory in a single device and does not depend on the device count, right? Then what about TPUs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some definition to -h. Wdyt? For TPU, we can defer adding documentation for it since we don't have that info in the catalog.

Copy link
Member Author

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

Thanks @WoosukKwon, PTAL.

sky/cli.py Outdated Show resolved Hide resolved
sky/cli.py Outdated Show resolved Hide resolved
sky/clouds/service_catalog/common.py Outdated Show resolved Hide resolved
Comment on lines +3036 to +3037
device_memory_str = (f'{item.device_memory:.0f}GB' if
not pd.isna(item.device_memory) else '-')
Copy link
Member Author

Choose a reason for hiding this comment

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

Added some definition to -h. Wdyt? For TPU, we can defer adding documentation for it since we don't have that info in the catalog.

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

Comment on lines +2943 to +2946
* ``DEVICE_MEM``: Memory of a single device; does not depend on the device
count of the instance (VM).

* ``HOST_MEM``: Memory of the host instance (VM).
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Do we need `` here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, this doesn't look good on -h but is what's needed for rst/sphinx docs.

@concretevitamin concretevitamin merged commit 466691d into master Apr 3, 2023
@concretevitamin concretevitamin deleted the dev-mem branch April 3, 2023 20:20
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.

2 participants