refactor: simplify Summarizer, add Document Merger

[anakin87]

Related Issues

• fixes Make Summarizer work in indexing pipeline #3403

Proposed Changes:

As discussed in #3403

• make the summarizer suitable for indexing pipelines
• write the summarization results in meta instead of altering document content (current behavior)
• remove generate_single_summary parameter: currently it transforms several documents into one
  (it is better to design a dedicated node for the purpose of merging documents)

How did you test it?

• Adapted some tests
• Other tests to be added?

Notes for the reviewer

Just a first draft to understand what this change breaks

Checklist

• I have read the contributors guidelines and the code of conduct
• I have updated the related issue with new insights and changes
• I added tests that demonstrate the correct behavior of the change
• I've used the conventional commit convention for my PR title
• I documented my code
• I ran pre-commit hooks and fixed any issue

[anakin87]

As expected, this refactoring was breaking a few things, which I tried to fix. 😄
I made some comments in correspondence with the tests, which required the most onerous changes.

@ZanSara @brandenchan @bogdankostic please feel free to jump in and help to go in the right direction...

[ZanSara]

Looking great so far! I don't think you need more tests for this node right now, given we're only removing functionality. I found a coupe of leftover prints and one question only.

In addition, let's add the DocumentMerger in this same PR. This way we can re-introduce some of the removed tests with the new setup (docs merger + summarizer) and make sure no functionality is lost.

[anakin87]

@ZanSara thanks! When I have some time, I'll make the changes you suggested...

Just some questions:
how do you imagine the Document Merger?
How do we deal with the metadata of aggregated documents? Do we remove them?

[ZanSara]

How do you imagine the Document Merger?

So, the DocumentMerger should be super simple, something that just merges the content of the documents so that given a list of N, it returns a list of 1 (that's because Pipeline.run() always expects documents to be a List - and Pipeline.run_batch() always expects documents to be a List of List).

We might want to deal with the possibility of receiving input from multiple nodes. That's a messy topic though, so if you see that it becomes intractable and makes the node complex, ignore that. If anyone needs to handle multiple inputs, they can use JoinDocuments on top of the Merger for now.

How do we deal with the metadata of aggregated documents? Do we remove them?

Now that's a great question 😄 My intuition says that we should apply the following heuristic:

1. If a key is present with the same value in all documents to be merged, add it to the merged doc too.
2. Any other key is removed.

Mind that keys might contain nested values. I'd apply such heuristic recursively on dictionary entries.

Let's also pay attention to keep "important" keys like name, which is often accessed. I don't know how we could deal with a conflict there, so let's see which (if any) test break when we just treat it as any other, and decide once we have the outcomes. I also can't name any other critical field right now, so let's see if any test breaks and add other "special" keys to this list as necessary.

[vblagoje]

Hey @anakin87 and @ZanSara, I am always for simplification and refactoring, but I don't think we can remove generate_single_summary feature in a non-major release - even if it was a total mess. There might be some users who are relying on it.

[anakin87]

By introducing Document Merger, I think we would offer the same feature in a different and more structured way.
However, I agree that if someone is using generate_single_summary, we will break down how this feature is currently working...

@vblagoje @ZanSara Before investing time in this PR, I wait for your opinions!

[ZanSara]

Hey @anakin87! I'm going to review this PR now. There has been two new test suites introduced a moment ago, they just need another rebase and they'll pass 👍

[ZanSara]

Great progress! That's precisely how I imagined it.

A couple of things to note;

• I think I found a heavy simplification of the meta fields merging algorithm and I've suggested it, Test it out if it works as expected or if I forgot something!
• We need to make sure the documentation for DocumentMerger is generated. To do so, let's add document_merger to this list:

  haystack/docs/_src/api/pydoc/other.yml

  Line 4 in 8ddeda8

  modules: ['docs2answers', 'join_docs', 'join_answers', 'route_documents']

[anakin87]

@ZanSara thanks for the great review!!!

[anakin87]

integration-tests-windows (nodes) is failing:

ERROR test/nodes/test_generator.py::test_generator_pipeline[embedding-memory] - RuntimeError: [enforce fail at C:\actions-runner_work\pytorch\pytorch\builder\windows\pytorch\c10\core\impl\alloc_cpu.cpp:81] data. DefaultCPUAllocator: not enough memory: you tried to allocate 4194304 bytes.

Any ideas?

[ZanSara]

Hey @anakin87 sorry for this CI issue. We're working on it!

[anakin87]

Hi @ZanSara, I see that thanks to your contributions the tests are passing now!

Can you explain to me very quickly what you did?
Maybe next time I will be able to solve the problem on my own...

[ZanSara]

Sure! This was an OOM (out-of-memory) error on the Windows GH runner. It's not the first time we see them...

What it means is that the machine simply doesn't have enough RAM left for all tests. What I've done was to reduce the memory footprint of the tests in two ways:

• By removing one test that seemed slightly redundant: refactor: simplify Summarizer, add Document Merger #3452 (comment)
• By using a much much smaller model for Summarizer in a test that was not designed to produce output: refactor: simplify Summarizer, add Document Merger #3452 (comment)

Unfortunately the test suites are very heavy and Windows runners tend to collapse rather fast. If that happens again, reducing the model size will probably help, but don't be afraid to ask for help if that's not enough. That's our fault after all! 😄