-
Notifications
You must be signed in to change notification settings - Fork 817
Fall back on guessed MIME type when requested multi-modal content doesn't have Content-Type header #1613
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?
Conversation
I did see difference in how bed rock is handling audio, video etc compared to that of other models. |
@DouweM Thoughts here? |
@kylesf Thanks! We'll want to address the issue of |
idk lot of things going on. guessing type is not ideal if we have fixed supported types. i.e. VideoMediaType - at the same time we have a test case like image_url = ImageUrl(url='https://goo.gle/instrument-img') where we need to do web req to get our type. prob needs more thinking. |
i do think this change allows better support as if we do req and that info is not given up, then we can use the extension. |
@DouweM thoughts? |
@kylesf Falling back on |
@DouweM got them all I believe |
@kylesf Thank you! The code looks good, but CI is complaining about coverage. To prevent having to add a test of this for each model, I suggest moving this logic to a helper function, and then adding a single test for this scenario that passes through that function. |
@DouweM - in my attempts to do this, I have not been happy with the resulting code, more complex and not as simple to implement. I will probably leave it here, for the team to decide what to do. I will just monkey patch on my side for now. Thanks! |
@Kludex I needed to add during testing of agents I built. I am not sure why tests did not catch the issue and whats the best long term approach for the project. Please share your thoughts.