DRAFT: Feat/poc OpenAI realtime api#935
Conversation
|
|
||
| @Slf4j | ||
| @RequiredArgsConstructor(access = AccessLevel.PACKAGE) | ||
| public class RealtimeChannelFactory { |
There was a problem hiding this comment.
Why did you make.a factory?
There was a problem hiding this comment.
That is one of the things I wanted to discuss. Current http - destination functionality does not support WSS. I had to tinker endpoint resolution and authorization logic in a way, which would be minimally disruptive for the current tooling.
Doing "all at once" seem to be a big change, I wanted to avoid huge PRs and deliver things gradually.
At the same time, we need a longer term approach, which I wanted to discuss (factory, modules, destination functionality extension, etc.)
| @@ -0,0 +1,54 @@ | |||
| package com.sap.ai.sdk.orchestration; | |||
There was a problem hiding this comment.
Why did you put it in the orchestration module and not the OpenAI module, or a new module?
There was a problem hiding this comment.
that's basically the main purpose of this draft PR, to discuss where/how it is better to put in our SDK. what would be your preference/advice?
There was a problem hiding this comment.
I would expect a solution similar to the previous PR in terms of structure, public API and tests. This would also answer your comment.
| .type(RealtimeConversationItemUserMessage.Content.Type.INPUT_TEXT) | ||
| .build()) | ||
| .build())) | ||
| .build(); |
There was a problem hiding this comment.
Should we hide those model classes from the user?
| var base64Audio = event.get("delta").asText(); | ||
| byte[] audio = Base64.getDecoder().decode(base64Audio); | ||
| this.outputConsumer.accept(audio, Boolean.FALSE); | ||
| log.warn("Called consumer with base64 converted to bytes: {} (base64)", base64Audio); |
There was a problem hiding this comment.
Be careful, logs with private data is are not allowed, also valid for other lines
There was a problem hiding this comment.
Thanks for pointing that out! Yes, this draft PR still contains debugging logs, I'll fully clean it up
| * mechanism is required to prevent unintended closing of connections while conversation is still | ||
| * expected to continue | ||
| */ | ||
| private static final int HEARTBEAT_INTERVAL_MILLIS = 4500; |
There was a problem hiding this comment.
Why would the conversation continue when the connection is idle?
Does the connection ever close after using the client?
There was a problem hiding this comment.
- This is a typical (main) use case for realtime api. Client sends text data and receives audio messages. While playing the received data, client does not make any input to the web socket, but still may continue sending data after the playback is finished. In this case, client should not run into issues with a dead (already closed) connection or start expensive multi-hop connection configuration process again
- Yes, implementation I provided implements
AutoCloseableand closes all managed resources when it is called
| SpringApplication.run(Application.class, args); | ||
| public static void main(final String[] args) throws Exception { | ||
| var ctx = SpringApplication.run(Application.class, args); | ||
| var svc = ctx.getBean(OrchestrationService.class); |
There was a problem hiding this comment.
ctx and svc are not clear variable names
There was a problem hiding this comment.
this is a "quick and dirty" showcase for the draft pr to align on the architecture / approach. I'm going to rework it and make properly in the final PR
| */ | ||
| public static void main(final String[] args) { | ||
| SpringApplication.run(Application.class, args); | ||
| public static void main(final String[] args) throws Exception { |
There was a problem hiding this comment.
Why hijack the main when you could make an endpoint and test
There was a problem hiding this comment.
this is a "quick and dirty" showcase for the draft pr to align on the architecture / approach. I'm going to rework it and make properly in the final PR
| while (!done.get() && System.currentTimeMillis() < timeout) { | ||
| Thread.yield(); | ||
| System.err.println("Waiting for messages..."); | ||
| Thread.sleep(500); |
There was a problem hiding this comment.
Is there no cleaner way than block the main thread
There was a problem hiding this comment.
this is a "quick and dirty" showcase for the draft pr to align on the architecture / approach. I'm going to rework it and make properly in the final PR
| <dependency> | ||
| <groupId>com.openai</groupId> | ||
| <artifactId>openai-java</artifactId> | ||
| <version>4.41.0</version> |
There was a problem hiding this comment.
We put version numbers in the properties
| * current response) | ||
| * @return text input channel to supply text to voice, should be closed when not needed anymore | ||
| */ | ||
| public TextInputChannel textToSpeech(BiConsumer<byte[], Boolean> output) { |
There was a problem hiding this comment.
IS there no better way than an unclear boolean that is only explained in 1 javadoc comment.
|
|
||
| @Slf4j | ||
| public class TextToSpeechRealtimeClient extends WSSOpenAIRealtimeClient | ||
| implements TextInputChannel { |
There was a problem hiding this comment.
Why do we have multiple classes / factories encapsulating each other when they have the same purpose and can't work without the other one. Does it follow SOLID principles?
There was a problem hiding this comment.
@CharlesDuboisSAP, For multiple reasons:
WSSOpenAIRealtimeClientimplements common functionality for WebSocket configuration and management for all OpenAI realtime clients. It is quite non trivial functionality and giving it separate scope from code business/domain level operations should reduce code cognitive load and organize code in a way that simplifies maintenanceTextToSpeechRealtimeClientimplements functionality, specific to text to speech operation. There will be multiple different clients (e.g.SpeechToTextandSpeechToSpeech). Data types are not the same, underlaying websocket is configured with different modalities between them and sharing the same socket for multiple flows is highly problematic if not practically impossible- The factory is definitely not on its intended way. One of the main reasons I made this draft PR is to discuss possible options to integrate this functionality into the existing SDK (e.g. all our current tooling is designed for http and I had to tinker to make this new functionality work with WSS and without significant changes in the existing codebase). @CharlesDuboisSAP , do you have a proposal on the possible new module, separate API, or other place, where, you think, it should be?
- Designing this technical solution I tried to balance multiple factors, to name a few: correctness, simplicity, adaptivity, ease of maintenance, code readability. On the code level, yes, in my view there is no compromises against SOLID principles, please let me know if you think otherwise and what in your opinion is against, if so
My current understanding is that the mentioned PR cannot be completed to a SDK realtime solution. Here is the context and outcomes of my research why I think so: There are three ways to implement realtime API with OpenAPI:
The previous PR implements SIP - it is a way for client and server to communicate about a connection in order to pass data, but not to establish connection or pass data in fact. The data layer should be implemented separately from the SIP protocol. OpenAPI SIP protocol by design works with client-side web hooks as a mean to pass data and accept incoming connections (calls). Outgoing calls (client initiated) are not seem to be possible at all using solely SIP API. We cannot expect SDK environment to have public http endpoints accessible from the internet to expose hooks. AI Core does not implement hook functionality either AFAIK. Given that WebRTC is difficult to support on the BE and, AFAIK, our backend only supports Websockets at the moment, Websockets seem to be the only option we have. |
Context
AI/ai-sdk-java-backlog#ISSUENUMBER.
Please provide a short description of what your change does and why it is needed.
Feature scope:
Definition of Done
Aligned changes with the JavaScript SDK