Double remote call on DefaultChatClient with content and chatResponse #2098
Conversation
Signed-off-by: Lorenzo Caenazzo <[email protected]>
There was a problem hiding this comment.
Hey! Since I've been summoned here by @tzolov, here's my view: The approach using a ThreadLocal is not appropriate. I don't know what the solution should be (most probably an architectural decision or some usage guidelines), but I can say that ThreadLocal will cause issues, e.g. if the client is shared across Threads. Also, resubscribing to the Flux of ChatResponse is most probably not permitted and actually would probably end up exercising the LLM call twice as well. I do not see any tests here either validating the approach, I think that would be necessary to make progress, but best to tackle the question of the intended usage and design. Implementing caching in any way might as well be unexpected in most scenarios regardless if it relies on ThreadLocal or some other mechanism.
|
Ok fine, the ThreadLocal is not the way to acheve this, we will continue to discuss on Issue, maybe only a renaming is needed to avoid confusion. |
|
thoughts @chemicL ? |
|
@markpollack my view is that this PR should be closed. The actual issue or source of misunderstanding is addressed in #2097 (comment) |
4 participants
This should fix the issue #2097 .