Runnable with message history

[baskaryan]

https://dev.smith.langchain.com/public/c1c06afb-2e4f-4b53-bf37-6926003e7fa9/r

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
langchain	⬜️ Ignored (Inspect)	Visit Preview		Nov 17, 2023 7:30pm

[hinthornw]

These branches would never be reached. run.outputs is always a dict

[baskaryan]

should this be Union[str, BaseMessage, Sequence[BaseMessage]] to be safer?

[eyurtsev]

Haven't finished reviewing yet -- have to run, will try to continue in the evening

[eyurtsev]

What's the reason why we need both instead of just having a session_id and having the client decide how to deal with that? The reason is that we're opinionated about it's assembled (i.e., we're not passing both parameters independently)

[baskaryan]

cc @nfcampos

[eyurtsev]

After thinking a bit I'm OK with user id and thread id being here by default -- seems like it correctly parameterizes history

[nfcampos]

This is what we wanted for opengpts doesn't necessarily make sense here

[eyurtsev]

With that said I'm OK with user id and thread id as being the default parametrization, but another thing we could do is to accept the parameterization through the init, and pass it along to the factory

[eyurtsev]

After thinking a bit I'm OK with user id and thread id being here by default -- seems like it correctly parameterizes history

[eyurtsev]

What if instead we let the factory accept a dict with inputs so we avoid doing the string concatenation here, then users won't have to create parsing code in the factory to determine if it was only thread id or user and thread id that were provided etc

    def _merge_configs(self, *configs: Optional[RunnableConfig]) -> RunnableConfig:
        config = super()._merge_configs(*configs)
        # extract thread_id
        if "thread_id" not in config.get("configurable", {}):
            example_input = {self.input_messages_key: "foo"}
            example_config = {"configurable": {"thread_id": "123"}}
            raise ValueError(
                "thread_id is required."
                " Pass it in as part of the config argument to .invoke() or .stream()"
                f"\neg. chain.invoke({example_input}, {example_config})"
            )
        # attach message_history
        thread_id = config["configurable"]["thread_id"]
        user_id = config["configurable"].get("user_id")
        config["configurable"]["message_history"] = self.factory(
            {"thread_id": thread_id, "user_id": user_id}
        )
        return config

[baskaryan]

no strong opinions on my end, thoughts @nfcampos?

[nfcampos]

I didn't do that because none of our chat history classes accept such a dict, so all would need wrappers to be used here if we go with that

[eyurtsev]

What do we think if we reduce this to session_id as a default to match existing implementations?

• It's possible to parse user id and thread id from the session id, so it won't block folks from implementing that.

---

In addition, we could do the following as an enhancement if we like it in the future:

• enhance functionality to accept a factory which takes either a dict or a str as the first argument and based on the type signature invoke it appropriately
• Allows user to override default config spec by passing something during initialization (and assume this only works with a factory that takes a dict)

[eyurtsev]

What do you think about changing the factory signature to:

factory: Callable[[dict], BaseChatMessageHistory]

It might be annoying client side to parse the string?

[nfcampos]

Replied in other thread

[eyurtsev]

Do we need to support an async version of on_end ?

[nfcampos]

With listeners runs it in thread pool. Given chat history classes don't have async methods unfortunately there's no advantage to having native async method

[eyurtsev]

Because this is handled by a tracer any exception in this code will be silently ignored. I don't think this is the desired behavior by default?

[eyurtsev]

I think this looks good

[eyurtsev]

Suggested change

	GetSessionHistoryCallable = Callable[..., BaseChatMessageHistory]
	GetSessionHistoryCallable = Callable[[str], BaseChatMessageHistory]

[baskaryan]

might cause issues if you pass in something that takes more than str (like RedisChatMessageHistory)