-
Notifications
You must be signed in to change notification settings - Fork 61
Review attachment support in the AI Proxy #371
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
base: main
Are you sure you want to change the base?
Review attachment support in the AI Proxy #371
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| matrix: | ||
| node-version: | ||
| - 20 | ||
| steps: |
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.
confused me not seeing a specific test job in the ci
| { | ||
| type: "file", | ||
| file: { | ||
| file_data: MD_DATA_URL, |
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.
@annalinwang i didn't go further in trying to convert text to pdf. i had a spike but then i fell short because of unicode support. going to assume we want to wait for the rust gateway to convert user's text to pdf kind of like i did here: https://braintrustdata.slack.com/archives/C08DRGSEMS8/p1768262457722489?thread_ts=1767909967.121049&cid=C08DRGSEMS8
| OpenAIChatCompletion | ||
| >({ | ||
| body: { | ||
| model: "gpt-audio", |
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.
@annalinwang i'm going to skip this, but let me know if you know of customers that want this. small lift, if there's a need.
…rt-in-the-ai-proxy
| @@ -0,0 +1,21 @@ | |||
| import { readFileSync } from "fs"; | |||
|
|
|||
| const fileToBase64 = (filename: string, mimetype: string) => | |||
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.
this avoids spamming the LLM with all the base64 encoded strings
| }, | ||
| "dependencies": { | ||
| "@anthropic-ai/sdk": "^0.39.0", | ||
| "@anthropic-ai/sdk": "^0.71.2", |
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.
Earlier versions didn't have plain text content parts.
| - name: Get pnpm store directory | ||
| shell: bash | ||
| run: echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV | ||
| - uses: actions/cache@v4 |
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.
nice cache
| ); | ||
| }); | ||
|
|
||
| it("should return error for markdown file content (unsupported)", async () => { |
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.
random but how come anthropic accepts markdown in a plain text format? openai doesnt?
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.
it is odd, but that's just how the openai spec/apis are. i've been meaning to test sending a markdown file to their files api and seeing if passing that id ref works (to get around the limitation), but it is a lot more complex even if it does
| expect(isMediaTypeSupported("image/jpeg", "openai")).toBe(false); | ||
| expect(isMediaTypeSupported("image/heic", "openai")).toBe(true); | ||
| expect(isMediaTypeSupported("image/heif", "openai")).toBe(true); |
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.
is this reversed? should it be true for jpeg and false for heic/heif?
| (type) => type.endsWith("heic") || type.endsWith("heif"), | ||
| ), | ||
| ), | ||
| ...toMediaTypeSupport(DOCUMENT_MEDIA_TYPES), | ||
| }, | ||
| anthropic: { | ||
| ...toMediaTypeSupport( | ||
| IMAGE_MEDIA_TYPES.filter( | ||
| (type) => type.endsWith("heic") || type.endsWith("heif"), |
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.
are these filters reversed?
isMediaTypeSupportedand other helpful exports https://github.com/braintrustdata/braintrust-proxy/pull/371/changes#diff-5ba8cdd959c987a01900df254e1b49c14eee68875449664cae80b02eca11b738R29-R36Also attempted to convert a plain file to pdf at runtime, but it would require an extra dependency. Didn't want to go that far in this round.