Skip to content

Commit b6b2bad

Browse files
committed
address comments
1 parent 6236ce8 commit b6b2bad

File tree

11 files changed

+52
-30
lines changed

11 files changed

+52
-30
lines changed

docs/source/reference/async.rst

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,15 +174,15 @@ To view all requests on the server, run ``sky api status``.
174174
.. code-block:: console
175175
176176
$ # List ongoing requests, default to show 50 requests
177-
$ # add `--limit 0` to show all ongoing requests
177+
$ # add `-l none` or `-l all` to show all ongoing requests
178178
$ sky api status
179179
ID User Name Created Status
180180
0d35ffa7-2813-4f3b-95c2-c5ab2238df50 user2 logs a few secs ago RUNNING
181181
a9d59602-b82b-4cf8-a10f-5cde4dd76f29 user1 launch a few secs ago RUNNING
182182
skypilot-status-refresh-daemon skypilot-system status 5 hrs ago RUNNING
183183
184184
$ # List finished and ongoing requests, default to show 50 requests
185-
$ # add `--limit 0` to show all finished and ongoing requests
185+
$ # add `-l none` or `-l all` to show all finished and ongoing requests
186186
$ sky api status -a
187187
188188
.. hint::

sky/client/cli/command.py

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6162,6 +6162,25 @@ def api_cancel(request_ids: Optional[List[str]], all: bool, all_users: bool):
61626162
fg='green')
61636163

61646164

