search

langchain-ai / langchainjs

🦜🔗 Build context-aware reasoning applications 🦜🔗
https://js.langchain.com/docs/
MIT License
12.4k stars 2.1k forks source link

Bug: Azure Openai function calling in streaming mode #4488

Closed dkindlund closed 3 months ago

dkindlund commented 7 months ago

I think the same bug that happened in https://github.com/langchain-ai/langchain also is happening within https://github.com/langchain-ai/langchainjs.

The details of the bug are mentioned in https://github.com/langchain-ai/langchain/pull/13768

This PR addresses an issue with the OpenAI API streaming response, where initially the key (arguments) is provided but the value is None. Subsequently, it updates with {"arguments": "{\n"}, leading to a type inconsistency that causes an exception. The specific error encountered is ValueError: additional_kwargs["arguments"] already exists in this message, but with a different type. This change aims to resolve this inconsistency and ensure smooth API interactions.

Specifically, if you compare this line in the Python code: https://github.com/langchain-ai/langchain/pull/13768/files#diff-24da96d76a6879fdeae0c02dee3d6be7fee76cc49963d61123c01d85a9f5cec9L79

To this line in the JavaScript code: https://github.com/langchain-ai/langchainjs/blob/main/langchain-core/src/messages/index.ts#L248

I believe the same bug exists.

Can someone confirm and possibly fix this? Thanks!

dosubot[bot] commented 7 months ago

🤖

Thank you for bringing this to our attention! It's great to see your interest in addressing this issue. If you have a proposed solution or fix in mind, we encourage you to create a pull request with your changes. Your contribution would be greatly appreciated by the community. Keep up the good work!

#### About Dosu This response is meant to be useful and save you time. It is **not meant to be a precise solution**, but rather a starting point for your own research. Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant. If you want to continue the conversation, start your reply with @dosu-bot.
dkindlund commented 7 months ago

There's also another Python-based PR mentioning this bug as well: https://github.com/langchain-ai/langchain/pull/13229

@dosu-bot, I don't think I'm familiar enough with the internals of the langchainjs code to submit a PR on this right now. I'm hoping some other langchainjs expert can comment on 1) whether this issue exists and 2) if the python-based fix can be equally applied to this library as well.

dosubot[bot] commented 7 months ago

🤖

Thank you for bringing this to our attention. Based on the information you've provided and the context from the langchainjs repository, it does seem plausible that the issue you've encountered in the Python-based repository could also exist in the JavaScript-based repository.

The line of code you've pointed out in the langchainjs repository is part of the formatURL function, which is used to format a URL by replacing path parameters with their corresponding values. This function is used in the defaultExecutionMethod function to format the URL of an API request before the request is sent.

In the langchainjs codebase, the function that handles the OpenAI API streaming response is mockFetchForOpenAIStream. This function mocks the Fetch API call and handles the streaming response from the OpenAI API. It processes the chunks of data and returns the response, received chunks, any error that occurred, and a boolean indicating whether the process is done or not.

Given the similarity in functionality between the Python and JavaScript implementations, it's possible that the fix applied in the Python-based repository could also be applied here. However, without a deeper understanding of the specific issue and the internals of the langchainjs code, it's hard to say for certain.

I would recommend creating an issue in the langchainjs repository detailing the problem and referencing the fixes in the Python-based repository. This would allow the maintainers and other contributors to the langchainjs repository to investigate the issue further and potentially apply a similar fix.

In the meantime, you might find it helpful to look at a similar solved issue in the langchainjs repository: AzureChatOpenAI Streaming causes IndexError. This issue was closed by a pull request: Azure new preview model support, usability fixes on July 25, 2023. It might provide some insight into how similar issues have been handled in the past.

I hope this information is helpful. If you have any further questions or need additional clarification, please don't hesitate to ask.

For more information, you can refer to the following sources:

-Dosu

Sources

#### About Dosu This response is meant to be useful and save you time. It is **not meant to be a precise solution**, but rather a starting point for your own research. Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant. If you want to continue the conversation, start your reply with @dosu-bot.
dkindlund commented 7 months ago

Hey @dosu-bot, thanks for trying, but it's not part of the formatURL function. Instead, the bug exists in this part of the codebase:

https://github.com/langchain-ai/langchainjs/blob/main/langchain-core/src/messages/index.ts#L248

Specifically, in this function:

export abstract class BaseMessageChunk extends BaseMessage {
  abstract concat(chunk: BaseMessageChunk): BaseMessageChunk;

  static _mergeAdditionalKwargs(
    left: NonNullable<BaseMessageFields["additional_kwargs"]>,
    right: NonNullable<BaseMessageFields["additional_kwargs"]>
  ): NonNullable<BaseMessageFields["additional_kwargs"]> {
    const merged = { ...left };
    for (const [key, value] of Object.entries(right)) {
      if (merged[key] === undefined) {
        merged[key] = value;
      } else if (typeof merged[key] !== typeof value) {
        throw new Error(
          `additional_kwargs[${key}] already exists in the message chunk, but with a different type.`
        );
      } else if (typeof merged[key] === "string") {
        merged[key] = (merged[key] as string) + value;
      } else if (
        !Array.isArray(merged[key]) &&
        typeof merged[key] === "object"
      ) {
        merged[key] = this._mergeAdditionalKwargs(
          merged[key] as NonNullable<BaseMessageFields["additional_kwargs"]>,
          value as NonNullable<BaseMessageFields["additional_kwargs"]>
        );
      } else if (
        key === "tool_calls" &&
        isOpenAIToolCallArray(merged[key]) &&
        isOpenAIToolCallArray(value)
      ) {
        for (const toolCall of value) {
          if (merged[key]?.[toolCall.index] !== undefined) {
            merged[key] = merged[key]?.map((value, i) => {
              if (i !== toolCall.index) {
                return value;
              }
              return {
                ...value,
                ...toolCall,
                function: {
                  name: toolCall.function.name ?? value.function.name,
                  arguments:
                    (value.function.arguments ?? "") +
                    (toolCall.function.arguments ?? ""),
                },
              };
            });
          } else {
            (merged[key] as OpenAIToolCall[])[toolCall.index] = toolCall;
          }
        }
      } else {
        throw new Error(
          `additional_kwargs[${key}] already exists in this message chunk.`
        );
      }
    }
    return merged;
  }
}

The core issue is: The TypeScript code is checking for type differences and undefined values but does not seem to account for the case where a key exists and the value is null (or equivalent) in left and a non-null value is present in right.

While I'm not an langchainjs expert, I did ask GPT-4-Turbo for its recommendation on a fix, and here's what it's saying:

To fix this issue in the TypeScript code, we need to add a check similar to the Python code that looks for null values in the left object and replaces them with the value from the right object if it exists. Here's how you could modify the _mergeAdditionalKwargs method to include this fix (shown as a diff):

