Support multiple MoFa agents and servers #325

joulei · 2024-12-11T07:24:05Z

Changes

This PR adds support for multiple MoFa severs and multiple agents.
I've extended the settings UI to support adding servers at runtime, and testing the connection.

Limitations

Currently we don't support editing a server address, this is because modifying a server address while in use adds a lot of complexity (some kind of mutex around the address, agent list diff reconciliation, agent id reconciliation, etc.). We could explore this in the future.
There are some minor improvements we can tackle separately:
- Add confirmation dialog when deleting a server
- Supporting server shutdown and turn on - we could keep the servers on the list but turn them on/off on demand
- Periodically check for server connection health and agent list refresh
Most MoFa server don't yet support sending an actual list of agents on the v1/models endpoint, therefore we're still hardcoding a Reasoner agent.

Screen recordings

Adding a new server and talking to an agent:

Screen.Recording.2024-12-13.at.4.24.40.PM.mov

Handling of no agents, disconnected servers or no servers:

Screen.Recording.Dec.13.2024.mp4

src/data/chats/mod.rs

src/data/chats/chat.rs

src/data/chats/mod.rs

noxware

I have to pause for a moment, I will finish the review when back

moly-mofa/src/lib.rs

noxware · 2024-12-16T17:34:09Z

moly-mofa/src/lib.rs

