[AWS] Fix check/status failure when no permission is granted for the account #2415
Conversation
sky/resources.py
Outdated
| @@ -172,7 +177,11 @@ def __init__( | |||
| self._try_validate_disk_tier() | |||
| self._try_validate_ports() | |||
|
|
|||
| @service_catalog.use_default_catalog_if_failed | |||
| # When querying the accelerators for the instance type, we will check the | |||
There was a problem hiding this comment.
Sorry, not following. Which callsite inside repr causes this? I only see some {accelerators} formatting.
There was a problem hiding this comment.
self.accelerators is a @property which will call into the self.cloud.get_accelerators_from_instance_type and invoke service_catalog.get_accelerators_from_instance_type.
There was a problem hiding this comment.
Got it, tried to incorporate this in.
sky/resources.py
Outdated
| @@ -305,6 +314,7 @@ def memory(self) -> Optional[str]: | |||
| return self._memory | |||
|
|
|||
| @property | |||
| @functools.lru_cache() | |||
There was a problem hiding this comment.
This is to reduce the frequency we call into the service_catalog underneath to reduce the overhead, i.e., searching the df for the accelerators. Since the resources object is immutable, caching this function should be safe.
There was a problem hiding this comment.
- Is the overhead noticeable? Worth documenting if so.
- Shall we add
max_size=1?
There was a problem hiding this comment.
The overhead is not significant, but it seems no harm to cache the output. The following commands take 3.3 seconds in master branch, but 0.008 seconds in the current PR.
python -u <<EOF
import sky
import time
r = sky.Resources(instance_type='p3.2xlarge')
total_time = 0
for i in range(1000):
start = time.time()
str(r)
total_time += time.time() - start
print(total_time)
EOF
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
sky/resources.py
Outdated
| @@ -172,7 +177,11 @@ def __init__( | |||
| self._try_validate_disk_tier() | |||
| self._try_validate_ports() | |||
|
|
|||
| @service_catalog.use_default_catalog_if_failed | |||
| # When querying the accelerators for the instance type, we will check the | |||
There was a problem hiding this comment.
Got it, tried to incorporate this in.
sky/resources.py
Outdated
| @@ -305,6 +314,7 @@ def memory(self) -> Optional[str]: | |||
| return self._memory | |||
|
|
|||
| @property | |||
| @functools.lru_cache() | |||
There was a problem hiding this comment.
- Is the overhead noticeable? Worth documenting if so.
- Shall we add
max_size=1?
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Fixes #2414
This PR fixes the following:
sky show-gpus/sky status) should not require the actual az mapping.sky checkshould disable AWS if the permission is not enough for fetching the availability zone mappingsFuture TODO: #2416
Tested (run the relevant ones):
bash format.shsky statussky checkpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_namebash tests/backward_comaptibility_tests.sh