static _mergeAdditionalKwargs(  
  left: NonNullable<BaseMessageFields["additional_kwargs"]>,  
  right: NonNullable<BaseMessageFields["additional_kwargs"]>  
): NonNullable<BaseMessageFields["additional_kwargs"]> {  
  const merged = { ...left };  
  for (const [key, value] of Object.entries(right)) {  
-   if (merged[key] === undefined) {  
+   if (merged[key] === undefined || merged[key] === null) {  
      merged[key] = value;  
-   } else if (typeof merged[key] !== typeof value) {  
+   } else if (merged[key] !== null && typeof merged[key] !== typeof value) {  
      throw new Error(  
        `additional_kwargs[${key}] already exists in the message chunk, but with a different type.`  
      );  
    } else if (typeof merged[key] === "string") {  
      merged[key] = (merged[key] as string) + value;  
    } else if (  
      !Array.isArray(merged[key]) &&  
-     typeof merged[key] === "object"  
+     typeof merged[key] === "object" &&  
+     merged[key] !== null  
    ) {  
      merged[key] = this._mergeAdditionalKwargs(  
        merged[key] as NonNullable<BaseMessageFields["additional_kwargs"]>,  
        value as NonNullable<BaseMessageFields["additional_kwargs"]>  
      );  
    } else if (  
      key === "tool_calls" &&  
      isOpenAIToolCallArray(merged[key]) &&  
      isOpenAIToolCallArray(value)  
    ) {  
      // ... existing tool_calls merging code ...  
    } else {  
      throw new Error(  
        `additional_kwargs[${key}] already exists in this message chunk.`  
      );  
    }  
  }  
  return merged;  
}

The changes are:

  1. Added a check for null values in the left object using || merged[key] === null.
  2. Made sure that when we check for type differences with typeof, we also ensure that merged[key] is not null. This prevents incorrectly overwriting non-null values with null ones.
dosubot[bot] commented 7 months ago

🤖

Thank you for bringing this to our attention. Based on the current implementation of the _mergeAdditionalKwargs method, it appears that you're correct. The method does not currently account for the case where a key exists and the value is null in left and a non-null value is present in right.

The suggested fix you provided seems to be a valid solution. It adds a check for null values in the left object and replaces them with the value from the right object if it exists. This would indeed handle the case you described.

However, please note that while this fix might work in your specific case, it's important to consider all possible edge cases and implications of this change. For example, there might be cases where a null value is expected and should not be replaced. Therefore, a comprehensive suite of tests would be necessary to ensure this change does not introduce new bugs or unexpected behavior.

Unfortunately, as a technical support rep, I'm unable to make changes to the codebase. I recommend creating an issue in the langchainjs repository detailing the bug and your proposed fix. This will allow the maintainers and contributors of the repository to review and potentially implement your fix.

For reference, here's the link to create a new issue: New Issue

Please include as much detail as possible in your issue, including the bug description, steps to reproduce, expected and actual results, and your proposed fix. This will help the maintainers and contributors understand the issue and expedite the review process.

I hope this helps! If you have any other questions or need further clarification, feel free to ask.

Sources

#### About Dosu This response is meant to be useful and save you time. It is **not meant to be a precise solution**, but rather a starting point for your own research. Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant. If you want to continue the conversation, start your reply with @dosu-bot.
dkindlund commented 7 months ago

Hey @jacoblee93 and @hcl242, since you guys both have recently handled similar issues with AzureOpenAI, could you possibly take a look at this one to see if you're also getting problems with langchainjs.

The (rough) steps to reproduce are:

The Python langchain developers already confirmed the problem existed in the Python version of langchain, as mentioned in the following PRs: https://github.com/langchain-ai/langchain/pull/13229 https://github.com/langchain-ai/langchain/pull/13768

I'm simply trying to port this fix over to langchainjs as well. Any thoughts would be appreciated -- thanks!

dkindlund commented 7 months ago

There exist unit tests for validating this edge case in the Python langchain library: https://github.com/langchain-ai/langchain/blob/master/libs/core/tests/unit_tests/test_messages.py#L48

...but I don't know where such equivalent unit tests exist in the langchainjs library.

Hey @aymeric-roucher, @hwchase17, since you both were involved with the PR fix for this issue on the Python side, perhaps you could provide more context here on the issue for the JavaScript team? Thanks in advance.

dkindlund commented 7 months ago

Update: We were able to work around this bug by having the LiteLLM OpenAI Proxy adjust its output during streaming function calls.

The OpenAI API returned streaming chunks in this format:

... "function_call":{"name":"web-browser","arguments":""} ...
... "function_call":{"arguments":"{\""} ...
... "function_call":{"arguments":"input"} ...
...

The LiteLLM proxy was returning those same chunks in this format:

... "function_call": {"arguments": "", "name": "web-browser"} ...
... "function_call": {"arguments": "{\"", "name": null} ...
... "function_call": {"arguments": "input", "name": null} ...
...

LangchainJS was throwing errors when processing those chunks from the LiteLLM proxy with this sort of error: additional_kwargs[name] already exists in the message chunk, but with a different type.

The workaround is to have the LiteLLM proxy basically strip the name parameter from each streaming chunk if the value is set to null.

We suspect this problem does not exist in the Python version of Langchain, because additional handling of these corner cases was handled more robustly.

If/when the OpenAI function calling streaming format changes in the future, it would make sense for LangchainJS to eventually have the same robust logic added to it.

For now, though, we've got a successful workaround.

jacoblee93 commented 7 months ago

Glad to hear it! Should tighten this up for sure.