Background task notifications #42

peterkeppert · 2024-09-25T13:37:48Z

PR for #38 I chose the safety check as example implementation.

I have a few points I would like improve here, but let's first see if the general idea is sound:

Adding task-specific fields to ChatMessage bloated the class a lot. Considering there are already fields specific to Human, AI and Tools messages, maybe it would be best to refactor so that there are specific ChatMessages mirroring Langchain messages.
As for changes in research_agent.py, I am not very happy that we would have to remember to remove task messages basically every time we want to use state["messages"] as model input. However, we need a place where all the messages including the Task messages are listed in order they were created, so that we can recreate the chat history including background task notifications.
I also don’t like the fact that TaskMessage is based on Langchain’s ChatMessage. BaseMessage would be more logical choice of base class, but then TaskMessage wouldn’t satisfy AnyMessage checks. I think that Langchain could support some kind of task message even itself. Do you think it makes sense to propose creation of such message type?

@JoshuaC215 please review the changes and let me know your thoughts.

JoshuaC215 · 2024-10-01T05:40:17Z

Hmmm so here are my quick and candid thoughts on this:

I like the Task abstraction
I like the nice isolated handling in service.py
I don't like the extension of ChatMessage to support TaskMessage and all the extra fields / code around that. I wonder if removing ChatMessage and just using LangChain types as mentioned in the other PR would help? Based on my experience with LangChain I doubt they will add this as a type, certainly not quickly without a strong use case.
I really don't like having to remove the tasks in every use of state["messages"], as you mentioned. Is there a way to store it under a different key in state? 🤔
I don't like the modifications in research_assistant.py and making that more complex to show this use case, since I think it's a bit niche. I wonder if it makes sense to add a simple version of the multiple agent support (kinda discussed in Supporting multiple agents with a discoverable registry of agents. #28) and then include this as a separate agent?

It might also be that this is just too complex to include as a niche case in the main repo (unless we see more discussion / demand for it) and we shouldn't work too much harder to try to force it in. I hope this doesn't land too discouraging and sorry if so!

peterkeppert · 2024-10-02T13:03:19Z

Not discouraging at all :)

ChatMessage abstraction - removing it as discussed in Add claude model example #41 would definitely help. Without the Langchain extending the message types, I think we need to live with the suboptimal solution that I proposed of extending Langchain's ChatMessage (to satisfy AnyMessage checks). The use case for an additional message type seems strong to me, especially looking at how OpenAI handles visualization of o1-series models' thinking process in ChatGPT UI. It looks very similar to what we are trying to do here, that is visualizing some background processing steps while the user waits for the tokens to start appearing, to make the waiting more palatable. The need grows as the background processes grow longer and more complex.
As for using state["messages"] for task messages, I think the idea of two state keys is sound. state["messages"] would contain clean conversation history (only AI/Human messages) and for example state["detailed_messages"] would contain complete history, including task messages. It needs to be a complete history so that we are able to place the task messages in correct places in chat when we are for example drawing chat from history. What do you think?
Multi-agent support discussed in Supporting multiple agents with a discoverable registry of agents. #28 looks interesting and it would make perfect sense to add such simple agent once the support is there, but I think background tasks alone don't justify building such support - they are not the core use case it should cover. Maybe simply a reference in README would be enough to get people started with this functionality?

JoshuaC215 · 2024-10-02T15:17:46Z

All makes sense to me. Using detailed_messages and duplicating sounds like a fine approach to preserve ordering information.

Thats an interesting point about o1 output and more general applicability. If you decide to open a langchain issue or discussion about this please do tag me in it / reference this! Will be interested to follow along.

Re: an agent implementation - readme approach sounds fine, another idea would be to add a simple agent in a new file in agent/ which uses this, and instructions to update the service to point to that agent (a one line change I think?) to try it out. Then it would be easy to test and easy to extend when we have the multi-agent support. If you do that approach and it makes sense to move common model init etc to a shared file feel free. Thoughts?

antonioalegria · 2024-10-06T08:01:48Z

Just jumping in on the conversation of removing the ChatMessage (also mentioned in #41), I do think ChatMessage is a useful abstraction. It simplifies and encapsulates what the ChatMessage should be in the presentation layer. It makes that layer much cleaner, even if it makes the service a bit more convoluted.

I think Langchain does not have a strong enough message abstraction as they can vary in structure inside depending on the model (as we see with the Anthropic models) and other context. Just my 2 cents.

peterkeppert · 2024-10-06T18:52:04Z

@antonioalegria I agree that Langchain's Message content field isn't the easiest to work with. The typing of Union[str, List[Union[str, Dict]]] introduces quite an overhead every time one wishes to parse it (or perhaps I am unaware of preexisting tooling). I wonder whether Langchain itself won't in the end standardize it on the List[Dict] format and simply apply type = "text" on items that would otherwise be strings. We could do it ourselves in the ChatMessage class. This would definitely be better than the current str content that prevents us from sending for example images to the UI altogether.

However, personally, I still favor removing ChatMessage class. It seems to me more in line with the purpose of the repository to provide a starting point for anybody willing to implement their own AI agent with Langgraph. By removing this class, we are giving up on trying to solve the issue of Langchain's imperfect message abstraction layer and simply showing how one can use it to implement an end-to-end agent with some limited UI capabilities (surely we won't demonstrate handling of all possible variations of message content). It removes some complexity from the code instead allowing us to focus on agent-related functionality, which I would be expecting this kind of repository to solve and leaving this particular issue to perhaps another project with heavier focus on UI.

That said, I believe the final decision rests with maintainer. @JoshuaC215 , please let us know how you decide. If you decide to remove the ChatMessage class, I think it would be best if you could open an issue for it, summarizing the reasons for the change. I think I would have some time next week to create a draft PR. Once we have this sorted out, I would finalize the background task notifications pull request.

JoshuaC215 · 2024-10-08T04:17:50Z

I wrote up the issue as requested, let me know what y'all think.

# Conflicts: # src/agent/research_assistant.py # src/schema/__init__.py # src/schema/schema.py # src/service/service.py Rollback research_agent changes

peterkeppert · 2024-10-21T14:04:50Z

Since we solved the chat message abstraction question, I resolved conflicts and added the "detailed_messages" state key. I have also added the simple version of the multiagent support as discussed in #28 , since I had it already developed. If you want to switch frontend to different backend agent, you need to switch URLs in client.py. Please review :)

What remains is to write up Lanchain issue for new message type, I will reference this thread.

antonioalegria · 2024-10-22T03:46:39Z

src/client/client.py

