Skip to content
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

[Breaking] change the behavior of ChatVertexAI.with_structured_output when specifying a dict #333

Merged

Conversation

kiarina
Copy link
Contributor

@kiarina kiarina commented Jun 27, 2024

The behavior of ChatVertexAI.with_structured_output when specifying a dict was different from ChatOpenAI and ChatAnthropicVertex, so it has been corrected.

  • Modified to accept not only the ["name", "description", "parameters"] format for the dict but also ["title", "description", "properties"].
  • When a dict is specified as the schema, it now extracts properties from the output.

This is considered breaking as it changes existing functionality.
If this change is unacceptable, please feel free to close it.

@lkuligin lkuligin requested a review from efriis June 27, 2024 12:43
@lkuligin
Copy link
Collaborator

asking @efriis for the second opinion :), but the change looks reasonable to me.
thanks for the PR!

Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will also break cases where a protobuf is passed in, which might be a useful feature (to match what vertex api provides). Will take a closer look when I'm back from vacation next week, and @baskaryan might have some takes before then!

The common abstraction currently includes pydantic BaseModel

The question seems to be whether we want to have the common abstraction also include:

  1. Whatever the provider API supports by default
  2. JSON Schema (OpenAI/Anthropic)

In general I'm more supportive of 1 (and closing this), and @baskaryan may have different input!

Copy link
Contributor

@baskaryan baskaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a good change but technically a breaking one, and dont think we can call it a bugfix. if we land we'll probably want to accompany with a minor version bump

@baskaryan
Copy link
Contributor

I think this will also break cases where a protobuf is passed in, which might be a useful feature (to match what vertex api provides). Will take a closer look when I'm back from vacation next week, and @baskaryan might have some takes before then!

The common abstraction currently includes pydantic BaseModel

The question seems to be whether we want to have the common abstraction also include:

  1. Whatever the provider API supports by default
  2. JSON Schema (OpenAI/Anthropic)

In general I'm more supportive of 1 (and closing this), and @baskaryan may have different input!

re: breaking support for protobuf schemas, agree we should avoid this. could we either explicitly check the type of the schema before calling convert_to_openai_function, or else try/except that conversion. and when the schema cannot be converted to openai format we keep current behavior

@kiarina
Copy link
Contributor Author

kiarina commented Jun 28, 2024

re: breaking support for protobuf schemas, agree we should avoid this. could we either explicitly check the type of the schema before calling convert_to_openai_function, or else try/except that conversion. and when the schema cannot be converted to openai format we keep current behavior

        elif isinstance(schema, dict) and all(
            k in schema for k in ("title", "description", "properties")
        ):
            schema = convert_to_openai_function(schema)
            parser = JsonOutputKeyToolsParser(
                key_name=schema["name"], first_tool_only=True
            )
        else:
            parser = JsonOutputToolsParser()

I've made it so that the behavior changes only when a dict containing title, description, and properties is passed. How does this look?

@kiarina kiarina force-pushed the improve-vertexai-structured-output branch from 65bc63c to 64a80e6 Compare June 28, 2024 09:59
@lkuligin lkuligin requested a review from baskaryan July 1, 2024 15:26
@lkuligin lkuligin merged commit 4f3c985 into langchain-ai:main Jul 2, 2024
19 checks passed
@kiarina kiarina deleted the improve-vertexai-structured-output branch July 2, 2024 18:46
@baskaryan
Copy link
Contributor

we should make the same fix in ChatGoogleGenerativeAI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants