alnzng commented on code in PR #150: URL: https://github.com/apache/flink-agents/pull/150#discussion_r2338657649
########## python/flink_agents/api/embedding_models/embedding_model.py: ########## @@ -0,0 +1,107 @@ +################################################################################ +# 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 abc import ABC, abstractmethod +from typing import Any, Dict + +from pydantic import Field +from typing_extensions import override + +from flink_agents.api.resource import Resource, ResourceType + + +class BaseEmbeddingModelConnection(Resource, ABC): + """Base abstract class for text embedding model connection. + + Responsible for managing model service connection configurations. + Specific implementations can add their own connection parameters like: + - Service address (base_url) for remote services + - API key (api_key) for authenticated services + - Authentication information, timeouts, etc. + + Provides the basic embedding interface for direct communication with model services. + + One connection can be shared in multiple embedding model setup. + """ + + @classmethod + @override + def resource_type(cls) -> ResourceType: + """Return resource type of class.""" + return ResourceType.EMBEDDING_MODEL_CONNECTION + + @abstractmethod + def embed_query(self, text: str, **kwargs: Any) -> list[float]: Review Comment: This `embed_query` naming was influenced by the LangChain's implementation which separate `query` and `documents` embeddings: https://github.com/langchain-ai/langchain/blob/master/libs/core/langchain_core/embeddings/embeddings.py ``` @abstractmethod def embed_documents(self, texts: list[str]) -> list[list[float]]: """Embed search docs. Args: texts: List of text to embed. Returns: List of embeddings. """ @abstractmethod def embed_query(self, text: str) -> list[float]: """Embed query text. Args: text: Text to embed. Returns: Embedding. """ ``` But I don't see any issue that if we want to use a generic method for handing both `query` and `documents` embeddings, especially I confirmed that `ollama` and `openai` APIs did follow this generic method implementation. So let me change it to `embed`. ########## python/flink_agents/api/embedding_models/embedding_model.py: ########## @@ -0,0 +1,107 @@ +################################################################################ +# 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 abc import ABC, abstractmethod +from typing import Any, Dict + +from pydantic import Field +from typing_extensions import override + +from flink_agents.api.resource import Resource, ResourceType + + +class BaseEmbeddingModelConnection(Resource, ABC): + """Base abstract class for text embedding model connection. + + Responsible for managing model service connection configurations. + Specific implementations can add their own connection parameters like: + - Service address (base_url) for remote services + - API key (api_key) for authenticated services + - Authentication information, timeouts, etc. + + Provides the basic embedding interface for direct communication with model services. + + One connection can be shared in multiple embedding model setup. + """ + + @classmethod + @override + def resource_type(cls) -> ResourceType: + """Return resource type of class.""" + return ResourceType.EMBEDDING_MODEL_CONNECTION + + @abstractmethod + def embed_query(self, text: str, **kwargs: Any) -> list[float]: + """Generate embedding vector for a single text query. + + Converts the input text into a high-dimensional vector representation + suitable for semantic similarity search and retrieval operations. + + Args: + text: The text string to convert into an embedding vector. + **kwargs: Additional parameters passed to the embedding model. + + Returns: + A list of floating-point numbers representing the embedding vector. + The dimension of the vector depends on the specific embedding model used. + """ + + +class BaseEmbeddingModelSetup(Resource, ABC): + """Base abstract class for text embedding model setup. + + Responsible for managing embedding model configurations, such as: + - Connection to embedding model service (connection_name) + - Model name (model_name) + + Provides the basic embedding interface for generating embeddings from text inputs. + """ + + connection_name: str = Field(description="Name of the referenced connection.") + model_name: str = Field(description="Name of the embedding model to use.") Review Comment: +1 on this, and below was my thoughts on the pros that providing model in `setup` for either Chat or Embedding Models: Logical Separation: ``` # Connection = "HOW to connect" ChatModelConnection(api_key="...", base_url="...") # Setup = "WHAT to use" + "HOW to configure" ChatModelSetup(connection="conn", model="gpt-4", temperature=0.7) ``` Resource Reuse: ``` # One connection, multiple model configs openai_conn = OpenAIConnection(api_key="...") gpt4_setup = OpenAISetup(connection="openai_conn", model="gpt-4") gpt35_setup = OpenAISetup(connection="openai_conn", model="gpt-3.5-turbo") ``` ########## python/flink_agents/integrations/embedding_models/local/ollama_embedding_model.py: ########## @@ -0,0 +1,165 @@ +################################################################################ +# 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, Dict, Optional, Union + +from ollama import Client +from pydantic import Field + +from flink_agents.api.embedding_models.embedding_model import ( + BaseEmbeddingModelConnection, + BaseEmbeddingModelSetup, +) + +DEFAULT_REQUEST_TIMEOUT = 30.0 + + +class OllamaEmbeddingModelConnection(BaseEmbeddingModelConnection): + """Ollama Embedding Model Connection which manages connection to Ollama server. + + Visit https://ollama.com/ to download and install Ollama. + + Run `ollama serve` to start a server. + + Run `ollama pull <model_name>` to download an embedding model + (e.g. nomic-embed-text, all-minilm, etc) to run. + + Attributes: + ---------- + base_url : str + Base url the Ollama server is hosted under. + model : str + Embedding model name to use. + request_timeout : float + The timeout for making http request to Ollama API server. + """ + + model: str = Field(description="Embedding model name to use.") Review Comment: Good catch! I should remove this `model` and we should only reply on the `model` give in `setup`. (I copied the code from ollama chat model, but didn't do clean up properly). I shared my thought above why I think we'd better define `model` in `setup`: from either Logical Separation and Resource Reuse perspectives. ########## python/flink_agents/integrations/embedding_models/local/tests/start_ollama_server.sh: ########## @@ -0,0 +1,32 @@ +################################################################################ +# 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. +################################################################################ + +# only works on linux +os=$(uname -s) +echo $os +if [[ $os == "Linux" ]]; then + curl -fsSL https://ollama.com/install.sh | sh + ret=$? + if [ "$ret" != "0" ] + then + exit $ret + fi + ollama serve + ollama pull all-minilm:22m + ollama run all-minilm:22m +fi Review Comment: Hmm, I haven't took a look this failure. I thought this might be caused by some env issues, because I saw the tests were executed successfully on different Python versions. <img width="830" height="91" alt="Screenshot 2025-09-10 at 11 03 57 PM" src="https://github.com/user-attachments/assets/bf15dce2-2f4b-4ee9-b350-c7f92188dd54"; /> I feel your theory is reasonable and let me verify it. Agreed with your suggestions on the improvements, let me think more about it. -- 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]
