Add method(stream_parse, ProviderPerplexity)
#380
Conversation
| ) | ||
|
|
||
| method(stream_parse, ProviderPerplexity) <- function(provider, event) { | ||
| if (is.null(event) || identical(event$data, "[DONE]")) { |
There was a problem hiding this comment.
I do wonder if we should just make this change in the ProviderOpenAI method, since it seems to affect a lot of backends.
There was a problem hiding this comment.
Tough call! I guess the data: [DONE] event makes sense for servers that wouldn't stop listening unless they get that specific event. The OpenAI "/chat/completions" endpoint provides the stop parameter that might have use for it
There was a problem hiding this comment.
I assume the python openAI SDK must look for either "[DONE]" or (the python equivalent of) NULL since otherwise these providers wouldn't claim to be openAI compatible.
There was a problem hiding this comment.
That seems to be accurate: https://github.com/openai/openai-python/blob/f66d2e6fdc51c4528c99bb25a8fbca6f9b9b872d/src/openai/_streaming.py#L52-L101
I wasn't aware of how much error handling can be added to the process.
Does this mean that it makes more sense to make the change in ProviderOpenAI?
There was a problem hiding this comment.
Yeah, I think so. And then remove any other custom methods where we've done this before. It would also be useful to add that link as a comment. (Thanks for finding it!)
There was a problem hiding this comment.
All right, I can do that. However, I don't think I'll be able to until the weekend. May I suggest to merge this PR (since it fixes the issue in-scope) and create a new issue for "Stop generating error when event data is NULL in method(stream_parse, ProviderOpenAI)"?
There was a problem hiding this comment.
I'd rather not add code that we then have to remove soon after. But I'm happy to do the fixes and should (🤞) be able to get to it on Thursday or Friday.
Change description:
stream_parse()method.method(stream_parse, ellmer::ProviderOpenAI)causes error in succesful calls to Perplexity API #379I noticed that there is no "tests/testthat/test-provider-perplexity.R" file. Would that be a requirement to approve this PR?