Repository navigation
feat: refactor langchain integration with integration core - #25
akihikokuroda wants to merge 13 commits into
Conversation
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
| return tool_calls | ||
|
|
||
| @staticmethod | ||
| def _parse_tool_calls_from_string(content_str: str) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
this function is very similar to the one in the base class. do we need to have a separate implementation, or can we make use of the base one?
There was a problem hiding this comment.
This function is not necessary. Removing.
| ) | ||
| except (ValueError, SyntaxError) as e: | ||
| # If parsing fails, skip this tool call | ||
| print(f"Warning: Failed to parse tool call arguments: {e}") |
There was a problem hiding this comment.
should this be a log rather than a print?
| # Create ChatResult with llm_output if provided | ||
| return ChatResult(generations=[generation], llm_output=kwargs.get("llm_output", {})) | ||
|
|
||
| def _extract_tool_calls(self, response: Any) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
similar question about this one, it's also very similar to the function in the base class
There was a problem hiding this comment.
I missed during refactor. Thanks!
| bound_model = self.__class__( | ||
| mellea_session=self.mellea_session, | ||
| model_name=self.model_name, | ||
| streaming=self.streaming, | ||
| requirements=self._requirements, | ||
| strategy=self._strategy, | ||
| ) |
There was a problem hiding this comment.
do we need to pass kwargs too?
| bound_model = self.__class__( | |
| mellea_session=self.mellea_session, | |
| model_name=self.model_name, | |
| streaming=self.streaming, | |
| requirements=self._requirements, | |
| strategy=self._strategy, | |
| ) | |
| bound_model = self.__class__( | |
| mellea_session=self.mellea_session, | |
| model_name=self.model_name, | |
| streaming=self.streaming, | |
| requirements=self._requirements, | |
| strategy=self._strategy, | |
| **kwargs, | |
| ) |
| bound_model._bound_tools = tools # type: ignore | ||
| bound_model._tool_choice = kwargs.get("tool_choice") # type: ignore |
There was a problem hiding this comment.
we may at some point want to consider making these PrivateAttr. There may be problems down the line if someone calls model_dump() on these (which I believe both LangSmith and LangServe do), but not a high priority right now.
|
|
||
| Args: | ||
| messages: List of messages to generate from | ||
| stop: Stop sequences (not currently supported) |
There was a problem hiding this comment.
we should log if a stop sequence is passed and that it is ignored / not supported as of now
|
|
||
| mellea_session: Any = Field(description="The Mellea session to use for generation") | ||
| model_name: str = Field(default="mellea", description="Name to identify this model") | ||
| streaming: bool = Field(default=False, description="Whether to stream responses by default") |
There was a problem hiding this comment.
below (~L395) we mention that we don't support streaming, so should we drop this variable so we don't mislead?
| # Convert response using message converter | ||
| result = self.message_converter.from_mellea(response) | ||
|
|
||
| # Execute tool calls if present |
There was a problem hiding this comment.
My understanding of langchain is that the model returns a message with tool calls, but the agent executor actually runs them and sends results back. Whereas here we're running the tools and including results inside _generate. I think that may cause problems for LangGraph / LangChain agents — worth checking against langgraph's react agent implementation
There was a problem hiding this comment.
Comment put this section of code.
There was a problem hiding this comment.
this is just for testing right? for a library we probably don't want to commit this
| @@ -0,0 +1,75 @@ | |||
| [build-system] | |||
| requires = ["setuptools>=61.0", "wheel"] | |||
| build-backend = "setuptools.build_meta" | |||
There was a problem hiding this comment.
in core we're using hatchling to build. let's standardize
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
| This integration allows LangChain users to leverage Mellea's structured approach to generative programming while using familiar LangChain APIs. It provides full support for: | ||
|
|
||
| - ✅ **Chat Completion**: Standard synchronous and asynchronous generation | ||
| - ✅ **Streaming**: Real-time token-by-token generation |
There was a problem hiding this comment.
We probably need to update this line, as we don't have real streaming implemented in mellea yet
| # NOTE: Tool execution is commented out to align with standard LangChain behavior. | ||
| # In LangChain/LangGraph, the model returns tool calls but doesn't execute them. | ||
| # The agent executor handles tool execution and sends results back to the model. | ||
| # | ||
| # If you need automatic tool execution for testing or specific use cases, | ||
| # uncomment the code below, but be aware this deviates from standard patterns. |
There was a problem hiding this comment.
I would think if someone needed this functionality there should be some alternative implementation rather than having them uncomment code (which they will most likely have to go searching in their site-packages for).
There was a problem hiding this comment.
The AgentExecutor has been deprecated in LangChain. For automatic tool call, it needs LangGraph. What can or should be done here?
| metadata: dict[str, Any] | ||
|
|
||
|
|
||
| class MessageConverter(Protocol): |
There was a problem hiding this comment.
should we also add extract_last_user_message here too?
| # Display tool execution results if available | ||
| if ( | ||
| hasattr(response, "response_metadata") | ||
| and "tool_execution_results" in response.response_metadata |
There was a problem hiding this comment.
will this still show tool execution results with the code you commented out?
| # The model only returns tool calls in the AIMessage, which the agent then executes. | ||
| # The _execute_tool_calls method below is for internal use and compatibility testing only. | ||
|
|
||
| class Config: |
There was a problem hiding this comment.
pydantic has changed how they define configs in v2: https://docs.pydantic.dev/latest/migration/#changes-to-config
since we're using v2 in pyproject.toml, we may want to think about updating
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
For the CI issue fix. close this and open a new one. |
Refactor of #22