[CLI] Speed up sky status by caching
#6166
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Michaelvll
reviewed
Jul 10, 2025
sky/client/cli/command.py
Outdated
Comment on lines
1822
to
1838
| with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor: | ||
| ssh_future = executor.submit(update_ssh_configs, cluster_records) | ||
|
|
||
| # Process cluster records while SSH configs are being updated | ||
| hints = [] | ||
| normal_clusters = [] | ||
| controllers = [] | ||
| for cluster_record in cluster_records: | ||
| cluster_name = cluster_record['name'] | ||
| controller = controller_utils.Controllers.from_name(cluster_name) | ||
| if controller is not None: | ||
| controllers.append(cluster_record) | ||
| else: | ||
| normal_clusters.append(cluster_record) | ||
|
|
||
| # Wait for SSH config updates to complete | ||
| ssh_future.result() |
Collaborator
There was a problem hiding this comment.
We need to double check if this can provide any speedup.
Collaborator
Author
There was a problem hiding this comment.
Master
hyperfine --warmup 3 --runs 10 'sky status'
Benchmark 1: sky status
Time (mean ± σ): 2.382 s ± 0.057 s [User: 0.445 s, System: 0.087 s]
Range (min … max): 2.269 s … 2.446 s 10 runs
With health check caching
hyperfine --warmup 3 --runs 10 'sky status'
Benchmark 1: sky status
Time (mean ± σ): 2.043 s ± 0.062 s [User: 0.447 s, System: 0.086 s]
Range (min … max): 1.954 s … 2.186 s 10 runs
With health check caching + grouping request
hyperfine --warmup 3 --runs 10 'sky status'
Benchmark 1: sky status
Time (mean ± σ): 1.711 s ± 0.092 s [User: 0.425 s, System: 0.083 s]
Range (min … max): 1.641 s … 1.951 s 10 runs
It does speedup
cg505
approved these changes
Jul 10, 2025
| return ApiServerInfo(status=ApiServerStatus.UNHEALTHY) | ||
|
|
||
|
|
||
| @cachetools.cached(cache=cachetools.TTLCache(maxsize=10, |
Collaborator
There was a problem hiding this comment.
Do we need to add a lock= kwarg to the cachetools.cached call for thread safety?
| f'View logs at: {constants.API_SERVER_LOGS}') | ||
| try: | ||
| # Clear the cache to ensure fresh checks during startup | ||
| get_api_server_status.cache_clear() # type: ignore |
Collaborator
There was a problem hiding this comment.
typing 😔 but I see there is no way around this
Collaborator
|
should we get this merged? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on top of speedup-api branch
Here are the results
Here's the profile of each:
Before:
After:
If you take a look closely with the number of
check..calls understatus, there's one for every request (blocking). These changes merge it into one call and the main perf improvement is keepingget_api_server_statuscached,Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)