@@ -50,7 +50,7 @@ async def ainvoke(
            request.model = model
        async with httpx.AsyncClient() as client:
            response = await client.post(
-                f"{self.base_url}/invoke",
+                f"{self.base_url}/invoke/research-assistant",


Client should not be tied to the research-assistant, especially not in so many places, otherwise it will be harder to use for other agents we might implement.

The agent may be a path parameter. Also would make more sense to make this part of a separate changeset.

Done, removed multiagent implementation

antonioalegria · 2024-10-22T03:48:08Z

src/schema/schema.py

@@ -98,6 +104,25 @@ class ChatMessage(BaseModel):
        description="Original LangChain message in serialized form.",
        default={},
    )
+    task_name: str | None = Field(


to clean up ChatMessage, we could add a Task class with these fields and ChatMessage could have a task optional field.

This looks appealing, but I think it breaks the proposed abstraction.

Task class models a background task. As the task progresses (when it starts, finishes, or when it produces data during its execution), it emits TaskMessages, which (similar to how HumanMessages or AIMessages are handled) are then converted to ChatMessages, which serve as universal communication tool between server and client.

That said, I believe placing Task itself into ChatMessage is wrong, as this way ChatMessage would contain a reference to background Task, which can progress during ChatMessage's lifetime. This contradicts ChatMessage's purpose to communicate a single notification produced by Task execution at given point in time.

antonioalegria · 2024-10-22T03:48:56Z

src/schema/schema.py

@@ -128,6 +153,17 @@ def from_langchain(cls, message: BaseMessage) -> "ChatMessage":
                    original=original,
                )
                return tool_message
+            case TaskMessage():
+                task_message = cls(
+                    type="task",


Make TaskMessage have type=Task in the class definition, that way you don't need to pass the type here

I don't think this will work for two reasons:

ChatMessage type is not the same as Langchain BaseMessage type. They are distinct fields. AIMessage, HumanMessage and ToolMessage have their type set in the respective Langchain classes, but we still have to set ChatMessage's type manually as in these other cases. We could write everywhere:

type=message.type, ...

However, I am not sure of the value of this change.

TaskMessage is based on Langchain's ChatMessage which has type already set to chat. I couldn't base it on Langchain's BaseMessage, where I could set type arbitrarily, because TaskMessage needs the message to pass AnyMessage Langchain type checks, which Langchain and Langgraph rely on when working with messages. Using BaseMessage as base class would fail AnyMessage checks.

antonioalegria · 2024-10-22T03:49:25Z

src/schema/schema.py

+
+    role: str = "system_task"
+    """The role of the speaker."""
+


set type to Literal["task"] = "task"

Discussed above

antonioalegria · 2024-10-22T03:50:49Z

src/service/service.py

@@ -98,29 +99,50 @@ async def invoke(user_input: UserInput) -> ChatMessage:
        raise HTTPException(status_code=500, detail=str(e))


-async def message_generator(user_input: StreamInput) -> AsyncGenerator[str, None]:
+@app.post("/invoke/research-assistant")


research-assistant should not be in spread across the service code, only in the setup code (lifespan).

Done, removed multiagent implementation

antonioalegria · 2024-10-22T03:54:04Z

Since we solved the chat message abstraction question, I resolved conflicts and added the "detailed_messages" state key. I have also added the simple version of the multiagent support as discussed in #28 , since I had it already developed. If you want to switch frontend to different backend agent, you need to switch URLs in client.py. Please review :)

What remains is to write up Lanchain issue for new message type, I will reference this thread.

I do think the multiagent code should be in a separate changeset, and the service and client code should be as unaware as possible as to what agent is setup. In the main branch today that is totally transparent in the client and in the server it is contained in the lifespan setup, as it should be.

My suggestion is to move the multi-agent support to another issue/pull-request so that approaches could be discussed.

peterkeppert · 2024-10-22T07:20:41Z

Thanks for suggestions, multiagent implementation has been removed altogether, let's wait for progress on #28 . Instead, I added instructions to readme file.

JoshuaC215 · 2024-10-26T16:51:12Z

I left some thoughts on how to move this PR forward here

I'm hoping I'll have some time soon (in the next week or so, maybe sooner) to work on some of the changes that will unblock this.

JoshuaC215 · 2024-10-28T05:57:53Z

Spent some more time looking at this. Here's a commit of how I'd want to roughly approach merging this after #75 is merged. Open to discussion.

c5c7a2c#diff-b678273f64e8e9df8f3a41b23f9fba9ca9e4809b93b53843fb1c370a86e69110

^ I tested this version and it seems to work! 🎉

I think we could do the changes in service.py in a generic way and maybe even put this in the main streamlit app (although I'm a bit hesitant about that 😅 for complexity reasons, but maybe its OK).

One thing to call out, it wasn't obvious to me why we'd need the "detailed_messages" thing so I removed that. But open to discuss. It gets lost in history but I think that's OK? I don't really want to add this detailed_messages thing everywhere. Alternatively, maybe going back to just storing them in messages and filtering out of the LLM calls is the way to go.

peterkeppert · 2024-10-28T08:08:42Z

I checked the commit, thanks, looks great!

We agreed on having detailed_messages instead of filtering messages to keep messages as main conversation thread clean. In my case, content of task messages aids in understanding how and from which data the corresponding answer was generated or where an error occured. Of course, one can use Langsmith for the same purpose, but I wanted to give users a simple in-place insight without needing to use a different app - and therefore it makes sense to store these background messages in history.

For the toolkit's purpose, it probably makes sense to limit the use case to support only transient background task messages, which are not stored in history. In that case, neithed detailed_messages nor filtering is necessary. This limitation is definitely fine by me.

JoshuaC215 · 2024-10-28T14:45:24Z

Sounds good - I guess when I was trying it I wasn't seeing the use case for "detailed_messages" but maybe it makes sense in a more complex app. Anyway sounds like this will work for now. Happy to discuss that more after it's merged if you think there's a case to include it in the main toolkit.

Will revisit after #75 is merged in the next few days (hopefully). I want to get some review from other contributors since it's a big change. If you have any feedback on that PR (or other changes I made yesterday) please feel free to share!

JoshuaC215 · 2024-10-31T14:51:05Z

src/streamlit_app.py

+                status.write("---")
+                task_messages[task_data.run_id] = msg
+                state = "complete"
+                for message in task_messages.values():


message isn't actually used here. I was looking through this whole section of code and I'm not sure it works the way you intended - probably can use more cleanup

JoshuaC215 · 2024-10-31T15:41:37Z

I merged all the pre-requisite changes including the general implementation for CustomData

Here's my pass on adding a bg task agent:
main...bg-task-agent

I didn't clean up the code in the streamlit app marked above ^

Feel free to adapt this PR / open a new one. Or we can just use my branch if you prefer.

peterkeppert-anasoft added 2 commits September 25, 2024 14:13

background task notifications

2e10fe5

fix CI issues

7ac215b

JoshuaC215 mentioned this pull request Oct 8, 2024

Cleaning up ChatMessage ? #49

Closed

peterkeppert-anasoft added 2 commits October 21, 2024 08:16

Merge branch 'main' into background-task-notification

0d70a24

# Conflicts: # src/agent/research_assistant.py # src/schema/__init__.py # src/schema/schema.py # src/service/service.py Rollback research_agent changes

multiagent service, agent with bg tasks

cd7467d

antonioalegria reviewed Oct 22, 2024

View reviewed changes

peterkeppert-anasoft added 2 commits October 22, 2024 08:24

remove multiagent implementation

a624b67

readme entry for background task notifications

335f5a8

use ChatMessage custom type

a130b76

JoshuaC215 reviewed Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background task notifications #42

Background task notifications #42

peterkeppert commented Sep 25, 2024 •

edited

Loading

JoshuaC215 commented Oct 1, 2024

peterkeppert commented Oct 2, 2024

JoshuaC215 commented Oct 2, 2024

antonioalegria commented Oct 6, 2024

peterkeppert commented Oct 6, 2024

JoshuaC215 commented Oct 8, 2024

peterkeppert commented Oct 21, 2024

antonioalegria Oct 22, 2024

peterkeppert Oct 22, 2024

antonioalegria Oct 22, 2024

peterkeppert Oct 22, 2024

antonioalegria Oct 22, 2024

peterkeppert Oct 22, 2024

antonioalegria Oct 22, 2024

peterkeppert Oct 22, 2024

antonioalegria Oct 22, 2024

peterkeppert Oct 22, 2024

antonioalegria commented Oct 22, 2024

peterkeppert commented Oct 22, 2024

JoshuaC215 commented Oct 26, 2024

JoshuaC215 commented Oct 28, 2024

peterkeppert commented Oct 28, 2024 •

edited

Loading

JoshuaC215 commented Oct 28, 2024

JoshuaC215 Oct 31, 2024 •

edited

Loading

JoshuaC215 commented Oct 31, 2024

Background task notifications #42

Are you sure you want to change the base?

Background task notifications #42

Conversation

peterkeppert commented Sep 25, 2024 • edited Loading

JoshuaC215 commented Oct 1, 2024

peterkeppert commented Oct 2, 2024

JoshuaC215 commented Oct 2, 2024

antonioalegria commented Oct 6, 2024

peterkeppert commented Oct 6, 2024

JoshuaC215 commented Oct 8, 2024

peterkeppert commented Oct 21, 2024

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

antonioalegria commented Oct 22, 2024

peterkeppert commented Oct 22, 2024

JoshuaC215 commented Oct 26, 2024

JoshuaC215 commented Oct 28, 2024

peterkeppert commented Oct 28, 2024 • edited Loading

JoshuaC215 commented Oct 28, 2024

JoshuaC215 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

JoshuaC215 commented Oct 31, 2024

peterkeppert commented Sep 25, 2024 •

edited

Loading

peterkeppert commented Oct 28, 2024 •

edited

Loading

JoshuaC215 Oct 31, 2024 •

edited

Loading