6165+
class IntOrNone(click.ParamType):
6166+
"""Int or None"""
6167+
name = 'int-or-none'
6168+
6169+
def convert(self, value, param, ctx):
6170+
if isinstance(value, int):
6171+
return value
6172+
if isinstance(value, str) and value.lower() in ('none', 'all'):
6173+
return None
6174+
try:
6175+
return int(value)
6176+
except ValueError:
6177+
self.fail(f'{value!r} is not a valid integer or "none" or "all"',
6178+
param, ctx)
6179+
6180+
6181+
INT_OR_NONE = IntOrNone()
6182+
6183+
61656184
@api.command('status', cls=_DocumentedCodeCommand)
61666185
@flags.config_option(expose_value=False)
61676186
@click.argument('request_ids',
@@ -6179,15 +6198,15 @@ def api_cancel(request_ids: Optional[List[str]], all: bool, all_users: bool):
61796198
'--limit',
61806199
'-l',
61816200
default=_NUM_REQUESTS_TO_SHOW,
6182-
type=int,
6201+
type=INT_OR_NONE,
61836202
required=False,
61846203
help=(f'Number of requests to show, default is {_NUM_REQUESTS_TO_SHOW},'
6185-
f' set to 0 to show all requests.'))
6204+
f' set to "none" or "all" to show all requests.'))
61866205
@flags.verbose_option('Show more details.')
61876206
@usage_lib.entrypoint
61886207
# pylint: disable=redefined-builtin
61896208
def api_status(request_ids: Optional[List[str]], all_status: bool,
6190-
verbose: bool, limit: int):
6209+
verbose: bool, limit: Optional[int]):
61916210
"""List requests on SkyPilot API server."""
61926211
if not request_ids:
61936212
request_ids = None
@@ -6220,8 +6239,12 @@ def api_status(request_ids: Optional[List[str]], all_status: bool,
62206239
if verbose:
62216240
dummy_row.append('-')
62226241
table.add_row(dummy_row)
6223-
click.echo()
62246242
click.echo(table)
6243+
if limit and len(request_list) >= limit:
6244+
click.echo()
6245+
click.echo(
6246+
f'Showing {limit} requests. Use "-l none" or "-l all" to show'
6247+
f' all requests.')
62256248

62266249

62276250
@api.command('login', cls=_DocumentedCodeCommand)

sky/client/sdk.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2159,7 +2159,7 @@ def api_status(
21592159
request_ids: Optional[List[Union[server_common.RequestId[T], str]]] = None,
21602160
# pylint: disable=redefined-builtin
21612161
all_status: bool = False,
2162-
request_limit: int = 0,
2162+
limit: Optional[int] = None,
21632163
fields: Optional[List[str]] = None,
21642164
) -> List[payloads.RequestPayload]:
21652165
"""Lists all requests.
@@ -2169,7 +2169,7 @@ def api_status(
21692169
If None, all requests are queried.
21702170
all_status: Whether to list all finished requests as well. This argument
21712171
is ignored if request_ids is not None.
2172-
request_limit: The number of requests to show. If 0, show all requests.
2172+
limit: The number of requests to show. If None, show all requests.
21732173
fields: The fields to get. If None, get all fields.
21742174
21752175
Returns:
@@ -2182,7 +2182,7 @@ def api_status(
21822182
body = payloads.RequestStatusBody(
21832183
request_ids=request_ids,
21842184
all_status=all_status,
2185-
request_limit=request_limit,
2185+
limit=limit,
21862186
fields=fields,
21872187
)
21882188
response = server_common.make_authenticated_request(

sky/server/constants.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,7 @@
6464

6565
# Cookie header for stream request id.
6666
STREAM_REQUEST_HEADER = 'X-SkyPilot-Stream-Request-ID'
67+
68+
# Valid empty values for pickled fields (base64-encoded pickled None)
69+
# base64.b64encode(pickle.dumps(None)).decode('utf-8')
70+
EMPTY_PICKLED_VALUE = 'gAROLg=='

sky/server/requests/payloads.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ class RequestStatusBody(pydantic.BaseModel):
573573
"""The request body for the API request status endpoint."""
574574
request_ids: Optional[List[str]] = None
575575
all_status: bool = False
576-
request_limit: int = 0
576+
limit: Optional[int] = None
577577
fields: Optional[List[str]] = None
578578

579579

sky/server/requests/requests.py

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -349,19 +349,15 @@ def _update_request_row_fields(
349349
# Convert tuple to dictionary for easier manipulation
350350
content = dict(zip(fields, row))
351351

352-
# Valid empty values for pickled fields (base64-encoded pickled None)
353-
# base64.b64encode(pickle.dumps(None)).decode('utf-8')
354-
empty_pickled_value = 'gAROLg=='
355-
356352
# Required fields in RequestPayload
357353
if 'request_id' not in fields:
358354
content['request_id'] = ''
359355
if 'name' not in fields:
360356
content['name'] = ''
361357
if 'entrypoint' not in fields:
362-
content['entrypoint'] = empty_pickled_value
358+
content['entrypoint'] = server_constants.EMPTY_PICKLED_VALUE
363359
if 'request_body' not in fields:
364-
content['request_body'] = empty_pickled_value
360+
content['request_body'] = server_constants.EMPTY_PICKLED_VALUE
365361
if 'status' not in fields:
366362
content['status'] = RequestStatus.PENDING.value
367363
if 'created_at' not in fields:
@@ -732,7 +728,7 @@ class RequestTaskFilter:
732728
Mutually exclusive with exclude_request_names.
733729
finished_before: if provided, only include requests finished before this
734730
timestamp.
735-
request_limit: the number of requests to show. If 0, show all requests.
731+
limit: the number of requests to show. If None, show all requests.
736732
737733
Raises:
738734
ValueError: If both exclude_request_names and include_request_names are
@@ -744,7 +740,7 @@ class RequestTaskFilter:
744740
exclude_request_names: Optional[List[str]] = None
745741
include_request_names: Optional[List[str]] = None
746742
finished_before: Optional[float] = None
747-
request_limit: int = 0
743+
limit: Optional[int] = None
748744
fields: Optional[List[str]] = None
749745

750746
def __post_init__(self):
@@ -792,8 +788,8 @@ def build_query(self) -> Tuple[str, List[Any]]:
792788
columns_str = ', '.join(self.fields)
793789
query_str = (f'SELECT {columns_str} FROM {REQUEST_TABLE}{filter_str} '
794790
'ORDER BY created_at DESC')
795-
if self.request_limit > 0:
796-
query_str += f' LIMIT {self.request_limit}'
791+
if self.limit is not None:
792+
query_str += f' LIMIT {self.limit}'
797793
return query_str, filter_params
798794

799795

sky/server/server.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1625,8 +1625,8 @@ async def api_status(
16251625
None, description='Request IDs to get status for.'),
16261626
all_status: bool = fastapi.Query(
16271627
False, description='Get finished requests as well.'),
1628-
request_limit: int = fastapi.Query(
1629-
0, description='Number of requests to show. If 0, show all requests.'),
1628+
limit: Optional[int] = fastapi.Query(
1629+
None, description='Number of requests to show.'),
16301630
fields: Optional[List[str]] = fastapi.Query(
16311631
None, description='Fields to get. If None, get all fields.'),
16321632
) -> List[payloads.RequestPayload]:
@@ -1641,7 +1641,7 @@ async def api_status(
16411641
request_tasks = await requests_lib.get_request_tasks_with_fields_async(
16421642
req_filter=requests_lib.RequestTaskFilter(
16431643
status=statuses,
1644-
request_limit=request_limit,
1644+
limit=limit,
16451645
fields=fields,
16461646
),
16471647
fields=fields,

tests/load_tests/workloads/basic.sh

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,11 @@ sky stop $CLUSTER -y
2828
sky status -u --refresh
2929
sky start $CLUSTER -y
3030
sky down $CLUSTER -y
31-
sky api status
31+
sky api status -l none
3232
sky jobs launch -y -n $JOB --infra $CLOUD 'for i in {1..60}; do echo "$i" && sleep 0.1; done' --workdir ${workdir}
3333
sky jobs queue
3434
sky jobs logs -n $JOB || true
3535
sky jobs logs -n $JOB --controller
3636
sky volumes ls
3737
sky show-gpus
3838
sky cost-report --days 7
39-

tests/smoke_tests/backward_compat/test_backward_compat.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -578,7 +578,7 @@ def test_client_server_compatibility_old_server(self, generic_cloud: str):
578578
f'{self.ACTIVATE_BASE} && {smoke_tests_utils.SKY_API_RESTART} && '
579579
f'sky jobs launch -d --infra {generic_cloud} -y {smoke_tests_utils.LOW_RESOURCE_ARG} -n {job_name} "echo hello world; sleep 60"',
580580
# No restart on switch to current, cli in current, server in base, verify cli works with different version of sky server
581-
f'{self.ACTIVATE_CURRENT} && sky api status',
581+
f'{self.ACTIVATE_CURRENT} && sky api status -l none',
582582
f'{self.ACTIVATE_CURRENT} && sky api info',
583583
f'{self.ACTIVATE_CURRENT} && {self._wait_for_managed_job_status(job_name, [sky.ManagedJobStatus.RUNNING])}',
584584
f'{self.ACTIVATE_CURRENT} && result="$(sky jobs queue)"; echo "$result"; echo "$result" | grep {job_name} | grep RUNNING',

tests/smoke_tests/test_api_server.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,13 +253,13 @@ def test_requests_scheduling(generic_cloud: str):
253253
f'sky launch -y -c {name} --cloud {generic_cloud} {smoke_tests_utils.LOW_RESOURCE_ARG} -n dispatch tests/test_yamls/minimal.yaml',
254254
f'for i in {{1..10}}; do sky exec {name} -n job-${{i}} "sleep 60" --async; done',
255255
# Wait for all reqeusts get scheduled and executed, do not check the status here to make this case focus.
256-
(f's=$(sky api status -a | grep {username});'
256+
(f's=$(sky api status -a -l none | grep {username});'
257257
'timeout=60; start=$SECONDS; '
258258
'until ! echo "$s" | grep "" | grep "PENDING|RUNNING"; do '
259259
' if [ $((SECONDS - start)) -gt $timeout ]; then '
260260
' echo "Timeout waiting for jobs to be scheduled"; exit 1; '
261261
' fi; '
262-
f' sleep 5; s=$(sky api status -a | grep {username});'
262+
f' sleep 5; s=$(sky api status -a -l none | grep {username});'
263263
' echo "Waiting for request get scheduled"; echo "$s"; '
264264
'done'),
265265
],

0 commit comments

Comments
 (0)