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

feat: add support for base64 encoded audio files #380

Closed

Conversation

mdimado
Copy link

@mdimado mdimado commented Jan 21, 2025

Purpose

  • Add support for base64 encoded files

Proposed Changes

  • remove audio blocking check
  • add base64 audio file handling in MultimodalMessage
  • add audio content entries with audio/wav mime type

Issues

Copy link
Contributor

@rachwalk rachwalk left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, I will approve the changes once the tests pass and you address the comments I made. Also please git rebase onto development, so the newest features are included. Do you think you'd be interested in working on #374? Especially the 2. point - ensuring that LLMs can accept this audio format. For now it would be okay, if only .wav is supported, the audio conversion functions can be done later.

Comment on lines +38 to +40
# remove the audio blocking check
# if self.audios not in [None, []]:
# raise ValueError("Audio is not yet supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove a block of code please don't leave it as a comment. This will help maintain the codebase clean.

Copy link
Author

Choose a reason for hiding this comment

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

my bad, I will not leave it as a comment. I will remove it in the next pr

src/rai/rai/messages/multimodal.py Outdated Show resolved Hide resolved
@rachwalk
Copy link
Contributor

It also seems that there were tests which checked for audio not being supported. Since your PR adds support for audio it would be beneficial if you updated these tests too.

@mdimado
Copy link
Author

mdimado commented Jan 22, 2025

alright, I'll create a pr in with the updated tests

@rachwalk
Copy link
Contributor

@mdimado Can I close this PR in favour of #382?

@mdimado
Copy link
Author

mdimado commented Jan 24, 2025

@mdimado Can I close this PR in favour of #382?

Sure

@rachwalk
Copy link
Contributor

Closing in favour of #382

@rachwalk rachwalk closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ensure api complience with audio for multimodal messages
2 participants