[UX/show-gpus] Improve sorting.

[concretevitamin]

Previous, prices across clouds + within cloud are not sorted properly.

This PR:

        # For each gpu name (count not included):
        #   - Group by cloud
        #   - Sort within each group by prices
        #   - Sort groups by each cloud's (min price, min spot price)

Comparison: two text files in https://gist.github.com/concretevitamin/1fda666bfa129aedd3a3ced485e8f05c

Speed seems the same:

#!/bin/bash
set -ex

export BASH_XTRACEFD=1

sky show-gpus L4
sky show-gpus L4:1
sky show-gpus L4:1 --all-regions
sky show-gpus L4 --cloud aws

sky show-gpus A100
sky show-gpus A100:8
sky show-gpus A100:8 --all-regions
sky show-gpus A100:8 --cloud aws

master: median 15.454 total

15.454 total
14.471 total
16.712 total

PR: median 15.391 total
15.391 total
15.497 total
14.612 total

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

[Michaelvll]

Nice! It is great that we can have additional sorting ability for our sky show-gpus. A long existing issue I was wondering was that if we can group by the (GPU, gpu_count, cloud) and sort by price, instead of group by cloud.
Most of the time I looked at the table, I tried to directly compare those machines with the same config across clouds, and our current way mixes different count makes it quite hard to compare. Wdyt?

[concretevitamin]

IIRC the reason we did it this way was to optimize for single-cloud users. For them, it'd be messy to group by (GPU, gpu_count, cloud) as they need to skim across many irrelevant clouds.

[Michaelvll]

For a single cloud user, they can always specify --cloud in show-gpus to get the results for that cloud only, so it might not be a blocker for the change.

I think we didn't do the (GPU, gpu_count, cloud) because the information retrieved from the catalogs is for each cloud, and we did not have the regrouping and resorting code path in CLI before. Since we have the "frontend" sorting enabled, we could probably switch to the tuple.
IMO, in most cases, comparing different configs of GPUs may not be as useful as comparing the same config but with different pricing.

That said, I am fine with having the current version merged first and have another issue for the proposed change for the different key for grouping.

[concretevitamin]

I think we want to optimize for single-cloud only users. For them it's cumbersome to type an additional --cloud argument because it should be inferred as unique. Given these, I believe the current treatment is good.

[Michaelvll]

OK, I think I can now think of a benefit for grouping by cloud:
It will be easier to see all the available number of GPUs offered by a specific cloud.

While for the proposed key for grouping, it will be easier to find which cloud offers the specific number of GPU requested.

I am still half-half for this though, since most of the time a user knows how many GPUs they want for their workloads, e.g., 7B model requires a single L4, while 70B requires at least 4 40GB GPUs.
It should be fine to merge this PR first, as it is definitely better than the current unsorted version in master branch.

[concretevitamin]

It will be easier to see all the available number of GPUs offered by a specific cloud.

Yep. Also, if one's single-cloud only, easier to show that cloud's offerings in a consecutive group.

While for the proposed key for grouping, it will be easier to find which cloud offers the specific number of GPU requested.

For that, if users pass the count in the argument, both ways of grouping are the same (for most cases, except some where the same cloud has different X:1 offerings).

[Michaelvll]

Thanks for adding the sort for the GPU pricing @concretevitamin! LGTM. Please run the format.sh before merging.

Also, it would be nice to add some unit tests for the behavior of show-gpus.

Another issue, it seems we have A100 and A100-SXM separated for RunPod while combined for lambda labs. We probably need to separate them for all the clouds.

The above issues can be fixed in another PR.

[Michaelvll]

Just a kind bump for this @concretevitamin. Are we going to merge it?

[concretevitamin]

Oops, slipped through the cracks. Just opened two issues for the above items. Merging this now.