-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
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 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.
# remove the audio blocking check | ||
# if self.audios not in [None, []]: | ||
# raise ValueError("Audio is not yet supported") |
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.
If you remove a block of code please don't leave it as a comment. This will help maintain the codebase clean.
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.
my bad, I will not leave it as a comment. I will remove it in the next pr
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. |
Co-authored-by: Kajetan Rachwał <[email protected]>
alright, I'll create a pr in with the updated tests |
Closing in favour of #382 |
Purpose
Proposed Changes
Issues