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

Evaluating a pipeline consisting only of a reader node #2132

Merged
merged 9 commits into from
Feb 9, 2022

Conversation

julian-risch
Copy link
Member

@julian-risch julian-risch commented Feb 7, 2022

Proposed changes:

  • pipeline.eval() gets an additional parameter pass_documents_as_input that enables passing the gold documents specified in the labels to the first node in the pipeline as input. It's an alternative way to evaluate the reader node only, with the advantage that no retriever needs to be specified in the pipeline.

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@julian-risch julian-risch requested a review from tstadel February 7, 2022 10:12
@julian-risch julian-risch changed the title Reader eval only Evaluating a pipeline consisting only of a reader node Feb 7, 2022
@tstadel
Copy link
Member

tstadel commented Feb 7, 2022

I'm sure I'm missing something. But doesn't this do exactly the same as add_isolated_node_eval besides that the results are populated with eval_mode="integrated" rather than "isolated"?

@julian-risch
Copy link
Member Author

julian-risch commented Feb 8, 2022

I'm sure I'm missing something. But doesn't this do exactly the same as add_isolated_node_eval besides that the results are populated with eval_mode="integrated" rather than "isolated"?

The effect is the same, yes. The difference is that the PR here also works with a pipeline that consists only of a reader node. For some use cases you might not have a retriever. Thus, it would be unintuitive to create an ExtractiveQAPipeline and then run add_isolated_node_eval to evaluate only a reader.

@ju-gu
Copy link
Member

ju-gu commented Feb 8, 2022

    def run(self, query: str, documents: List[Document] = None, top_k: Optional[int] = None, labels: Optional[MultiLabel] = None, add_isolated_node_eval: bool = False):  # type: ignore
        self.query_count += 1
        if documents:
            predict = self.timing(self.predict, "query_time")
            results = predict(query=query, documents=documents, top_k=top_k)
        else:
            results = {"answers": []}

        # Add corresponding document_name and more meta data, if an answer contains the document_id
        results["answers"] = [
            BaseReader.add_doc_meta_data_to_answer(documents=documents, answer=answer) for answer in results["answers"]
        ]

        # run evaluation with labels as node inputs
        if add_isolated_node_eval and labels is not None:
            predict = self.timing(self.predict, "query_time")
            unique_docs = {label.document.id: label.document for label in labels.labels}
            relevant_documents = unique_docs.values()
            results_label_input = predict(query=query, documents=relevant_documents, top_k=top_k)

            # Add corresponding document_name and more meta data, if an answer contains the document_id
            results["answers_isolated"] = [
                BaseReader.add_doc_meta_data_to_answer(documents=relevant_documents, answer=answer)
                for answer in results_label_input["answers"]
            ]

        return results, "output_1"

@tstadel
Copy link
Member

tstadel commented Feb 8, 2022

I'm sure I'm missing something. But doesn't this do exactly the same as add_isolated_node_eval besides that the results are populated with eval_mode="integrated" rather than "isolated"?

The effect is the same, yes. The difference is that the PR here also works with a pipeline that consists only of a reader node. For some use cases you might not have a retriever. Thus, it would be unintuitive to create an ExtractiveQAPipeline and then run add_isolated_node_eval to evaluate only a reader.

@ju-gu and I just found out that we can achieve the same with isolated_node_eval (see @ju-gu's changes above).
All we need to do is change BaseReader.run() a bit:

  • make documents param optional
  • fix add_doc_meta_data_to_answer's documents param: should be relevant_documents instead
  • move predict = self.timing(self.predict, "query_time") before if documents block

The only advantage I can see for pass_documents_as_input is that it works with any node, not only Readers. But this is something we have deferred on porpuse. In addition, we might want to move the isolated_node_eval code from BaseReader.run() to BaseComponent._dispatch_run() in order to support all nodes. So I would opt for the isolated_node_eval solution here.

Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

LGTM!

@julian-risch julian-risch merged commit 7fab027 into master Feb 9, 2022
@julian-risch julian-risch deleted the reader_eval_only branch February 9, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants