-
-
Notifications
You must be signed in to change notification settings - Fork 15.6k
[openai api] log exception in exception handler (1/N) #31164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,18 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | ||
| import json | ||
| from http import HTTPStatus | ||
| from typing import Final | ||
|
|
||
| import pytest | ||
| import schemathesis | ||
| from httpx import URL | ||
| from hypothesis import settings | ||
| from schemathesis import GenerationConfig | ||
| from schemathesis.checks import not_a_server_error | ||
| from schemathesis.internal.checks import CheckContext | ||
| from schemathesis.models import Case | ||
| from schemathesis.transports.responses import GenericResponse | ||
|
|
||
| from ...utils import RemoteOpenAIServer | ||
|
|
||
|
|
@@ -127,10 +133,25 @@ def no_invalid_types(case: schemathesis.models.Case): | |
| return strategy.filter(no_invalid_types) | ||
|
|
||
|
|
||
| def customized_not_a_server_error( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New changes. By default all server error will cause an exception or failure in schemathesis. We need to customize this by allowing |
||
| ctx: CheckContext, response: GenericResponse, case: Case | ||
| ) -> bool | None: | ||
| try: | ||
| return not_a_server_error(ctx, response, case) | ||
| except Exception: | ||
| if ( | ||
| URL(response.request.url).path | ||
| in ["/v1/chat/completions/render", "/v1/chat/completions"] | ||
| and response.status_code == HTTPStatus.NOT_IMPLEMENTED.value | ||
| ): | ||
| return True | ||
| raise | ||
|
|
||
|
|
||
| @schema.parametrize() | ||
| @schema.override(headers={"Content-Type": "application/json"}) | ||
| @settings(deadline=LONG_TIMEOUT_SECONDS * 1000, max_examples=50) | ||
| def test_openapi_stateless(case: schemathesis.Case): | ||
| def test_openapi_stateless(case: Case): | ||
| key = ( | ||
| case.operation.method.upper(), | ||
| case.operation.path, | ||
|
|
@@ -155,4 +176,9 @@ def test_openapi_stateless(case: schemathesis.Case): | |
| }.get(key, DEFAULT_TIMEOUT_SECONDS) | ||
|
|
||
| # No need to verify SSL certificate for localhost | ||
| case.call_and_validate(verify=False, timeout=timeout) | ||
| case.call_and_validate( | ||
| verify=False, | ||
| timeout=timeout, | ||
| additional_checks=(customized_not_a_server_error,), | ||
| excluded_checks=(not_a_server_error,), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ def chat(request: Request) -> OpenAIServingChat | None: | |
| HTTPStatus.BAD_REQUEST.value: {"model": ErrorResponse}, | ||
| HTTPStatus.NOT_FOUND.value: {"model": ErrorResponse}, | ||
| HTTPStatus.INTERNAL_SERVER_ERROR.value: {"model": ErrorResponse}, | ||
| HTTPStatus.NOT_IMPLEMENTED.value: {"model": ErrorResponse}, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }, | ||
| ) | ||
| @with_cancellation | ||
|
|
@@ -54,10 +55,7 @@ async def create_chat_completion(request: ChatCompletionRequest, raw_request: Re | |
| message="The model does not support Chat Completions API" | ||
| ) | ||
|
|
||
| try: | ||
| generator = await handler.create_chat_completion(request, raw_request) | ||
| except Exception as e: | ||
| generator = handler.create_error_response(e) | ||
| generator = await handler.create_chat_completion(request, raw_request) | ||
|
|
||
| if isinstance(generator, ErrorResponse): | ||
| return JSONResponse( | ||
|
|
@@ -81,6 +79,7 @@ async def create_chat_completion(request: ChatCompletionRequest, raw_request: Re | |
| HTTPStatus.BAD_REQUEST.value: {"model": ErrorResponse}, | ||
| HTTPStatus.NOT_FOUND.value: {"model": ErrorResponse}, | ||
| HTTPStatus.INTERNAL_SERVER_ERROR.value: {"model": ErrorResponse}, | ||
| HTTPStatus.NOT_IMPLEMENTED.value: {"model": ErrorResponse}, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }, | ||
| ) | ||
| async def render_chat_completion(request: ChatCompletionRequest, raw_request: Request): | ||
|
|
@@ -93,10 +92,7 @@ async def render_chat_completion(request: ChatCompletionRequest, raw_request: Re | |
| message="The model does not support Chat Completions API" | ||
| ) | ||
|
|
||
| try: | ||
| result = await handler.render_chat_request(request) | ||
| except Exception as e: | ||
| result = handler.create_error_response(e) | ||
| result = await handler.render_chat_request(request) | ||
|
|
||
| if isinstance(result, ErrorResponse): | ||
| return JSONResponse(content=result.model_dump(), status_code=result.error.code) | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make an e2e version of this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need some time to add an e2e test for this case. Can we postpone this later and merge this pr firstly. I have tested this manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ask claude or codex to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add this test in the next PR.