Repository: skypilot-org/skypilot
Pull request: [API server] better error message on API version mismatch
<!-- Describe the changes in this PR -->

close #4878

Tested locally by modifying the version constant. UX screenshots:

## Remote server too old

<img width="1036" alt="image" src="https://github.com/user-attachments/assets/0eeaf7e1-bf84-4507-90ed-841f2be3ecc9" />

## Client too old (remote server)

<img width="1030" alt="image" src="https://github.com/user-attachments/assets/bc1da669-3762-45be-ab34-08b80aefe9fd" />

## Local client-server version mismatch

<img width="1029" alt="image" src="https://github.com/user-attachments/assets/900e7fd0-d3d9-4bb7-931e-b18d76910244" />

The hint message of other cases can be found in unit test cases

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [x] Code formatting: `bash format.sh`
- [x] Manual test see above, also refer to the new unit test case
- [ ] 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: `conda deactivate; bash -i tests/backward_compatibility_tests.sh`


Problem statement:
[API server] Refine error message of version mismatch
One of our users reported the following error when trying to use a remote API server:

```
RuntimeError: SkyPilot API server is too old: v1 (client version is v3). Please restart the SkyPilot API server with: sky api stop; sky api start
```

This is only valid for local API servers and should be for remote API servers, including:

- `sky api stop; sky api start` just does not work for remote API server
- It might be client too old when the remote API server is upgraded by local CLI not;
- If user might have no permission to upgrade the remote API server, we may hint a downgrade

Hints from issue discussion:

Commit message: Address review comments

Signed-off-by: Aylei <rayingecho@gmail.com>

Patch to review:
diff --git a/sky/server/common.py b/sky/server/common.py
index 1997ddf8c00..0e088365181 100644
--- a/sky/server/common.py
+++ b/sky/server/common.py
@@ -46,12 +46,29 @@
 API_SERVER_CLIENT_DIR = pathlib.Path('~/.sky/api_server/clients')
 RETRY_COUNT_ON_TIMEOUT = 3
 
