-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
Update serpapi.py #4947
Update serpapi.py #4947
Conversation
added link option in process_response
langchain/utilities/serpapi.py
Outdated
@@ -148,6 +148,8 @@ def _process_response(res: dict) -> str: | |||
and "description" in res["knowledge_graph"].keys() | |||
): | |||
toret = res["knowledge_graph"]["description"] | |||
elif "link" in res["organic_results"][0].keys(): |
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.
Question: what would make the link take precedence over a snippet? The latter seems more immediately useful to the LLM unless it is given a browser/curl
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.
True point, a snippet can give direct context to LLM. Would it make sense to add a flag to run method, in order to control what the output is?
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.
It could. I'm not sure people are going to actively use that level of configurability out of the box though. Could we just add this as the last option for now? Then it would be less impactful to existing users who rely on the snippets
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.
Oh yes definitely, I agree with your point that a snippet is more useful, and adding it like that might break existing codes. What I was saying was more of an open suggestion/discussion.
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.
We can consider it in a subsequent PR!
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.
For this Pr, should we add the link as a last option before the last "not found option".
I will make a new PR with the change we discussed about having the option to manually override. And can continue there about it
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.
Yes! - add as a last option, don't add an additional config flag here, can open a different PR to consider that. Thank you!
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.
Added all the changes
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.
thanks!
Added link option in _process_response
Fixes # (issue)
In _process_response link provided correct answers while the snippet reply provided non working links
@vowelparrot
Before submitting
Who can review?
Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested: