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

core[minor], openai[minor], langchain[patch]: output format on openai #17302

Merged
merged 26 commits into from
Feb 22, 2024

Conversation

baskaryan
Copy link
Collaborator

@baskaryan baskaryan commented Feb 9, 2024

class Foo(BaseModel):
  bar: str

structured_llm = ChatOpenAI().with_output_format(Foo)

@efriis efriis added the partner label Feb 9, 2024
@efriis efriis self-assigned this Feb 9, 2024
Copy link

vercel bot commented Feb 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Feb 22, 2024 11:15pm

*,
mode: Literal["tools", "json"] = "tools",
enforce_schema: bool = True,
return_single: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is return_single?

output_schema: Union[Dict[str, Any], Type[_BM]],
*,
mode: Literal["tools", "json"] = "tools",
enforce_schema: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

enforce_schema sounds like validation, but this is not a validation step rather model forced to do tool invocation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what's a better name? want something not too openai tools specific if we're going to add this method (or similar methods) to other models


def with_output_format(
self,
output_schema: Union[Dict[str, Any], Type[_BM]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an opinion of what to do about multiple schemas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The underlying API supports multiple tools which could be represented as different schemas

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would you just want to use bind_tools in that case?

mode: Literal["tools", "json"] = "tools",
enforce_schema: bool = True,
return_single: bool = True,
) -> Runnable[LanguageModelInput, _BM]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure that this should return pydantic model by default

To me at least there are 2 separate ideas: (i) schema specification, (ii) optional validation against the schema

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for schema specification, not hard to turn pydantic -> dict if you don't want pydantic output

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 like it. Small q

Comment on lines 63 to 64
_OutputSchema = TypeVar("_OutputSchema")
_OutputFormat = TypeVar("_OutputFormat")
Copy link
Member

Choose a reason for hiding this comment

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

should we restrict these a bit? Right now could be anything right? Seems difficult to use with both.

Just an output base model as well and force it to be pydantic or general json type (list/dict/etc) doesn't seem too bad to me

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Feb 14, 2024
""""""
if kwargs:
raise ValueError(f"Received unsupported arguments {kwargs}")
is_pydantic_schema = _is_pydantic_class(output_schema)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still in favor of making a conceptual distinction between schema declaration vs. the parsing.

For example, I will usually want to declare my schema with pydantic, but I don't necessarily want the output to be in pydantic (e.g., i want to stream the structured json out)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can always dict_schema = convert_to_openai_tool(pydantic_schema) before passing in schema? i just don't like adding a parameter until we know it's really needed. we can always add it later

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK that will work. We can add a validator='auto'" parameter later on, allowing one to set validator=Noneorvalidator=Type[BaseModel]` or something along those lines?


def with_output_format(
self,
output_schema: _OutputSchema,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an escape hatch to say that nothing can be extracted?

tool_choice = True -> so tool call will always be forced

The edge case that we need to handle:

  • Schema is a single instance rather than a collection
  • Goal for example: identify the most qualified candidate based on the following list of candidates and their qualifications

@baskaryan baskaryan changed the title rfc: output format on openai core[minor], openai[minor]: output format on openai Feb 15, 2024
@baskaryan baskaryan marked this pull request as ready for review February 15, 2024 21:27
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. Ɑ: models Related to LLMs or chat model modules 🔌: openai Primarily related to OpenAI integrations 🤖:improvement Medium size change to existing code to handle new use-cases labels Feb 15, 2024
Copy link
Collaborator Author

@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.

  • mv PydanticOutputParser from langchain -> core
  • update OpenAI tools parsers to support return_single
  • cp updated OpenAI tools parsers from langchain -> openai
  • introduce FormattedOutputMixin
  • update ChatOpenAI to implement FormattedOutputMixin

@baskaryan baskaryan changed the title core[minor], openai[minor]: output format on openai core[minor], openai[minor], langchain[minor]: output format on openai Feb 15, 2024
@baskaryan baskaryan changed the title core[minor], openai[minor], langchain[minor]: output format on openai core[minor], openai[minor], langchain[patch]: output format on openai Feb 15, 2024
libs/core/langchain_core/language_models/output_format.py Outdated Show resolved Hide resolved
libs/core/langchain_core/language_models/output_format.py Outdated Show resolved Hide resolved
Comment on lines 206 to 208
_BM = TypeVar("_BM", bound=BaseModel)
_OutputSchema = Union[Dict[str, Any], Type[_BM]]
_FormattedOutput = Union[BaseMessage, Dict, _BM]
Copy link
Member

Choose a reason for hiding this comment

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

Would it be simpler if we had these be the types for any model that supports structured outputs, so each implementation doesn't have to worry about generics? Just whether or not it supports structured outputs?

Comment on lines 757 to 769
method: Literal["function_calling", "json_mode"] = "function_calling",
return_type: Literal["all"] = "all",
**kwargs: Any,
) -> Runnable[LanguageModelInput, _AllFormattedOutput]:
...

@overload
def with_output_format(
self,
schema: _OutputSchema,
*,
method: Literal["function_calling", "json_mode"] = "function_calling",
return_type: Literal["parsed"] = "parsed",
Copy link
Member

Choose a reason for hiding this comment

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

The different defaults on the overloads are a bit confusing. Shouldn't defaults be the same as the catch-all definition?

Copy link
Member

Choose a reason for hiding this comment

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

i.e. Shouldn't return_type default always be parsed or not have a default? For the overload of return_type: Literal["all"] I think the user has to have it manually defined, so maybe shouldn't have a default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the types can't overlap for overloads

_OutputSchema = TypeVar("_OutputSchema")


class StructuredOutputMixin(Generic[_OutputSchema], ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

we are back to NOT putting this on base class? why not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will only be documented on classes that implement it

Copy link
Contributor

Choose a reason for hiding this comment

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

so the downside of putting on base class (and defaulting to not implemented) is that it will show up in the documentation for all classes? that doesnt seem that bad...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what's downside of mixin?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not that it shows up in documentation, but that it's hard to discover which models actually implement the interface.

After we implement, this we'll need to figure out how users will discover that this is an option

from langchain_core.pydantic_v1 import BaseModel
from langchain_core.runnables import Runnable

_OutputSchema = TypeVar("_OutputSchema")
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this? IMO this doesnt really add that much and makes it harder to user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

harder for user or contributor? don't think affects user experience?

Copy link
Contributor

Choose a reason for hiding this comment

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

contributor + any user that tries to read the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the places the mixin is implemented there won't be a typevar, so what's the hard part for someone reading code?

don't feel super strongly on this though if we think typevar is that bad

@eyurtsev
Copy link
Collaborator

This fails with an opaque message due to missing doc-string

from langchain_openai.chat_models import ChatOpenAI

model = ChatOpenAI()

from pydantic import BaseModel

class Person(BaseModel):
    name: str
    age: int

model.with_structured_output(Person).invoke('hello my name is chester')
BadRequestError: Error code: 400 - {'error': {'message': '"Usage docs: https://docs.pydantic.dev/2.6/concepts/models/ A base class for creating Pydantic models. Attributes:\\n    __class_vars__: The names of classvars defined on the model.\\n    __private_attributes__: Metadata about the private attributes of the model.\\n    __signature__: The signature for instantiating the model.     __pydantic_complete__: Whether model building is completed, or if there are still undefined fields.\\n    __pydantic_core_schema__: The pydantic-core schema used to build the SchemaValidator and SchemaSerializer.\\n    __pydantic_custom_init__: Whether the model has a custom `__init__` function.\\n    __pydantic_decorators__: Metadata containing the decorators defined on the model.\\n        This replaces `Model.__validators__` and `Model.__root_validators__` from Pydantic V1.\\n    __pydantic_generic_metadata__: Metadata for generic models; contains data used for a similar purpose to\\n        __args__, __origin__, __parameters__ in typing-module generics. May eventually be replaced by these.\\n    __pydantic_parent_namespace__: Parent namespace of the model, used for automatic rebuilding of models.\\n    __pydantic_post_init__: The name of the post-init method for the model, if defined.\\n    __pydantic_root_model__: Whether the model is a `RootModel`.\\n    __pydantic_serializer__: The pydantic-core SchemaSerializer used to dump instances of the model.\\n    __pydantic_validator__: The pydantic-core SchemaValidator used to validate instances of the model.     __pydantic_extra__: An instance attribute with the values of extra fields from validation when\\n        `model_config[\'extra\'] == \'allow\'`.\\n    __pydantic_fields_set__: An instance attribute with the names of fields explicitly set.\\n    __pydantic_private__: Instance attribute with the values of private attributes set on the model instance." is too long - \'tools.0.function.description\'', 'type': 'invalid_request_error', 'param': None, 'code': None}}```

@eyurtsev
Copy link
Collaborator

Fails silently -- we could add an instance check maybe?

from typing import List

from langchain_openai.chat_models import ChatOpenAI

model = ChatOpenAI()

from pydantic import BaseModel

class Person(BaseModel):
    name: str
    age: int


model.with_structured_output(List[Person]).invoke('hello my name is chester and i am 20 years old')

@@ -22,6 +22,8 @@ class JsonOutputToolsParser(BaseGenerationOutputParser[Any]):
"""
return_id: bool = False
"""Whether to return the tool call id."""
return_single: bool = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we think that return_single is generic enough to warrant introducing it into the parser? Is this the right place? How does this interact with streaming? Would unpacking at the caller code work?


Also if we do keep it here is there a better name? e.g., first_tool_only?

The doc-string should probably explain why this is necessary?

)
else:
key_name = convert_to_openai_tool(schema)["function"]["name"]
output_parser = JsonOutputKeyToolsParser(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use a runnable lambda here to unpack? is it to make a clearer trace?

@efriis efriis removed their assignment Feb 21, 2024
@efriis efriis self-assigned this Feb 22, 2024
@baskaryan baskaryan merged commit b5f8cf9 into master Feb 22, 2024
91 checks passed
@baskaryan baskaryan deleted the bagatur/rfc_structured_on_openai branch February 22, 2024 23:33
k8si pushed a commit to Mozilla-Ocho/langchain that referenced this pull request Feb 23, 2024
…structured_output langchain-ai#17302)

```python
class Foo(BaseModel):
  bar: str

structured_llm = ChatOpenAI().with_structured_output(Foo)
```

---------

Co-authored-by: Erick Friis <erick@langchain.dev>
al1p pushed a commit to al1p/langchain that referenced this pull request Feb 27, 2024
…structured_output langchain-ai#17302)

```python
class Foo(BaseModel):
  bar: str

structured_llm = ChatOpenAI().with_structured_output(Foo)
```

---------

Co-authored-by: Erick Friis <erick@langchain.dev>
haydeniw pushed a commit to haydeniw/langchain that referenced this pull request Feb 27, 2024
…structured_output langchain-ai#17302)

```python
class Foo(BaseModel):
  bar: str

structured_llm = ChatOpenAI().with_structured_output(Foo)
```

---------

Co-authored-by: Erick Friis <erick@langchain.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases lgtm PR looks good. Use to confirm that a PR is ready for merging. Ɑ: models Related to LLMs or chat model modules 🔌: openai Primarily related to OpenAI integrations partner size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants