-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(anthropic): add support for container upload block type in message formatting #9097
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
Conversation
🦋 Changeset detectedLatest commit: 834e5a0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@pokey is attempting to deploy a commit to the LangChain Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
} else { | ||
console.warn( | ||
`Skipping unsupported content block of type "${contentPart.type}".` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One big gotcha here for me was that we silently drop unknown blocks. I turned it into a warning, but my instinct would be to warn and then just pass them through untouched. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would throw here because it means we not fully support Anthropic's API which causes such unknown errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One consideration is that message .content looks subtly different between providers, which means if I used openai generated messages in an anthropic call and had the content fall through we'd always get 400 errors. I also don't think it's great DX to flood the console with these warning messages if we know that some messages aren't compatible with anthropic to boot.
Generally when serializing provider requests, we take the approach of "best effort" parsing where we serialize messages for a request to the best of our ability (typically by having explicit conversion for messages that come out of the same provider + some other well known formats), but we don't explicitly fail since we can't interpret every possible message format (n^n complexity)
this is part of the motivation for standard content in v1!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That makes sense, though today's behaviour of just dropping the messages is a bit surprising. I wonder if we should coerce it into a text block or something. Or have a flag to govern behaviour, because it would be useful during dev to fail fast but then be robust in prod
Anyways I'll remove the warning from this PR as it sounds like there's a bigger conversation to be had here. Shall I open a github discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've replaced the warning with a comment explaining why we silently drop blocks ofa unexpected type, and added a test to capture that behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor for throwing an error but wouldn't consider this blocking.
Thanks 👍
} else { | ||
console.warn( | ||
`Skipping unsupported content block of type "${contentPart.type}".` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would throw here because it means we not fully support Anthropic's API which causes such unknown errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for catching this! A few things:
- an integration test would be much appreciated 🙏
- I detailed this a bit in the comments, but we should remove the warning
} else { | ||
console.warn( | ||
`Skipping unsupported content block of type "${contentPart.type}".` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One consideration is that message .content looks subtly different between providers, which means if I used openai generated messages in an anthropic call and had the content fall through we'd always get 400 errors. I also don't think it's great DX to flood the console with these warning messages if we know that some messages aren't compatible with anthropic to boot.
Generally when serializing provider requests, we take the approach of "best effort" parsing where we serialize messages for a request to the best of our ability (typically by having explicit conversion for messages that come out of the same provider + some other well known formats), but we don't explicitly fail since we can't interpret every possible message format (n^n complexity)
this is part of the motivation for standard content in v1!
Integration test added and warning removed; should be good to merge |
…pload-block-type-in-message-formatting
Any idea why CI is failing? Doesn't look related to my change but can't say for certain |
yep I definitely broke it |
…pload-block-type-in-message-formatting
…pload-block-type-in-message-formatting
We currently silently drop all content blocks of type
"container_upload"
, which are used to make files accessible from Anthropic beta code execution containers. This PR adds support for themSee Anthropic code execution docs for more info