[lengrongfu]

Purpose

FIX #24207

Add a optional generate param to /health?generate=true api, can to generate a max_tokens is 2.

Test Plan

If EngineCore progress crash, this api can response 500 http status code.

Test Result

---

Essential Elements of an Effective PR Description Checklist

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan, such as providing test command.
• The test results, such as pasting the results comparison before and after, or e2e results
• (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[gemini-code-assist]

Code Review

This pull request adds an optional generate parameter to the /health endpoint for a more thorough health check. The implementation is a good start, but the error handling can be made more robust. I've suggested a change to catch a broader range of exceptions and to handle all parts of the health check consistently within a single try...except block. This will prevent unhandled exceptions and ensure that any failure during the health check correctly returns a 500 status code.

[tomasruizt]

2 ideas:

1. The abstract EngineClient could define a method minimal_generation() that encapsulates this logic. It already defines other logic like beam_search(). Perhaps even returning the response string. This makes the minimal generation usable even without the REST API. The REST API delgates to this method when generate=True.

prompt = "Hi"
sampling_params = SamplingParams(temperature=0, max_tokens=2)
request_id = random_uuid()
async for _ in client.generate(prompt, sampling_params,
                               request_id):
    pass

2. Currently, the exception is translated into a http 500 response, but the causing exception is not logged. You could log the exception in the try catch before returning (https://stackoverflow.com/a/5191885/5730291),

except Exception as e:
    logger.exception(e)
    return Response(status_code=500)

[lengrongfu]

@tomasruizt hi, according to you suggest, update done.

[simon-mo]

I have some concern over this design. The request will be enqueued, but if the queue is long (but vLLM is still healthy), the request will timeout. Priority scheduling is not enabled by default. This creates false positive signal for health check generation.

Instead, we should only do generation if there is no generation when engine is idle so we are not interrupting current batch.

Also cc @njhill @robertgshaw2-redhat on the AsyncLLM interface addition

[tomasruizt]

@simon-mo very good point!

I'd like to argue nevertheless, that a timeout on generate is highly informative for the client, since it means, as you said, that the server cannot serve generation requests with the requested timeout constraint.

It's a different outcome than http 500 error, which signals that the server is completely dead.

This is a nuanced difference, but the users of this endpoint (kubernetes users) are also likely to understand it from the endpoint docs.

The difference in outcomes is also clear by the fact that in the timeout outcome the EngineClient does not throw any exception.

[lengrongfu]

We whether can to check scheduler running queue length, if it gt 0, don't exec generate method.

[lengrongfu]

@simon-mo very good point!

I'd like to argue nevertheless, that a timeout on generate is highly informative for the client, since it means, as you said, that the server cannot serve generation requests with the requested timeout constraint.

It's a different outcome than http 500 error, which signals that the server is completely dead.

This is a nuanced difference, but the users of this endpoint (kubernetes users) are also likely to understand it from the endpoint docs.

The difference in outcomes is also clear by the fact that in the timeout outcome the EngineClient does not throw any exception.

I think /health api only make sure server is ok, can work, but generate timeout should use metric to export this issue. so when scheduler running queue length is more than 0 or wating queue length more than 0, we should think this serve is health.
@simon-mo @tomasruizt

[cadedaniel]

It is often helpful to distinguish between readiness and liveness for healthchecks. Ready being ready to handle more load, and live being the server is live and either recovering or starting up.

Separately, can we add a test for this? Otherwise, it is difficult to rely on this feature as it can break at any time.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @lengrongfu.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[markmc]

The assumption here is that if there is any running or waiting request, a new request would complete successfully? Why is that a valid assumption?

[markmc]

Oh, I see Simon's comment:

Instead, we should only do generation if there is no generation when engine is idle so we are not interrupting current batch.

Isn't it likely that if the engine or worker processes are stuck in some way, that there would be in-flight requests also stuck?

[lengrongfu]

Yes, I can only assume that the engine is still working and it is ready, do you have a better suggestion? @markmc

[markmc]

I'm saying we need to actually try to execute a request here if we want to detect hung engine or worker processes

[lengrongfu]

Yes, there are several issues with this approach:

1. If there are too many requests in the queue, the /health request will time out.
2. If the /health request is executed every 3 seconds, it may affect actual user requests.

We need to make a careful trade-off.

[markmc]

No need to collect the outputs, since you don't use them

[markmc]

This doesn't feel like something that should be part of the AsyncLLM API. Can it just be a helper function in the API server code?

[markmc]

Can this hang indefinitely? Can we time out and return unhealthy if hangs?

[lengrongfu]

Currently it is blocking, and a timeout can be set.

However, if you generate directly without checking whether the queue is empty, misjudgment may occur, for example, when a large number of user requests are being processed at this time.

[github-actions]

This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you!

[github-actions]

This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you!