-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Message drafts #3044
Message drafts #3044
Conversation
❌ pre-commit failed. |
2 similar comments
❌ pre-commit failed. |
❌ pre-commit failed. |
❌ pre-commit failed. |
❌ pre-commit failed. |
inference/server/alembic/versions/2023_05_07_0923-19178d144265_add_sibling_active_to_message.py
Outdated
Show resolved
Hide resolved
@@ -19,6 +19,7 @@ class DbMessage(SQLModel, table=True): | |||
reports: list["DbReport"] = Relationship(back_populates="message") | |||
|
|||
parent_id: str | None = Field(None) | |||
active_sibling: bool | None = Field(None, sa_column=sa.Column(pg.JSONB)) |
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 does active_sibling
mean exactly? Feels like an odd name
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.
In a conversation tree you can have messages which you can switch between using the arrows at the bottom. This variable just stores whether or not that particular message was the one which was the last showed thread. This way when a user makes a long thread it won't 'disappear' when they load the conversation again. The 'sibling' part is referring to how these messages come from the same parent message so I thought it would be a good term to use. If you have a better alternative I can change it.
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.
Hmm ok. This feels like it is really information about the chat, not the message. Can we not just store a single active_message
value in the chat table, which is the final message of the currently active path down the chat tree? Then from this it is already implied what siblings are active earlier in the tree
This would also simplify the endpoint, we could merge it into update_chat
endpoint
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.
Sorry only seeing this now: How are the multiple generated draft messages now referenced in the database? And was is finally stored in the chat-message tree?
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 draft messages are stored as regular messages in the messages
table. The message comparisons are stored in a new table called message_eval
which contains the chat id, user id, message id of the selected message, and the message ids it was selected over.
I’ve renamed the active_message_id property to active_thread_tail_message_id to make it more clear that it’s purpose is storing the last viewed thread and not any sort of message comparison.
Thanks, I think this mostly makes sense from inference backend perspective, I left a couple of questions and one change request |
❌ pre-commit failed. |
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.
Thanks! This seems fine to me from backend perspective now. Will approve tomorrow if neither of Yannic/Andreas wish to review first
inference/server/oasst_inference_server/user_chat_repository.py
Outdated
Show resolved
Hide resolved
❌ pre-commit failed. |
Co-authored-by: notmd <[email protected]>
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.
Thank you, need 1 more approval from Yannic or Andreas.
...ver/alembic/versions/2023_05_18_1748-2d67fbdc5b46_add_active_messsage_id_and_message_eval.py
Outdated
Show resolved
Hide resolved
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.
Thanks a lot for this important feature.
I would appreciate feedback to the following points:
-
It seems as if the draft generation would always be active when
NUM_GENERATED_DRAFTS > 1
. This will completely break any regular chat-experience and force the user to go through the message-selection process, even for simple "Hello" prompts. In general we definitely want to collect this form of feedback data but the question is if we really should make regular use of assistant impossible. If message alternatives would be generated when user presses regenerate or down-vote it would IMO be a much more unobtrusive experience and users would get additional option when they actually need them. -
It is not clear to me how draft messages are tracked now in the database. Could you please explain the meaning the
active_message_id
column inchat
table (and why is it a json field)? How are drafts messages stored and referenced in the database?
|
❌ pre-commit failed. |
❌ pre-commit failed. |
1 similar comment
❌ pre-commit failed. |
❌ pre-commit failed. |
closes #2931 (slightly changed goal based on advice from the discord, generate full messages, not 'x' tokens. Full messages are more useful data)
[ ] Disable drafts when queue is too long / server is under load(Suggested to leave to next PR in the discord)