+impl Default for MofaAgent {
+    /// Returns a dummy agent whenever the corresponding Agent cannot be found
+    /// (due to the server not being available, the server no longer providing the agent, etc.).
+    fn default() -> Self {
+        MofaAgent {
+            id: AgentId("default".to_string()),
+            name: "Inaccesible Agent".to_string(),
+            description: "Default".to_string(),
+            agent_type: AgentType::Reasoner,
+            server_id: MofaServerId("default".to_string()),
+        }
+    }


Wouldn't be more idiomatic and explicit to just deal with Option<MofaAgent> when necessary?

noxware · 2024-12-16T17:46:14Z

moly-mofa/src/lib.rs

-        vec![
-            MofaAgent::Example,
-            // Keeping only one agent for now. We will revisit this later when MoFa is more stable.
+    pub fn fetch_agents(&self, tx: mpsc::Sender<MofaServerResponse>) {


nitpick: add Sender to the use statement we already have and shorten all the mpsc::Sender

noxware · 2024-12-16T17:51:16Z

moly-mofa/src/lib.rs

+    /// 
+    /// This function runs in a separate thread and processes commands received through the command channel.
+    /// 
+    /// The loop continues until the command channel is closed or an unrecoverable error occurs.


Well, since we are doing .recv().unwrap(), the channel being closed is also an unrecoverable error.

If we want the thread to finish gracefully, we would need to do something like

while let Some(message) = command_receiver.recv() { ... }

instead of the loop with unwrap().

noxware · 2024-12-16T18:15:46Z

moly-mofa/src/lib.rs

+
+                    let address_clone = address.clone();
                    current_request = Some(rt.spawn(async move {
                        let resp = client
-                            .post(format!("{}/v1/chat/completions", url))
+                            .post(format!("{}/v1/chat/completions", address_clone))


You don't need to move address itself inside the coroutine. The async block is needed to run the async task, but the request can be defined outside and moved later. Making this clone unnecessary with almost no changes.

let req = client .post(format!("{}/v1/chat/completions", &address)) .json(&data); current_request = Some(rt.spawn(async move { let resp = req.send().await.expect("Failed to send request"); // everything else stays the same... }));

noxware · 2024-12-16T18:48:38Z

src/chat/chat_history.rs


        enum Item<'a> {
            ChatsHeader,
            AgentsHeader,
+            NoAgentsWarning(&'a str),


maybe irrelevant note: I don't know if it is more idiomatic, but as long as this is not dynamic, this can be 'static, which may be a more informative lifetime, as you could quickly know by reading this line that messages are pre-defined and never constructed at runtime.

noxware · 2024-12-16T19:01:18Z

src/chat/chat_panel.rs

+                            .available_agents
+                            .get(&agent_id)
+                            .cloned()
+                            .unwrap_or_default();


mmm i think it may be better to ensure we can't start a chat with an unrecognized agent, and panic or eprintln something here to indicate a dev mistake

noxware · 2024-12-16T19:06:24Z

src/chat/chat_history_card.rs

+                    .chats.available_agents
+                    .get(&agent_id)
+                    .cloned()
+                    .unwrap_or_default();


I would divide this flow to handle the case of agent missing, or at least, if you really want to do this, I would make a constructor MofaAgent::unknown() or something similar instead of using the default as that

choose your pill

noxware · 2024-12-16T19:13:56Z

src/chat/entity_button.rs

@@ -135,7 +154,7 @@ impl EntityButton {
    }

    pub fn set_agent(&mut self, agent: &MofaAgent) {
-        self.set_entity(ChatEntityRef::Agent(agent));
+        self.set_entity(ChatEntityRef::Agent(&agent));


agent is already of type &MofaAgent so the added & is not necessary

noxware · 2024-12-16T19:27:58Z

src/chat/model_selector_item.rs

+    pub fn set_agent(&mut self, agent: &MofaAgent) {
        let Some(mut inner) = self.borrow_mut() else { return };
-        inner.entity = ModelSelectorEntity::Agent(agent);
+        inner.entity = ModelSelectorEntity::Agent(agent.clone());


You want to work with an owned MofaAgent, so keep taking an owned MofaAgent :P

I would avoid cloning inside the function if possible and let the caller clone if it's really necessary.

Does inverting the two last lines work?

pub fn set_agent(&mut self, agent: MofaAgent) { let Some(mut inner) = self.borrow_mut() else { return; }; inner.chat_agent_avatar(id!(avatar)).set_agent(&agent); inner.entity = ModelSelectorEntity::Agent(agent); }

noxware

ufff took me a lot longer than I expected but it's done, not the perfect review but it's a review :D

noxware · 2024-12-16T22:07:15Z

src/chat/model_selector_list.rs

@@ -75,11 +74,11 @@ impl Widget for ModelSelectorList {
    }

    fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep {
-        let store = scope.data.get::<Store>().unwrap();
+        let mut store = scope.data.get_mut::<Store>().unwrap();


seems like store doesn't need to be mutable

src/data/chats/chat.rs

noxware · 2024-12-16T22:42:03Z

src/data/chats/chat_entity.rs

@@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize};
 pub enum ChatEntityId {
    ModelFile(FileID),
    /// Since agents are currently fixed enum values, the agent itself is the identifier.


Time to remove this comment, thanks :)

noxware · 2024-12-16T22:53:41Z

src/data/chats/mod.rs

 use std::{cell::RefCell, path::PathBuf, rc::Rc};

 use super::filesystem::setup_chats_folder;

+#[derive(Clone, Debug)]
+pub struct MofaServer {
+    pub address: String,


But the client already contains the address 🤔

noxware · 2024-12-16T22:54:18Z

src/data/chats/mod.rs

+#[derive(Clone, Debug)]
+pub struct MofaServer {
+    pub address: String,
+    pub client: Rc<MofaClient>,


Why wrapping it in an Rc?

noxware · 2024-12-16T23:30:04Z

src/landing/model_list.rs

@@ -149,14 +158,15 @@ impl Widget for ModelList {
    }

    fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep {
-        let store = scope.data.get::<Store>().unwrap();
-        let agents = MofaBackend::available_agents();
+        let store = scope.data.get_mut::<Store>().unwrap();


get_mut but variable is not mutable, so probably unnecessary

noxware · 2024-12-16T23:33:01Z

src/landing/model_list.rs

-                    *margin_bottom = 0.0;
+                let agents_availability = store.chats.are_agents_available();
+                match agents_availability {
+                    AgentsAvailability::NoServers => items.push(Item::NoAgentsWarning("Not connected to any MoFa servers.")),


hmmm... i suppose these strings are the same as in chat_history, maybe you could add a to_human_message()-like function to AgentsAvailability if you want to dedup, but I also like messages defined in the UI itself so, don't mind, just thinking aloud

noxware · 2024-12-16T23:35:44Z

src/settings/mofa_settings.rs

+    ConnectionStatusIcon = <View> {
+        visible: false
+        cursor: Hand


So this as a whole is a button, not an icon, right?

noxware · 2024-12-16T23:40:32Z

src/settings/mofa_settings.rs

-                color: #000
+        separator = <View> {
+            margin: {left: 20, right: 20, top: 0, bottom: 10}
+            width: Fill,


stupid comment crossing my mind: views are width: Fill, height: Fill by default. We are not being explicit about height being Fill, but we are explicit on width being Fill. Shall we remove width: Fill? add height? LET'S CHANGE THE WHOLE CODEBASE TO BE CONSISTENT :D 🔥

noxware · 2024-12-16T23:45:24Z

src/settings/mofa_settings.rs

+                        }
+
+                        let server = &servers[item_id].clone();
+                        let mut item_scope = Scope::with_props(server);


joulei added 16 commits December 11, 2024 16:21

WIP - support multiple MoFa clients

d62f0aa

Merge branch 'dev' into multiple-agents-support

ca9af02

Remove some cloning

5b422d2

Move mofa handling to chats mod

0a0e5cc

Cleanup agents list

d7ea3d6

Remove MofaClientInner indirection

8c71600

MofaServerItemProps

46516db

Rename ServerInfo into MofaServer

2ee347a

Some more cloning cleanup

cf767f7

Cleanup

bbc3892

Cleanup

ea07577

Cleanup

9d321e6

Cleanup

30fe839

Cleanup

3f88210

Refactor server testing and agent fetching

798379b

Handle no agents in the UI

511ef45

joulei marked this pull request as ready for review December 13, 2024 07:39

joulei commented Dec 13, 2024

View reviewed changes

src/data/chats/mod.rs Show resolved Hide resolved

joulei commented Dec 13, 2024

View reviewed changes

src/data/chats/chat.rs Show resolved Hide resolved

joulei requested a review from noxware December 13, 2024 07:52

joulei commented Dec 13, 2024

View reviewed changes

src/data/chats/mod.rs Show resolved Hide resolved

noxware reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple MoFa agents and servers #325

Support multiple MoFa agents and servers #325

joulei commented Dec 11, 2024 •

edited

Loading

noxware left a comment

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware left a comment

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024

noxware Dec 16, 2024 •

edited

Loading

noxware Dec 16, 2024

Support multiple MoFa agents and servers #325

Are you sure you want to change the base?

Support multiple MoFa agents and servers #325

Conversation

joulei commented Dec 11, 2024 • edited Loading

Changes

Limitations

Screen recordings

noxware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noxware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noxware Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joulei commented Dec 11, 2024 •

edited

Loading

noxware Dec 16, 2024 •

edited

Loading