Skip to content

core[minor]: StringOutputParser and text BaseMessage contents #4705

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

Merged
merged 5 commits into from
Mar 12, 2024

Conversation

afirstenberg
Copy link
Contributor

Added a number of methods to BaseLLMOutputParser and call from invoke() to allow subclasses to override the string representation of a BaseMessage. Take advantage of this as part of StringOutputParser to make sure any text types are represented as human-readable strings.

Fixes #4695

Copy link

vercel bot commented Mar 11, 2024

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

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 8:34pm
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 12, 2024 8:34pm

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. auto:improvement Medium size change to existing code to handle new use-cases labels Mar 11, 2024
@jacoblee93
Copy link
Collaborator

Thank you for this! The silent failure/truncation on complex message image outputs makes me a bit nervous though. Happy to make the change myself.

@jacoblee93 jacoblee93 added the close PRs that need one or two touch-ups to be ready label Mar 12, 2024
@afirstenberg
Copy link
Contributor Author

I have no strong feelings, but here's my justification for choosing an empty string:

  • Converting something to a displayable string should never error out. There should always be SOMETHING to display to the user
  • One could argue if that should be "" or "[IMG]" or something else.
  • It is easy to override (it is in it's own function)
  • I don't think any model is sending this back right now, but when a model does, it shouldn't break existing code.

That said, as long as the _imageUrlContentToString() function is the one throwing the exception, so it can be overridden - I have no objections. (May also need to add something to the default switch statement as well to handle any future undefined types. I'm better with that being an exception since there are no other types defined today.)

@jacoblee93 jacoblee93 changed the title core [minor]: StringOutputParser and more complicated BaseMessage contents core [minor]: StringOutputParser and text BaseMessage contents Mar 12, 2024
case "image_url":
return this._imageUrlContentToString(content);
default:
return "";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're throwing errors - should probably also throw an error here. (For any future content types that may be created. Marking an unknown content type.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes good call

@jacoblee93 jacoblee93 changed the title core [minor]: StringOutputParser and text BaseMessage contents core[minor]: StringOutputParser and text BaseMessage contents Mar 12, 2024
@jacoblee93
Copy link
Collaborator

Thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases close PRs that need one or two touch-ups to be ready size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StringOutputParser does not handle non-string BaseMessage content
2 participants