feature: Check sleep mode only interacting with the /is_sleeping endpoint

[jonoillar]

Describe the feature

Scenario:

I was testing the router with a fake-server, very similar to this one: https://github.com/vllm-project/production-stack/blob/main/src/tests/perftest/fake-openai-server.py

I packaged the app within a docker image:

FROM python:3.10-slim AS build

# Install dependencies
COPY requirements.txt .
RUN apt-get update && apt-get install -y gcc
RUN pip install --no-cache-dir --user -r requirements.txt

# Set working directory
WORKDIR /opt/project


# Copy application code
COPY . .
# Set PYTHONPATH
ENV PYTHONPATH=/opt/project

CMD ["python3", "./fake-openai-server.py", "--host", "0.0.0.0", "--port", "8000"]

And deploying it within kubernetes, here is the pod spec:

apiVersion: v1
kind: Pod
metadata:
  generateName: vllm-k8s-test-fake-server-fake-model-deployment-vllm-5bc7967b99-
  generation: 1
  labels:
    environment: test
    helm-release-name: vllm-k8s-test-fake-server
    model: fake-model
    pod-template-hash: 5bc7967b99
    release: test
  name: vllm-k8s-test-fake-server-fake-model-deployment-vllm-5bc79kndb9
  namespace: default
  ownerReferences:
  - apiVersion: apps/v1
    blockOwnerDeletion: true
    controller: true
    kind: ReplicaSet
    name: vllm-k8s-test-fake-server-fake-model-deployment-vllm-5bc7967b99
spec:
  containers:
  - image: fake-server:latest
    imagePullPolicy: IfNotPresent
    livenessProbe:
      failureThreshold: 3
      httpGet:
        path: /health
        port: 8000
        scheme: HTTP
      initialDelaySeconds: 15
      periodSeconds: 10
      successThreshold: 1
      timeoutSeconds: 1
    name: vllm
    ports:
    - containerPort: 8000
      name: container-port
      protocol: TCP
    - containerPort: 55555
      name: zmq-port
      protocol: TCP
    - containerPort: 9999
      name: ucx-port
      protocol: TCP
    startupProbe:
      failureThreshold: 60
      httpGet:
        path: /health
        port: 8000
        scheme: HTTP
      initialDelaySeconds: 15
      periodSeconds: 10
      successThreshold: 1
      timeoutSeconds: 1

Important thing to note: there is no command section within the pod definition. This is because I just want to run the command that is specified in the docker image

And then within the vllm router, within the service discovery I was getting this error:

K8s watcher error: 'NoneType' object is not iterable (service_discovery.py:617:vllm_router.service_discovery)

I dug down trying to understand when this error could occur.

In the _check_engine_sleep_mode I found this piece of code:

            pod = self.k8s_api.read_namespaced_pod(
                name=pod_name, namespace=self.namespace
            )
            for container in pod.spec.containers:
                if container.name == "vllm":
                    for arg in container.command:
                        if arg == "--enable-sleep-mode":
                            enable_sleep_mode = True
                            break
            return enable_sleep_mode

This code breaks in the line for arg in container.command: if no command has been provided to the pod -> kubernetes sets it by default to None

Modification suggestion

I think the vllm-router should not make any assumption on what is the command set in the pod definition of the model pod. I think the API endpoint exposed by the model pods are the only "contract" made between vllm router and vllm model pods
If someone wants to create a custom vllm image, that doesn't need to be run with the exact same command as vllm OR that already contains the command within the docker image, then the vllm router should work with it.

That's why I suggest, for testing if an engine is sleeping or not, to only query the is_sleeping endpoint.

• if the endpoint raises 404 (not defined) -> we consider engine is not sleeping
• if endpoint returns false -> we consider engine is sleeping
• if endpoint returns true -> we consider engine is sleeping

please tell me if you like the suggestion

Why do you need this feature?

No response

Additional context

No response

[jonoillar]

https://github.com/vllm-project/vllm/blob/325aa3dee922b344a26b9e74d9ae3c769828e70e/docs/features/sleep_mode.md?plain=1#L80 they say that /is_sleeping endpoint is only available in a particular deployment mode, which I don't think it is ideal but well that's how it is. That's why I proposed to implement the logic "if the endpoint raises 404 (not defined) -> we consider engine is not sleeping" within the router

[YuhanLiu11]

Thanks for submitting the issue.
@dumb0002 Can you take a look at this issue? Thanks!

[dumb0002]

@jonoillar (cc:@YuhanLiu11): thanks for raising this issue. The is_sleeping endpoint is only available when the following are set: env var VLLM_SERVER_DEV_MODE=1 and flag --enable-sleep-mode. Then, it's not necessary to continually query that endpoint if the vLLM engine is not configured to expose that endpoint. During my implementation, I come across this scenario, and I noticed that querying is_sleeping endpoint when engine is not exposing corrupts the logs of the PS router: it shows as error messages or failed requests in the logs every 10 seconds (the default watch interval for vLLM pods by the PS router). This become even worst if we have multiple PS routers deployed in the same environment for scalability or high availability scenarios. A simpler solution for the issue that you raised above could be just to check if container.command is set or not for a container.

[jonoillar]

Thanks for the explanation @dumb0002 !

A simpler solution for the issue that you raised above could be just to check if container.command is set or not for a container.

That is the solution that you have implemented as of today. My point is that solution is not super robust: the vllm router should not assume anything as how the vllm model pod has been launched.

My use case was a vllm model pod launched without specifying the container.command. This is because the command was already specified in the docker image.
This use case happens a lot when I need to create a custom vllm image for a model that is not currently supported by vllm.

However, I do understand your point that the /is_sleeping endpoint doesn't always exist, and that querying an endpoint that we know doesn't work will fill the logs with noise.

A few ideas to avoid filling the logs with noise AND having a more robust implementation:

1. The first time /is_sleeping is called and raises an error for a given model pod, for the rest of the watch don't call it ever again
2. Ignore the logs of the /is_sleeping endpoint call in the vllm router -> don't fill vllm router logs with noise

The point 1. might be a bit complicated with the way the service discovery is set now. But 2. could be quite easy to implement

[dumb0002]

@jonoillar, thanks for the detailed comment above. You raised an interesting use-case that needs to be covered indeed.

However, any solution needs to avoid impacting the performance of the PS, for example: constantly querying the /is_sleeping endpoint for every request will increase the latency of processing requests. Hence, I propose a slight variation of the point (1) that you proposed above as following:

1. Check if container.command is set or not for a container (currently it is not done);

2. If container.command is not set then query the /is_sleeping endpoint once to verify if sleep mode was enabled. If the response is successful, then set the variable enable_sleep_mode = True;

The issue of noisy log will only exist for your specific use-case which is not the most common way vLLM pods are deployed.

[jonoillar]

Thanks for the proposal

I'm comparing my proposal and your proposal and explore the different use cases:

@dumb0002 proposal

1. Check if container.command is set or not for a container (currently it is not done);

2. If container.command is not set then query the /is_sleeping endpoint once to verify if sleep mode was enabled. If the response is successful, then set the variable enable_sleep_mode = True;

When container.command is set AND /is_sleeping endpoint available

In this case, at each pass to _add_engine, the /is_sleeping will be called ONCE

When container.command is set AND /is_sleeping endpoint unavailable

In this case, at each pass to _add_engine, the /is_sleeping will NOT be called

When container.command is NOT set AND /is_sleeping endpoint available

In this case, at each pass to _add_engine, the /is_sleeping will be called TWICE: once in _check_engine_sleep_mode and once in _get_engine_sleep_status

When container.command is NOT set AND /is_sleeping endpoint unavailable

In this case, at each pass to _add_engine, the /is_sleeping will be called ONCE: in _check_engine_sleep_mode

@jonoillar proposal

Merge _check_engine_sleep_mode and _get_engine_sleep_status into one function -> _get_engine_sleep_status_v2

Logic of the function: always call the /is_sleeping endpoint once and apply this logic to the answer:

• if the endpoint raises 404 (not defined) -> we consider engine is not sleeping
• if endpoint returns false -> we consider engine is sleeping
• if endpoint returns true -> we consider engine is sleeping

Benefit

1. For the use case When container.command is NOT set AND /is_sleeping endpoint available, we do ONE less call to /is_sleeping
2. Better consistency in the code: we don't rely on how specifically the vLLM pods are deployed

SAME

When container.command is NOT set AND /is_sleeping endpoint unavailable and When container.command is set AND /is_sleeping endpoint available will have the same amount of call to /is_sleeping

Drawback

