-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add stuff-summarisation using runnable #558
Conversation
…e through the chat view.
…d-stuff-summarisation Pulling in work from REDBOX-337 so I can unblock myself developing summarisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially wasn't sold on the direction, but the more I think about it, the more I like it.
Overall, I think:
- We should have a principle of one
build_*
function per route (even the mundane ones), and these functions should use common keyword arguments and datatypes, and identical output shapes - This enables
build_chain
to focus on what it's really about -- routing logic -- because it makes chain building trivial - The summary chain needs work
- If Turning RAG into a runnable function #554 gets in before this PR, why not bring LCEL on the original runnables into this PR? As I see it, this is the "everything's in, plumb it up" PR
if callable(route_response): | ||
# if route_response is not None: | ||
build_chain = route_response | ||
chain, params = await build_chain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous comment about **kwargs
. Here you could then safely do...
build_chain(
**{
"chat_request": chat_request,
"user_uuid": user_uuid,
"llm": llm,
"vector_store": vector_store,
"storage_handler": storage_handler
}
)
...content in the knowledge it'll work for every single build_*
function. No more treating the chat function differently, no more worrying about how to handle future use cases!
core_api/src/build_chains.py
Outdated
async def build_stuff_chain( | ||
chat_request: ChatRequest, | ||
user_uuid: UUID, | ||
llm: ChatLiteLLM, | ||
vector_store: ElasticsearchStore, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> tuple[Runnable, dict[str, Any]]
on all these to help understand what they're doing
core_api/src/build_chains.py
Outdated
async def build_vanilla_chain( | ||
chat_request: ChatRequest, | ||
) -> ChatPromptTemplate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add
**kwargs
to all these function signatures - Ensure (as you currently have) that when they use a common keyword argument, it takes the same data
- Now you can call them all with unpacked dictionaries of keyword arguments
This means that build_vanilla_chain
can be called in just the same way as all the other build_*
functions even though it only needs a fraction of the arguments.
core_api/src/build_chains.py
Outdated
chat_request: ChatRequest, | ||
user_uuid: UUID, | ||
llm: ChatLiteLLM, | ||
vector_store: ElasticsearchStore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest generalising to build_summary_chain
-- I think these build_*
functions should match the routes even if they wrangle multiple runnables to do so.
See summarise()
in the summarise notebook for my take on this function. You're going to need a storage_handler: ElasticsearchStorageHandler
arg so you can retrieve the documents to summarise using core_api.src.format.get_file_chunked_to_tokens()
.
core_api/src/build_chains.py
Outdated
params = { | ||
"question": question, | ||
"content": context, | ||
"messages": [(msg.role, msg.text) for msg in previous_history], | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the unit test for what this should be (and actually a good example of what you need in this function overall)
# elif route_response is a Runnable | ||
if callable(route_response): | ||
# if route_response is not None: | ||
build_chain = route_response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overwrites its own function name -- change it
core_api/src/routes/chat.py
Outdated
|
||
return docs_with_sources_chain, params | ||
|
||
|
||
async def build_chain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get an output signature on this.
I also think a name like route_input_to_chain()
would be more helpful to understand what's going on here.
As I suggest above, I think this function's main role is to deal with routing logic. Getting from route -> chain can be made trivial, especially in a-build_
-function-per-route world.
@@ -54,72 +56,13 @@ | |||
"ability": ChatPromptTemplate.from_template(ABILITY_RESPONSE), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo make this a nice consistent dict[str, Callable]
-- make all values functions. Ways you might tackle this:
build_precanned_chain(response: ChatPromptTemplate, ...)
, andfunctools.partial
a version with theresponse
filled into each route- Start the pattern of "one
build_*
function per route" on the principle that it makes them all extensible, and most of them will one day need to be
I like this because it means in the function that deals with routing, once you've got the route name, you can just
...
return ROUTE_RESPONSES.get(route.name)(
**{
"chat_request": chat_request,
"user_uuid": user_uuid,
"llm": llm,
"vector_store": vector_store,
"storage_handler": storage_handler
}
)
This means that function can focus on the routing logic, which will get more complex as user override is added, and the "what chain do I need" stuff is moved out of the way.
Tbh, if you got to the point where the above was possible, consider changing build_chain
to pure routing logic, returning a string, because that little snippet would be all you needed to connect routes with the configured runnables you need. Just put it in the endpoint directly.
As I say, 50/50 on this one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although strap in for mypy's take on that code...
… WIP, as needs integrating with streaming
Merged as part of #570 |
Context
First pass as adding stuff summarisation functionality using Langchain Expression Language (LCEL) runnables.
Changes proposed in this pull request
chat.py
which uses the routing layer to decide with chain to build.build_chains.py
. Inbuild_chains.py
there is the originalbuild_vanilla_chain
andbuild_retrieval_chain
(both of which we will want to change to LCEL runnables as soon as possible, but that is not the focus of this PR)build_chain
selected isbuild_stuff_chain
. The idea behind this is it would be extensive to many different types of chains mapped to different routes in future.build_stuff_chain
is not yet complete. It uses themake_stuff_document_runnable
. @wpfl-dbt could you add retrieval of the documents forsummarisation?
Guidance to review
Relevant links
Things to check