-SKY_API_VERSION_WARNING = (
+SKY_CLIENT_TOO_OLD_WARNING = (
+    f'{colorama.Fore.YELLOW}Your SkyPilot client is too old: '
+    f'v{{client_version}} (server version is v{{server_version}}). '
+    'Please refer to the following link to upgrade your client:\n'
+    f'{colorama.Style.RESET_ALL}'
+    f'{colorama.Style.DIM}'
+    'https://docs.skypilot.co/en/latest/getting-started/installation.html'
+    f'{colorama.Style.RESET_ALL}')
+SKY_SERVER_TOO_OLD_WARNING = (
     f'{colorama.Fore.YELLOW}SkyPilot API server is too old: '
-    f'v{{server_version}} (client version is v{{client_version}}). '
-    'Please restart the SkyPilot API server with: '
-    'sky api stop; sky api start'
+    f'v{{server_version}} (client version is v{{client_version}}). {{hint}}'
+    f'{colorama.Style.RESET_ALL}')
+RESTART_LOCAL_API_SERVER_HINT = ('Please restart the SkyPilot API server with: '
+                                 'sky api stop; sky api start')
+UPGRADE_REMOTE_SERVER_HINT = (
+    'Please refer to the following link to upgrade your server:\n'
+    f'{colorama.Style.RESET_ALL}'
+    f'{colorama.Style.DIM}'
+    'https://docs.skypilot.co/en/latest/reference/api-server/api-server-admin-deploy.html'  # pylint: disable=line-too-long
     f'{colorama.Style.RESET_ALL}')
+# Local API version number.
+_LOCAL_API_VERSION: int = int(server_constants.API_VERSION)
+
 RequestId = str
 ApiVersion = Optional[str]
 
@@ -251,11 +268,30 @@ def check_server_healthy(endpoint: Optional[str] = None,) -> None:
     api_server_info = get_api_server_status(endpoint)
     api_server_status = api_server_info.status
     if api_server_status == ApiServerStatus.VERSION_MISMATCH:
+        sv, cv = api_server_info.api_version, _LOCAL_API_VERSION
+        assert sv is not None, 'API server version is None'
+        try:
+            server_is_older = int(sv) < _LOCAL_API_VERSION
+        except ValueError:
+            # Raised when the server version using an unknown scheme.
+            # Version compatibility checking is expected to handle all legacy
+            # cases so we assume the server is newer when the version scheme
+            # is unknown.
+            logger.debug('API server version using unknown scheme: %s', sv)
+            server_is_older = False
+        if server_is_older:
+            if is_api_server_local():
+                hint = RESTART_LOCAL_API_SERVER_HINT
+            else:
+                hint = UPGRADE_REMOTE_SERVER_HINT
+            msg = SKY_SERVER_TOO_OLD_WARNING.format(client_version=cv,
+                                                    server_version=sv,
+                                                    hint=hint)
+        else:
+            msg = SKY_CLIENT_TOO_OLD_WARNING.format(client_version=cv,
+                                                    server_version=sv)
         with ux_utils.print_exception_no_traceback():
-            raise RuntimeError(
-                SKY_API_VERSION_WARNING.format(
-                    server_version=api_server_info.api_version,
-                    client_version=server_constants.API_VERSION))
+            raise RuntimeError(msg)
     elif api_server_status == ApiServerStatus.UNHEALTHY:
         with ux_utils.print_exception_no_traceback():
             raise exceptions.ApiServerConnectionError(endpoint)
diff --git a/tests/unit_tests/sky/server/test_common.py b/tests/unit_tests/sky/server/test_common.py
new file mode 100644
index 00000000000..b93b4ee78e1
--- /dev/null
+++ b/tests/unit_tests/sky/server/test_common.py
@@ -0,0 +1,89 @@
+"""Unit tests for the SkyPilot API server common module."""
+import sys
+import unittest
+from unittest import mock
+
+import pytest
+
+from sky import exceptions
+from sky.server import common
+from sky.server import constants as server_constants
+from sky.server.common import ApiServerInfo
+from sky.server.common import ApiServerStatus
+
+
+class TestCheckServerHealthy(unittest.TestCase):
+    """Tests for check_server_healthy function."""
+
+    @mock.patch('sky.server.common.get_api_server_status')
+    def test_healthy_server(self, mock_get_status):
+        """Test when server is healthy."""
+        mock_get_status.return_value = ApiServerInfo(
+            status=ApiServerStatus.HEALTHY,
+            api_version=server_constants.API_VERSION)
+
+        # Should not raise any exception
+        common.check_server_healthy()
+
+    @mock.patch('sky.server.common.get_api_server_status')
+    def test_unhealthy_server(self, mock_get_status):
+        """Test when server is unhealthy."""
+        mock_get_status.return_value = ApiServerInfo(
+            status=ApiServerStatus.UNHEALTHY, api_version=None)
+
+        with pytest.raises(exceptions.ApiServerConnectionError):
+            common.check_server_healthy()
+
+    @mock.patch('sky.server.common.get_api_server_status')
+    @mock.patch('sky.server.common.is_api_server_local')
+    def test_local_server_older(self, mock_is_local, mock_get_status):
+        """Test when local server version is older than client."""
+        mock_is_local.return_value = True
+        mock_get_status.return_value = ApiServerInfo(
+            status=ApiServerStatus.VERSION_MISMATCH,
+            api_version='0')  # Always older thant client version
+
+        with pytest.raises(RuntimeError) as exc_info:
+            common.check_server_healthy()
+
+        # Correct error message
+        assert 'SkyPilot API server is too old' in str(exc_info.value)
+        # Should hint user to restart local API server
+        assert 'sky api stop; sky api start' in str(exc_info.value)
+
+    @mock.patch('sky.server.common.get_api_server_status')
+    @mock.patch('sky.server.common.is_api_server_local')
+    def test_remote_server_older(self, mock_is_local, mock_get_status):
+        """Test when remote server version is older than client."""
+        mock_is_local.return_value = False
+        mock_get_status.return_value = ApiServerInfo(
+            status=ApiServerStatus.VERSION_MISMATCH,
+            api_version='0')  # Always older thant client version
+
+        with pytest.raises(RuntimeError) as exc_info:
+            common.check_server_healthy()
+
+        # Correct error message
+        assert 'SkyPilot API server is too old' in str(exc_info.value)
+        # Should hint user to upgrade remote server
+        assert 'Please refer to the following link to upgrade your server' in str(
+            exc_info.value)
+
+    @mock.patch('sky.server.common.get_api_server_status')
+    @mock.patch('sky.server.common.is_api_server_local')
+    def test_client_older(self, mock_is_local, mock_get_status):
+        """Test when client version is older than server."""
+        mock_is_local.return_value = False
+        mock_get_status.return_value = ApiServerInfo(
+            status=ApiServerStatus.VERSION_MISMATCH,
+            api_version=str(
+                sys.maxsize))  # Guaranteed to be newer than client version
+
+        with pytest.raises(RuntimeError) as exc_info:
+            common.check_server_healthy()
+
+        # Correct error message
+        assert 'Your SkyPilot client is too old' in str(exc_info.value)
+        # Should hint user to upgrade client
+        assert 'Please refer to the following link to upgrade your client' in str(
+            exc_info.value)


Code review comments:
diff_hunk: @@ -46,12 +46,29 @@
 API_SERVER_CLIENT_DIR = pathlib.Path('~/.sky/api_server/clients')
 RETRY_COUNT_ON_TIMEOUT = 3
 
-SKY_API_VERSION_WARNING = (
+SKY_CLIENT_TOO_OLD_WARNING = (
+    f'{colorama.Fore.YELLOW}Your SkyPilot client is too old: '
+    f'v{{client_version}} (server version is v{{server_version}}). '
+    'Please refer to the following link to upgrade your client:\n'
+    f'{colorama.Style.RESET_ALL}'
+    f'{colorama.Style.DIM}'
+    'https://docs.skypilot.co/en/latest/getting-started/installation.html'
+    f'{colorama.Style.RESET_ALL}')
+SKY_SERVER_TOO_OLD_WARNING = (
     f'{colorama.Fore.YELLOW}SkyPilot API server is too old: '
-    f'v{{server_version}} (client version is v{{client_version}}). '
-    'Please restart the SkyPilot API server with: '
-    'sky api stop; sky api start'
+    f'v{{server_version}} (client version is v{{client_version}}). {{hint}}'
path: sky/server/common.py
text: @user2:
Although we allow vague versions with the internal version, we should not emphasize the internal version number here. We can make it easier for people to take actions, i.e. showing the version/commit of the SkyPilot package they should upgrade to. For example, we should check the server commit/version:
1. if the server version is something like 1.0.0.dev202xxxx, we should ask user to upgrade their local SkyPilot to `skypilot-nightly==202xxx`
2. if the server version is something like `0.x.x`, we should ask user to upgrade local SkyPilot to `skypilot==0.x.x`
3. if the server version is something like `1.0.0.dev0` and the commit to be `abcde`, we should asks the user to upgrade local version to a specific commit `pip install git+https://github.com...@user1`

@author:
Great idea! Then I think despite which one is older, we should always hint the user to upgrade/downgrade the client to the server version when there is a version mismatch, since the user may not have control over the server deployment. Wdyt?

@user2:
That sounds good to me!

In the server being older case, we can add another sentence, like "contact your admin to upgrade the API server or downgrade your local SkyPilot ... "