Add /feedback endpoint #74
Conversation
umago
force-pushed
the
feedback-endpoint
branch
3 times, most recently
from
360c063 to
3e73b95
Compare
This patch adds the /feedback endpoint. Note: Authentication and user ID retrieval haven't been included yet, as they don't appear to be implemented in the Lightspeed Llama-stack version. TODOs were left in the code for those parts. Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
|
yup we need this. Will take me some time to review, but I promise to do it |
|
@tisnik no rush! |
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
I just quickly went through the PR. But overall I think it looks good:)
|
|
||
| user_id = retrieve_user_id(auth) | ||
| try: | ||
| store_feedback(user_id, feedback_request.model_dump(exclude={"model_config"})) |
There was a problem hiding this comment.
thought: I'm thinking about whether it would make sense to set some upper limit on the length of the user's feedback. This would be to prevent resource exhaustion if some malicious party decides to test out the endpoint.
question: Is the plan to store the feedback later somewhere in a DB and not in a plaintext file?
There was a problem hiding this comment.
True, we can think of that as well. For now, I'm just mimicking what was present in the previous service.
Regarding the question, looking at the data collection [0], it does seem like it still sends it in plain-text. It's just zipped and sent to console.redhat.com. May be something we want to change too.
But I think these things can be added on top of this work. For now, keeping it compatible to what it was before seems fine IMHO.
There was a problem hiding this comment.
Yes, definitely I think this is a good start! 👍 I was just curious.
Thanks for the link, it makes it a bit clearer to me. I do not know much about the feedback part.
There was a problem hiding this comment.
basically yes, the plan is to use DB (probably sqlite as it's used a lot in llama-stack itself). The JSON as files solution was easier to implement in openshift scenario, but personally I don't like it very much TBH :)
There was a problem hiding this comment.
@lpiwowar +1 for having config to limit feedback. Would you like to create a ticket for it? Or if you don't want to, I can take it, whatever
There was a problem hiding this comment.
True, we can think of that as well. For now, I'm just mimicking what was present in the previous service.
Regarding the question, looking at the data collection [0], it does seem like it still sends it in plain-text. It's just zipped and sent to console.redhat.com. May be something we want to change too.
If the format would change, also the pipelines in RH network will need changes... But doable, of course ;)
There was a problem hiding this comment.
@tisnik I can take a look at the limiting of the feedback. I'll create a ticket:).
There was a problem hiding this comment.
Ticket created:), @umago it is on our board as well. It's a small ticket, but I'm just being transparent. I guess we are still figuring out how to best collaborate together.
If the format would change, also the pipelines in RH network will need changes... But doable, of course ;)
Yes:), I guess this change would be a bit bigger. I guess if it works, then it is fine for now (?). Maybe something we might want to take later?
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
It looks good, let's use it! Thank you
3 participants
Description
This patch adds the /feedback endpoint.
Note: Authentication and user ID retrieval haven't been included yet, as they don't appear to be implemented in the Lightspeed Llama-stack version. TODOs were left in the code for those parts.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing