-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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 New Retriever Interface with Callbacks #5962
Conversation
_new_arg_supported: bool = False | ||
_expects_other_args: bool = False | ||
|
||
def __init_subclass__(cls, **kwargs: Any) -> None: |
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 where the magic is injected @hwchase17 @agola11 @nfcampos
5735a14
to
808cf7e
Compare
adca72e
to
e5fa935
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
lgtm
@@ -107,7 +114,13 @@ def _call( | |||
) | |||
else: | |||
new_question = question | |||
docs = self._get_docs(new_question, inputs) | |||
accepts_run_manager = ( |
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 this may be overkill. this only matters if someone has subclassed this and overridden _get_docs right?
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.
Yeah, though _get_docs is abstract so it would be anyone who subclasses this outside our repo. I don't know if it is the case
…trievers, adding tag arg to get_child()
9e67686
to
6f8fbf1
Compare
6f8fbf1
to
4b7d641
Compare
"""Get documents relevant for a query. | ||
|
||
def _get_relevant_documents( | ||
self, query: str, *, run_manager: CallbackManagerForRetrieverRun, **kwargs: Any |
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.
ahhh are we sure we wanna introduce arbitrary kwargs here? something we've previously discussed and ik @hwchase17 wanted to keep the interface tight/minimal
This introduced unexpected behavior for some retrievers. Running the I no longer see expected logging when running:
For E.g., it will run It appears the method name has been changed to And it now requires some additional args (e.g., AFAICT, If this is expected, then documentation will need to be updated for Also this exposes that fact that we need a test for |
Handle the new retriever events in a way that (I think) is entirely backwards compatible? Needs more testing for some of the chain changes and all. This creates an entire new run type, however. We could also just treat this as an event within a chain run presumably (same with memory) Adds a subclass initializer that upgrades old retriever implementations to the new schema, along with tests to ensure they work. First commit doesn't upgrade any of our retriever implementations (to show that we can pass the tests along with additional ones testing the upgrade logic). Second commit upgrades the known universe of retrievers in langchain. - [X] Add callback handling methods for retriever start/end/error (open to renaming to 'retrieval' if you want that) - [X] Update BaseRetriever schema to support callbacks - [X] Tests for upgrading old "v1" retrievers for backwards compatibility - [X] Update existing retriever implementations to implement the new interface - [X] Update calls within chains to .{a]get_relevant_documents to pass the child callback manager - [X] Update the notebooks/docs to reflect the new interface - [X] Test notebooks thoroughly Not handled: - Memory pass throughs: retrieval memory doesn't have a parent callback manager passed through the method --------- Co-authored-by: Nuno Campos <nuno@boringbits.io> Co-authored-by: William Fu-Hinthorn <13333726+hinthornw@users.noreply.github.com>
Handle the new retriever events in a way that (I think) is entirely backwards compatible? Needs more testing for some of the chain changes and all.
This creates an entire new run type, however. We could also just treat this as an event within a chain run presumably (same with memory)
Adds a subclass initializer that upgrades old retriever implementations to the new schema, along with tests to ensure they work.
First commit doesn't upgrade any of our retriever implementations (to show that we can pass the tests along with additional ones testing the upgrade logic).
Second commit upgrades the known universe of retrievers in langchain.
Not handled: