-
Notifications
You must be signed in to change notification settings - Fork 40
FEATURE: new endpoint for directly accessing a persona #876
Conversation
The new `/admin/plugins/discourse-ai/ai-personas/stream-reply.json` was added. This endpoint streams data direct from a persona and can be used to access a persona from remote systems leaving a paper trail in PMs about the conversation that happened This endpoint is only accessible to admins.
|
Example usage: |
keegangeorge
left a comment
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.
LGTM overall, just a few small suggestions
Co-authored-by: Gabriel Grubba <[email protected]>
Co-authored-by: Keegan George <[email protected]>
invalid_prompt_role is not used anywhere.
martin-brennan
left a comment
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.
Not sure if further refactors are planned here. I get that we are trying to move fast, but I think there is time to go back and improve things as well.
|
|
||
| CRLF = "\r\n" | ||
|
|
||
| def stream_reply |
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 see Keegan mentioned a similar thing, but this is an extremely fat controller action, I really don't feel like we should be doing this very often, if ever. It doesn't feel very maintainable or easy to read, and could easily expand even further in size. Much of this is a perfect case for using a service.
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.
The reasons why it's perfect for a service:
- It has model lookups
- It has several parameter validations and error messages
- It has a clear action and result from that action, which could also have more errors
Not saying the reply streaming stuff should be in one, but everything before it easily could be
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 have been waiting a bit on service docs and service stability, stuff has been changing recently
| end | ||
| end | ||
|
|
||
| def stage_user |
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.
Another thing that could be in a service, or at least a reusable class in core. Nothing except the custom field is AI-specific here
| def self.queue_streamed_reply(io, persona, user, topic, post) | ||
| schedule_block do | ||
| begin | ||
| io.write "HTTP/1.1 200 OK" |
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 guess it's for a good reason (performance?), but is there any way not to do this writing of headers and data completely manually?
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.
rails has streaming, but I don't want to stream from the foreground thread, I can make wrapper classes for this but at the end the same code will run
| end | ||
| end | ||
|
|
||
| class << self |
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.
Feels like stuff that should be generic/in core/an ActiveSupport::Concern that we could use elsewhere too?
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 will move it out to a service at some point definitely do not want a concern here , this is a very sharp knife I only want people that absolutely understand repercussions to use it.
The new
/admin/plugins/discourse-ai/ai-personas/stream-reply.jsonwas added.This endpoint streams data direct from a persona and can be used
to access a persona from remote systems leaving a paper trail in
PMs about the conversation that happened
This endpoint is only accessible to admins.