-
Notifications
You must be signed in to change notification settings - Fork 83
Conversation
6ddd05b
to
c4bfb06
Compare
c4bfb06
to
cf8e00e
Compare
"temperature": 0, | ||
} | ||
|
||
result = await self.inference_engine.chat( |
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.
what do you think about wrapping this in a try/except and returning an empty list and logging an error? In general our exception handling is not great (unrelated to this PR of course) and I was wondering if it would make sense to mark pipeline steps as critical or nice-to-have and handle exceptions in the pipeline processor rather than having to handle them in the steps themselves.
That would be outside the scope of this patch, for this one I just wonder about wrapping the chat in try/except and returning [] in case of an exception.
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.
Oh and since we were talking on slack about performance of the local vs remote model, just noting here that the local LLM takes anywhere between 1.5 - 4 seconds on my laptop. I will also measure the hosted LLMs for the same task.
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.
I added some comments but they are non-blocking. I think we should tune the prompt further to avoid the bad links and discuss if we want to inject the security-focused prompt always. I will also file an issue to think about handling exceptions in pipeline steps.
No description provided.