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

Add logic within summarisation route to select appropriate stuff or map reduce method #681

Merged
merged 8 commits into from
Jun 28, 2024

Conversation

andy-symonds
Copy link
Contributor

@andy-symonds andy-symonds commented Jun 27, 2024

Context

When a user wants to summarise a document, they are not concerned with the method that is used to do the summarisation. Under-the-hood, however, when going down the summarisation route core-api needs to select the appropriate method, stuff for basic summarisation or map reduce for large document summarisation.

Changes proposed in this pull request

Reduced summarisation runnable to a single build_summary_chain that uses logic within the langchain runnable to decide which summarisation method to use, based on the size of the document, set by summarisation_chunk_max_tokens.

core-api now passes back to the frontend which summarisation method was used.

Guidance to review

Relevant links

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests

…stuff or map reduce summarisation method based on length of list of strings
…n for basic summarisation and map reduce summarisation for large documents
@jamesrichards4 jamesrichards4 marked this pull request as ready for review June 28, 2024 14:04
Copy link
Collaborator

@wpfl-dbt wpfl-dbt left a comment

Choose a reason for hiding this comment

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

Mostly fine!

return (
RunnablePassthrough.assign(documents=(make_document_context | RunnableLambda(format_documents)))
| make_chat_prompt_from_messages_runnable(
# Stuff chain now missing the RunnabeLambda to format the chunks
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need to make it into production

@@ -45,6 +45,7 @@ pytest = "^8.2.2"
pytest-env = "^1.1.1"
pytest-mock = "^3.14.0"
pytest-cov = "^5.0.0"
pytest-dotenv = "^0.5.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already use pytest-env and tbh I strongly prefer it, but if we're going this way we should remove it. No one actually uses it.

tests/test_journey.py Show resolved Hide resolved
Copy link
Collaborator

@wpfl-dbt wpfl-dbt left a comment

Choose a reason for hiding this comment

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

Happy

@jamesrichards4 jamesrichards4 merged commit ac1b9f1 into main Jun 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants