Skip to content
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

bugfix: chunks get inserted in completion_first manner when completion_first = true #2245

Conversation

cdxker
Copy link
Member

@cdxker cdxker commented Aug 24, 2024

No description provided.

@cdxker cdxker marked this pull request as draft August 24, 2024 03:42
Arbiter::new().spawn(async move {
let chunk_v: Vec<String> = r.iter().collect();
let completion = chunk_v.join("");

let message_to_be_stored = if completion_first {
format!("{}{}", completion, chunk_metadatas_stringified)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes the most sense to store the chunks in the completion order the user wanted them to be streamed in. (Will likely work a lot better for the front ends to be modular.

@cdxker cdxker force-pushed the fa/bugfix-insert-chunks-in-correct-order-for-completion_first branch from 6da64b2 to adfd289 Compare August 24, 2024 04:05
.into_iter()
.filter_map(|mut message| {
if message.content.starts_with("||[{") {
match message.content.rsplit_once("}]") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the format string }} = a single }. Not sure why they don't just do a } but yeah.

Comment on lines 500 to 516
if message.content.starts_with("{[") {
// This is (chunks, content)
message.content = message
.content
.split("||")
.last()
.unwrap_or("I give up, I can't find a citation")
.to_string();
} else {
// This is (content, chunks)
message.content = message
.content
.rsplit("||")
.last()
.unwrap_or("I give up, I can't find a citation")
.to_string();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should with a strange combo of completion_first = true / false

@cdxker cdxker force-pushed the fa/bugfix-insert-chunks-in-correct-order-for-completion_first branch 2 times, most recently from bcfc050 to c0cf518 Compare August 24, 2024 04:30
@cdxker cdxker force-pushed the fa/bugfix-insert-chunks-in-correct-order-for-completion_first branch from c0cf518 to 2981cf2 Compare August 24, 2024 04:43
@cdxker cdxker marked this pull request as ready for review August 24, 2024 04:44
@cdxker cdxker force-pushed the fa/bugfix-insert-chunks-in-correct-order-for-completion_first branch from 2981cf2 to b4b4a95 Compare August 24, 2024 04:44
Copy link
Contributor

@skeptrunedev skeptrunedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Tested by cherry-pick'ing the frontend commit and duplicating the error then validating that this fixes it.

@skeptrunedev skeptrunedev force-pushed the fa/bugfix-insert-chunks-in-correct-order-for-completion_first branch from 03e74fb to 037247c Compare August 24, 2024 05:26
@skeptrunedev skeptrunedev self-requested a review August 24, 2024 05:27
@skeptrunedev skeptrunedev merged commit 6bdc134 into main Aug 24, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants