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

Improvement in web commands functionalities #7068

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

amirdaaee
Copy link

@amirdaaee amirdaaee commented Apr 3, 2024

Background

this pr is to address feature requests in #7067 and #7066. in summary it adds two functionalities:

  • adding support for alternative duckduckgo_search SDK supported backends (as described here).
  • support for proxy in selenium for read_web command

Changes 🏗️

  • autogpts/autogpt/autogpt/config/config.py: added duckduckgo_backend and selenium_proxy attributes to Config class to control new feature behavior
  • autogpts/autogpt/autogpt/commands/web_search.py: added backend argument in calling for text() method of DDGS in web_search function
  • autogpts/autogpt/autogpt/commands/web_selenium.py: added --proxy-server argument in web-drive options
  • autogpts/autogpt/.env.template: added documents about duckduckgo_backend and selenium_proxy variables

PR Quality Scorecard ✨

  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

Copy link

netlify bot commented Apr 3, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit 2e92436
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/66890c624e9dfc00087130ee
😎 Deploy Preview https://deploy-preview-7068--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 54.24%. Comparing base (39f70b0) to head (2e92436).
Report is 13 commits behind head on master.

Files Patch % Lines
forge/forge/components/web/selenium.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #7068       +/-   ##
===========================================
+ Coverage   21.68%   54.24%   +32.56%     
===========================================
  Files          62      122       +60     
  Lines        3399     6880     +3481     
  Branches      440      882      +442     
===========================================
+ Hits          737     3732     +2995     
- Misses       2625     3014      +389     
- Partials       37      134       +97     
Flag Coverage Δ
Linux 53.99% <71.42%> (+32.31%) ⬆️
Windows 50.78% <71.42%> (+29.06%) ⬆️
autogpt-agent 34.01% <ø> (?)
forge 58.67% <71.42%> (+36.98%) ⬆️
macOS 53.29% <71.42%> (+31.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amirdaaee amirdaaee marked this pull request as ready for review April 3, 2024 09:20
@amirdaaee amirdaaee requested a review from a team as a code owner April 3, 2024 09:20
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 22, 2024
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@Swiftyos
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review

⏱️ Estimated effort to review [1-5]

3, because the PR involves multiple files and introduces new functionalities which require understanding the existing configuration and command structures. The changes are moderate in complexity, involving backend integrations and proxy settings which need careful review to ensure they are implemented correctly and securely.

🧪 Relevant tests

No

🔍 Possible issues

Possible Bug: The duckduckgo_backend variable is fetched from agent.legacy_config but there is no check to ensure that legacy_config or duckduckgo_backend is not None before using it. This could lead to AttributeError if legacy_config is not properly configured.

Possible Bug: The proxy configuration in web_selenium.py directly appends the proxy server argument without validating the format or existence of the proxy variable. This might lead to incorrect browser behavior if the proxy string is malformed.

🔒 Security concerns

No

Code feedback:
relevant fileautogpts/autogpt/autogpt/commands/web_search.py
suggestion      

Add a null check for ddc_backend before using it in the DDGS().text method to prevent potential runtime errors if the configuration is missing. [important]

relevant linequery, max_results=num_results, backend=ddc_backend

relevant fileautogpts/autogpt/autogpt/commands/web_selenium.py
suggestion      

Validate the format of the proxy variable to ensure it includes the necessary protocol (e.g., http:// or https://). This will prevent potential issues with browser proxy configuration. [important]

relevant lineoptions.add_argument("--proxy-server=%s" % proxy)


✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

  • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

@ntindle ntindle requested review from Pwuts and kcze June 11, 2024 01:18
@ntindle
Copy link
Member

ntindle commented Jun 11, 2024

Sorry for the long wait, this needs updated to component based. Most of it should move over pretty easily. @kcze can answer any questions you have

kcze added 2 commits July 3, 2024 12:23
- Move config to component configuration
- Update docs
@kcze
Copy link
Contributor

kcze commented Jul 3, 2024

Hey @amirdaaee, thanks for the contribution. I've just made a PR request on your branch if you merge it then the PR should be ready to merge into AutoGPT's master.
I've merged master into your branch and moved the code so it works with the new component-based architecture.
You can read more about it here: https://docs.agpt.co/forge/components/introduction/

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Jul 6, 2024
Copy link
Contributor

github-actions bot commented Jul 6, 2024

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@github-actions github-actions bot added documentation Improvements or additions to documentation Forge labels Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoGPT Agent documentation Improvements or additions to documentation Forge size/m
Projects
Status: 🆕 Needs initial review
Development

Successfully merging this pull request may close these issues.

None yet

5 participants