When container.command is set AND /is_sleeping endpoint unavailable will have one 404 call to /is_sleeping

Note: the _add_engine method should ideally not be called more than ONCE per vLLM pods. That's actually a fix that I'm doing in this PR: #666 .

In that case, the benefit is:

1. One less call PER model pods to /is_sleeping for use case When container.command is NOT set AND /is_sleeping endpoint available
2. Code consistency

Drawback:

1. One more call PER model pods to /is_sleeping for use case When container.command is set AND /is_sleeping endpoint unavailable

Hope this is clear, and I let you decide which tradeoff you are willing to accept 👍 For me the code consistency should not be disregarded

[dumb0002]

@jonoillar, thanks for the detailed description above. I am a bit concerned with the following proposed function below as it might have negative impact in performance:

Logic of the function: always call the /is_sleeping endpoint once and apply this logic to the answer:

if the endpoint raises 404 (not defined) -> we consider engine is not sleeping
if endpoint returns false -> we consider engine is sleeping
if endpoint returns true -> we consider engine is sleeping

Making constant http calls to discover a feature that is not set by default might not be the optimal design - these calls consume some resources and in scenarios with multiple routers and many vLLM instances that could be noticeable. Furthermore, the PS watch function for vLLM pods resets every 10 seconds. Hence, if the vLLM engine is not configured on sleeping mode, will have constant calls to the is_sleeping endpoint every 10 seconds and 404 error messages. I am not sure yet how PR: #666 would change this behavior. Could you explain a little bit? Thanks!

[jonoillar]

Thanks for the answer! I understand the doubts completely

Making constant http calls to discover a feature that is not set by default might not be the optimal design - these calls consume some resources and in scenarios with multiple routers and many vLLM instances that could be noticeable.

Totally agree

Furthermore, the PS watch function for vLLM pods resets every 10 seconds. Hence, if the vLLM engine is not configured on sleeping mode, will have constant calls to the is_sleeping endpoint every 10 seconds and 404 error messages.

Since this PR: 8367d43

When setting k8s-watcher-timeout-seconds=0, the watch function never resets. In practical terms this means that for each pod there will be:

• Only one ADDED event
• MODIFIED event only when pod gets modified

In that case, since the MODIFIED event are not that common, I'd say we can afford having.

Before this PR, the watch function was resetting every 30 seconds. This practically meant that every 30 seconds, there was one ADDED event related to each pod. Which means that every 30 seconds, there was a call to /is_sleeping endpoint. In this case, I agree that having every 30 seconds a call that leads to 404 error message uses ressources for nothing 👍

I am not sure yet how PR: #666 would change this behavior. Could you explain a little bit? Thanks!

This PR aimed to have only 1 ADDED event per pod. Which, as explained above, has been achieved by PR 8367d43 . I already explained above the benefit of having only 1 ADDED event per pod 😉

Caveat: I've been investigating more the behaviour of the watch engine. In this comment: #668 (comment) I displayed scenarios where it is not desirable to have only 1 ADDED event per pod

In that case, your proposal (as displayed in #655 (comment)) is probably the best tradeoff. We could even tweak it a bit so that in the use case When container.command is NOT set AND /is_sleeping endpoint available, we only call the /is_sleeping endpoint once

Thanks for your patience and very useful comments !!

[dumb0002]

@jonoillar, thanks for this discussion as well as for the detailed response and explanation above.

In that case, your proposal (as displayed in #655 (comment)) is probably the best tradeoff. We could even tweak it a bit so that in the use case When container.command is NOT set AND /is_sleeping endpoint available, we only call the /is_sleeping endpoint once.

To address it, we could instead just set the var enable_sleep_mode = True always if container.command is NOT set and not make any calls to is_sleeping endpoint inside the func _check_engine_sleep_mode().

The changes in PR # 668 for the watch function might have introduced another issue: "if an engine is put to sleep after already started it will not be discovered by the router when another application bypass the router to put the engine to sleep."

BTW, although k8s-watcher-timeout-seconds is now set to zero the k8s server will close a watch stream after a period of time to rebalance load.

FYI, I have pushed a PR to vLLM upstream that is related to our discussion here: vllm-project/vllm#24176. This PR will help to address the issue that I just described above.

[dumb0002]

@jonoillar (cc: @YuhanLiu11), I opened this PR for the fix that we discussed here: #696