-
Notifications
You must be signed in to change notification settings - Fork 45
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
Flag options #200
Flag options #200
Conversation
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.
These changes with renaming the test modules all look solid
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.
Hi folks, I did a thorough walkthrough. In general I like how this PR cleans things up. I made some suggestions for readability and maintainability.
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.
I think we are almost done here. My remaining unresolved comments are about the tests.
def test_get_question(): | ||
question = "What is AutoGPT and how does it compare with MetaGPT" | ||
|
||
if cfg.SERPER_API_KEY is 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.
OK. @20001LastOrder can you rename real
to something more explicit, e.g. external-api-caller
?
@20001LastOrder @YujingYang666777 I see there are also conflicts with the main branch. Please merge main into this branch to resolve conflicts. |
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.
I added a few change requests.
Also, please ensure make test
runs clean. When I run it I get 15 failed, 27 passed, 1 skipped, 7 warnings. Maybe I'm doing something wrong?
def execute(self, query) -> str: | ||
query = self.config_gsite_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.
This code adds site:
to the query by calling config_gsite_query
.
Then SearchTool._run adds site:
to the query by calling query_with_gsqite
.
(Assuming they are both using the same config.)
We should just add site
to the query once, either here in GoogleSearch
or in SearchTool
.
If gsite is intended to be specific to google search then do the work here in GoogleSearch
.
Either way I suggest calling the function query_with_gsite
or something like that.
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.
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.
OK, I had a look at the implementation. It seems that this feature is meant to be added to the SearchTool, so the one in the GoogleSearch action is removed. And I moved the test accordingly.
I was able to get the same failures. I fixed some of the failed tests due to the change introduced to the search tool. However, it seems other tests also fail on the main branch. We should tackle these failure cases in a separate pr. |
I reduced test failures somewhat. We now have 7 failed, 33 passed, 1 skipped, 7 warnings in 678.56s (0:11:18) . |
We should use dashes instead of underscores in command line options. This confirms with the [Gnu style guide](https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html)
argparse
to support different flag options to slack sherpaargparse
for more reliable results