weiqingy commented on code in PR #945: URL: https://github.com/apache/flink-agents/pull/945#discussion_r3726297803
########## integrations/chat-models/openai/src/test/java/org/apache/flink/agents/integrations/chatmodels/openai/VLLMChatModelConnectionTest.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.agents.integrations.chatmodels.openai; + +import org.apache.flink.agents.api.resource.ResourceContext; +import org.apache.flink.agents.api.resource.ResourceDescriptor; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; + +/** + * Unit tests for {@link VLLMChatModelConnection} — constructor/default handling only, no network + * access. + */ +class VLLMChatModelConnectionTest { + + private static final ResourceContext NOOP = ResourceContext.fromGetResource((a, b) -> null); + + private static ResourceDescriptor.Builder connectionDescriptor() { + return ResourceDescriptor.Builder.newBuilder(VLLMChatModelConnection.class.getName()); + } + + @Test + @DisplayName("Constructor succeeds with no arguments: api_key and api_base_url are defaulted") + void testConstructorNoArguments() { + // The parent OpenAI connection throws when api_key is missing, so a successful + // construction here proves the vLLM defaults were injected. + ResourceDescriptor desc = connectionDescriptor().build(); + VLLMChatModelConnection conn = new VLLMChatModelConnection(desc, NOOP); + assertThat(conn).isInstanceOf(OpenAICompletionsConnection.class); + } + + @Test + @DisplayName("Constructor defaults blank api_key and api_base_url") + void testConstructorBlankArgumentsAreDefaulted() { + ResourceDescriptor desc = + connectionDescriptor() + .addInitialArgument("api_key", "") + .addInitialArgument("api_base_url", " ") + .build(); + assertThatCode(() -> new VLLMChatModelConnection(desc, NOOP)).doesNotThrowAnyException(); + } + + @Test + @DisplayName("Constructor honors explicit api_key and api_base_url") + void testConstructorExplicitArguments() { + ResourceDescriptor desc = + connectionDescriptor() + .addInitialArgument("api_key", "secret-key") + .addInitialArgument("api_base_url", "http://vllm-host:8000/v1";) + .addInitialArgument("timeout", 30) + .addInitialArgument("max_retries", 1) + .build(); + assertThatCode(() -> new VLLMChatModelConnection(desc, NOOP)).doesNotThrowAnyException(); Review Comment: None of the four tests here asserts the values the connection ends up with. If `withVLLMDefaults` overwrote `api_key` unconditionally, all four would still pass, because `"EMPTY"` never throws in the parent constructor. A real key silently discarded looks like the case this test is aiming at. `OpenAIClient` is private, so there's no seam today, but dropping `private` on `withVLLMDefaults` (`VLLMChatModelConnection.java:72`) would let a same-package test assert on the descriptor it returns. Something like this, if useful: ```java assertThat(VLLMChatModelConnection.withVLLMDefaults(desc).getInitialArguments()) .containsEntry("api_key", "secret-key") .containsEntry("api_base_url", "http://vllm-host:8000/v1";); ``` Is there a reason to keep it `private`? ########## python/flink_agents/integrations/chat_models/vllm/vllm_chat_model.py: ########## @@ -0,0 +1,83 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################# +from typing import Any + +from flink_agents.integrations.chat_models.openai.openai_chat_model import ( + OpenAIChatModelConnection, + OpenAIChatModelSetup, +) + +DEFAULT_VLLM_API_BASE_URL = "http://localhost:8000/v1"; +"""Default base URL of a local ``vllm serve`` instance.""" + +DEFAULT_VLLM_API_KEY = "EMPTY" +"""Placeholder credential used when the vLLM server is started without +``--api-key``. The OpenAI SDK requires a non-empty key, but the server ignores +its value.""" + + +class VLLMChatModelConnection(OpenAIChatModelConnection): + """Connection to a `vLLM <https://docs.vllm.ai>`_ server. + + vLLM exposes an OpenAI-compatible API, so this connection reuses + :class:`OpenAIChatModelConnection` with vLLM-friendly defaults: + + * ``api_base_url`` defaults to ``http://localhost:8000/v1``, the default + address of ``vllm serve``. + * ``api_key`` defaults to a placeholder, since vLLM servers started without + ``--api-key`` do not require a credential. Set it explicitly when the + server is started with ``--api-key``. + + All other attributes (``timeout``, ``max_retries``, ``default_headers``, + ``reuse_client``) behave exactly as in :class:`OpenAIChatModelConnection`. + """ + + def __init__( + self, + *, + api_key: str | None = None, + api_base_url: str | None = None, + **kwargs: Any, + ) -> None: + """Init method.""" + super().__init__( + api_key=api_key or DEFAULT_VLLM_API_KEY, + api_base_url=api_base_url or DEFAULT_VLLM_API_BASE_URL, + **kwargs, + ) + + +class VLLMChatModelSetup(OpenAIChatModelSetup): + """Settings for a chat model served by vLLM. + + Behaves like :class:`OpenAIChatModelSetup` with one difference: ``model`` + is required and has no default, because a vLLM server only serves the + model(s) it was started with — there is no meaningful universal default. + The value must match the model name announced by the server (see + ``vllm serve <model>``, or query ``GET /v1/models``). + """ + + def __init__(self, *, model: str | None = None, **kwargs: Any) -> None: + """Init method.""" + if not model: Review Comment: Java guards with `isBlank()`, Python with plain falsiness, so this line accepts a whitespace-only `model` while `VLLMChatModelSetup.java:52` rejects it. Same split for `api_key` / `api_base_url` at lines 59-60. `AGENTS.md` asks that "Public API changes must keep Java, Python, and YAML APIs semantically aligned", and both classes are new public API. The Java suite covers `" "` (`VLLMChatModelSetupTest.java:51-58`, `VLLMChatModelConnectionTest.java:51-59`); Python covers only `""`. Worth aligning Python on `not model or not model.strip()`? ########## python/flink_agents/integrations/chat_models/vllm/vllm_chat_model.py: ########## @@ -0,0 +1,83 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################# +from typing import Any + +from flink_agents.integrations.chat_models.openai.openai_chat_model import ( + OpenAIChatModelConnection, + OpenAIChatModelSetup, +) + +DEFAULT_VLLM_API_BASE_URL = "http://localhost:8000/v1"; +"""Default base URL of a local ``vllm serve`` instance.""" + +DEFAULT_VLLM_API_KEY = "EMPTY" +"""Placeholder credential used when the vLLM server is started without +``--api-key``. The OpenAI SDK requires a non-empty key, but the server ignores +its value.""" + + +class VLLMChatModelConnection(OpenAIChatModelConnection): + """Connection to a `vLLM <https://docs.vllm.ai>`_ server. + + vLLM exposes an OpenAI-compatible API, so this connection reuses + :class:`OpenAIChatModelConnection` with vLLM-friendly defaults: + + * ``api_base_url`` defaults to ``http://localhost:8000/v1``, the default + address of ``vllm serve``. + * ``api_key`` defaults to a placeholder, since vLLM servers started without + ``--api-key`` do not require a credential. Set it explicitly when the + server is started with ``--api-key``. + + All other attributes (``timeout``, ``max_retries``, ``default_headers``, + ``reuse_client``) behave exactly as in :class:`OpenAIChatModelConnection`. + """ + + def __init__( + self, + *, + api_key: str | None = None, + api_base_url: str | None = None, + **kwargs: Any, + ) -> None: + """Init method.""" + super().__init__( + api_key=api_key or DEFAULT_VLLM_API_KEY, Review Comment: Both arguments are non-`None` by the time they reach the parent, so `resolve_openai_credentials` (`openai_chat_model.py:100`) never gets past `_get_from_param_or_env`'s first branch, and the documented param → env → `openai` module → default order collapses to param-only here. Someone running `vllm serve --api-key <token>` with `OPENAI_API_KEY` exported gets `"EMPTY"` sent and an unexplained 401. There's a real upside too: the defaults stop depending on the developer's environment, and Java has no env fallback either. Was pre-empting the parent's resolution deliberate? If so, would the docstring and the Python table in `chat_models.md` be the place to note that the env vars aren't consulted? ########## docs/content/docs/development/chat_models.md: ########## @@ -1207,6 +1207,113 @@ Some popular options include: Model availability and specifications may change. Always check the official DashScope documentation for the latest information before implementing in production. {{< /hint >}} +### vLLM Review Comment: There's a second place that lists providers: the "Native Integration Support Matrix" in `docs/content/docs/faq/faq.md:98-107`, a three-column table of provider, Python support, Java support. It currently names all eight existing chat models and not vLLM, so a reader who lands there would come away thinking vLLM isn't supported in either language. The rows are alphabetical, so a vLLM row linking to this new section would sit after Tongyi, with both columns ticked. Does that fit in this PR, or would you rather keep the faq change separate? ########## docs/content/docs/development/chat_models.md: ########## @@ -1207,6 +1207,113 @@ Some popular options include: Model availability and specifications may change. Always check the official DashScope documentation for the latest information before implementing in production. {{< /hint >}} +### vLLM + +[vLLM](https://docs.vllm.ai) serves open-weight models behind an OpenAI-compatible API and is a popular choice for self-hosted production deployments. Flink Agents provides a dedicated connection that reuses the OpenAI integration with vLLM-friendly defaults, in both Java and Python. + +#### Prerequisites + +1. Install vLLM and start a server: `vllm serve Qwen/Qwen2.5-7B-Instruct` +2. By default the server listens on `http://localhost:8000` and requires no API key. If the server is started with `--api-key`, pass the same key in the connection. + +#### VLLMChatModelConnection Parameters + +{{< tabs "VLLMChatModelConnection Parameters" >}} + +{{< tab "Python" >}} + +| Parameter | Type | Default | Description | +|-----------|------|---------|-------------| +| `api_base_url` | str | `"http://localhost:8000/v1"` | vLLM server URL | +| `api_key` | str | `"EMPTY"` | Only needed when the server is started with `--api-key`; the placeholder default works for unauthenticated servers | +| `timeout` | float | `60.0` | HTTP request timeout in seconds | +| `max_retries` | int | `3` | Maximum number of API retries | + +{{< /tab >}} + +{{< tab "Java" >}} + +| Parameter | Type | Default | Description | +|-----------|------|---------|-------------| +| `api_base_url` | String | `"http://localhost:8000/v1"` | vLLM server URL | +| `api_key` | String | `"EMPTY"` | Only needed when the server is started with `--api-key`; the placeholder default works for unauthenticated servers | +| `timeout` | Number | SDK default | Seconds before an API call times out | Review Comment: The inherited Java constructor casts both of these to `Integer` (`OpenAICompletionsConnection.java:101,106`) through an unchecked `getArgument`, so a YAML `timeout: 30.5` throws `ClassCastException` at construction. The other Java tables on this page say `int` (523-524, 926-927); want to match them here? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
