The Wayback Machine - https://web.archive.org/web/20230319142805/https://github.com/BloopAI/bloop/pull/262
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

Conversational Interface #262

Merged
merged 50 commits into from Mar 17, 2023
Merged

Conversational Interface #262

merged 50 commits into from Mar 17, 2023

Conversation

ggordonhall
Copy link
Contributor

@ggordonhall ggordonhall commented Mar 17, 2023

This adds support for a chat interface. With this bloop supports follow up queries which reference previous questions and answers in the conversational history. These are rendered in a chat window on the right-hand-side of the search results.

This bundles a host of changes, including:

  • Rake keyword extraction for search queries
  • Change of provider to OpenAI
  • Rewritten prompts which support OpenAI's 'message' based prompt-format

The abstractions in the answer.rs module will be re-worked in a future PR.

calyptobai and others added 30 commits March 13, 2023 15:55
For simplicity of merging in the changes from `main`, due to heavy
conflicts, this squashes all back-end commits down.

* conversational flow, take 2
* update prompts and history
* wip
* Clean up conversational logic, fix several bugs
* Fix field name in Answer API request
* Fix termination sequence
* Fix state machine logic
* Tie selected snippet to explanation prompt, per iteration
* Run cargo fmt
* Fix clippy lints
* Send snippets along during explanation phase

Co-authored-by: Andre Bogus <andre@bloop.ai>
This allows multiple independent conversations per user.
Copy link
Collaborator

@rsdy rsdy left a comment

Choose a reason for hiding this comment

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

I think there are serious code improvements that should be made before we can review this in earnest, otherwise as an outsider this will be very hard to pick up.

/// This gets the prior conversation. Be sure to drop the borrow before calling
/// [`add_conversation_entry`], lest we deadlock.
pub fn with_prior_conversation<T>(
&self,
user_id: &str,
f: impl Fn(&[(String, String)]) -> T,
) -> T {
self.prior_conversational_store
.get(user_id)
.map(|r| f(&r.value()[..]))
.unwrap_or_else(|| f(&[]))
}

/// add a new conversation entry to the store
pub fn add_conversation_entry(&self, user_id: String, query: String) {
match self.prior_conversational_store.entry(user_id) {
Entry::Occupied(mut o) => o.get_mut().push((query, String::new())),
Entry::Vacant(v) => {
v.insert(vec![(query, String::new())]);
}
}
}

/// extend the last answer for the session by the given fragment
pub fn extend_conversation_answer(&self, user_id: String, fragment: &str) {
if let Entry::Occupied(mut o) = self.prior_conversational_store.entry(user_id) {
if let Some((_, ref mut answer)) = o.get_mut().last_mut() {
answer.push_str(fragment);
} else {
error!("No answer to add {fragment} to");
}
} else {
error!("We should not answer if there is no question. Fragment {fragment}");
}
}

/// clear the conversation history for a user
pub fn purge_prior_conversation(&self, user_id: &str) {
self.prior_conversational_store.remove(user_id);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of leaving these in Application, I'd strongly prefer a dedicated component that wraps and contains this functionality. These are very low-level functions that do not logically belong in a high-level component such as Application.

/// This gets the prior conversation. Be sure to drop the borrow before calling
/// [`add_conversation_entry`], lest we deadlock.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit of a foot-gun. Can we instead use an Arc<Vec<Arc<(String, String)>>> here, which would make access to members of DashMap safe? You can then update the record with the result from make_mut: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.make_mut

@@ -122,6 +123,8 @@ impl Application {
env
};

let prior_conversational_store = Arc::new(DashMap::new());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Arc::default() would work just as well, but as mentioned below, it would be good to wrap this in an struct.

@@ -1,5 +1,5 @@
use std::{
Copy link
Collaborator

Choose a reason for hiding this comment

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

The size of this file is making it very hard to decipher what's exactly happening here.
It would be really helpful to move most of this:

  1. out of the webserver
  2. into smaller modules
  3. keep a super high-level, easy-to-review function call flow in the webserver. (1 small function )
break grown_text;
.push(Stage::new("start", &query).with_time(stop_watch.lap()));

loop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an unbounded loop inside a very long function, which make this logic inherently hard to reason about. It would be good to either make this shorter, or change this to loop to something with explicit bounds.

Copy link
Contributor

@calyptobai calyptobai left a comment

Choose a reason for hiding this comment

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

@ggordonhall ggordonhall merged commit ef5af19 into main Mar 17, 2023
16 of 18 checks passed
@ggordonhall ggordonhall deleted the chat-style-prompts branch March 17, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants