langchain4j
langchain4j copied to clipboard
Add azure openai audio
Hi! I have started with the audio transcription and would like to get feedback to ensure I'm heading in the right direction, as this is my first PR for this project.
Issue
Closes #255
Change
Add Azure OpenAI
General checklist
- [X] There are no breaking changes
- [X] I have added unit and integration tests for my change
- [X] I have manually run all the unit and integration tests in the module I have added/changed, and they are all green
- [X] I have manually run all the unit and integration tests in the core and main modules, and they are all green
- [ ] I have added/updated the documentation
- [ ] I have added an example in the examples repo (only for "big" features)
- [ ] I have added/updated Spring Boot starter(s) (if applicable)
/cc @agoncal (azure), @jdubois (azure)
Oh, and one more thing: do we plan to fully support all response_format and timestamp_granularities?
@SandraAhlgrimm any help on this one ?
@SandraAhlgrimm any help on this one ?
thanks, I put a meeting in your calendar
When will this be completed?
Is this feature not included in subsequent releases? I think this feature is very useful with ASR
@LizeRaes @langchain4j @dliubarskyi any thoughts :) ?
@anunnakian sorry, thoughts on what exactly? Could you please elaborate?
@anunnakian sorry, thoughts on what exactly? Could you please elaborate?
Hi @dliubarskyi, I took care of all comments and change requests about this PR (@SandraAhlgrimm gave me access to her branch). Could you please review it 🙏 I'm ready to handle all your feedbacks quickly
Hi @jdubois and @agoncal, I am looking forward to your review
Spotless is modifying a lot of files that have nothing to do with this PR, I know it will make the CI/CD break but I wouldn't commit those here.
@jdubois, I took spotless fixes out as discussed and will file this in another PR.
It's ready for your review
Issue #3576 Pull Request #3577 for formatting
@anunnakian so sorry about the delay with the review!
@dliubarskyi thanks for your speedy review! I addressed your comments.
@SandraAhlgrimm I plan to do a release tomorrow, and this PR is a good candidate to be included. Would you be able to address the comments till tomorrow noon? If not, no worries, I can do it myself, just allow me to push to your repo
@SandraAhlgrimm please ping me once this will be ready for merge
@dliubarskyi ready!
@SandraAhlgrimm only a single concern left: https://github.com/langchain4j/langchain4j/pull/2110#discussion_r2300613557
@SandraAhlgrimm only a single concern left: #2110 (comment)
Well, we can remove the FileName, but it might cause issues with (Azure) OpenAI for some types and requests
@SandraAhlgrimm it just does not make sense as far as I understand. Audio should be created from binaryData or base64Data, not from the url. But Audio.getFilename() expects a url.
Since there is no way to pass the file name at the moment, I would remove the getFilename() method and always use "audio.mp3" as the file name in the Azure OpenAI request. WDYT?
@SandraAhlgrimm it just does not make sense as far as I understand.
Audioshould be created frombinaryDataorbase64Data, not from theurl. ButAudio.getFilename()expects aurl.Since there is no way to pass the file name at the moment, I would remove the
getFilename()method and always use "audio.mp3" as the file name in the Azure OpenAI request. WDYT?
ok, I am with you! Thanks for explaining extra clear!
Oh, and one more thing: do we plan to fully support all
response_formatandtimestamp_granularities?
I put this here to finally get the current one merged first :D https://github.com/langchain4j/langchain4j/issues/3604