-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: Implement RAGEvaluationHarness
and related classes
#10
Conversation
7cd05d2
to
84c0fd2
Compare
84c0fd2
to
cb80fca
Compare
The test failure is due to a seemingly missing dependency, but the logs show that it's being installed 🤔 I'll look into this later, but the PR is reviewable. Tests pass locally. |
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.
Good work Madeesh, I left some minor comments, otherwise looks good.
return cls(rag_pipeline, rag_components, deepcopy(metrics)) | ||
|
||
@classmethod | ||
def default_with_keyword_retriever( |
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 is exactly like the function above, only changing the input_mapping={"query": "query"}
for the RAGExpectedComponent.QUERY_PROCESSOR
- maybe we can make this one function and make this an arg or some other way to collapse this into a single function?
RAGExpectedComponent.QUERY_PROCESSOR: RAGExpectedComponentMetadata(
name="retriever", input_mapping={"query": "query"}
)
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.
The motivation behind having separate "default" functions is to make it as simple as possible for the user to initialize a pipeline without having to understand the mapping of the expected components. If we were to merge the two into a single function, we'd have to expose some kind of enumeration to select the type of query processor component (as it can't be a simple boolean flag given the semantics of the parameter), which introduces more code than it removes.
Co-authored-by: David S. Batista <dsbatista@gmail.com>
…xperimental into feat/eval-harness-rag
Co-authored-by: David S. Batista <dsbatista@gmail.com>
Pull Request Test Coverage Report for Build 9349272889Details
💛 - Coveralls |
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.
Looks good. 👍
Proposed Changes:
This PR implements the
RAGEvaluationHarness
and its related classes, building up on #4, #5.Part of deepset-ai/haystack#7526.
How did you test it?
Unit tests
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.