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

Refactoring web surfer to use function decorators #1435

Merged
merged 13 commits into from
Feb 10, 2024
Merged

Conversation

davorrunje
Copy link
Collaborator

@davorrunje davorrunje commented Jan 27, 2024

Web surfer has been refactored to use function decorators instead of manually constructed JSON entries for each function.

Type annotations are fixed to pass mypy checks.

Why are these changes needed?

This is functionally equivalent code, it is rewritten to use function decorators to serve as an example to others.

Related issue number

Checks

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2024

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (5d81ed4) 35.03% compared to head (d8a2973) 47.49%.

Files Patch % Lines
autogen/agentchat/contrib/web_surfer.py 58.53% 27 Missing and 7 partials ⚠️
autogen/browser_utils.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1435       +/-   ##
===========================================
+ Coverage   35.03%   47.49%   +12.46%     
===========================================
  Files          44       44               
  Lines        5383     5392        +9     
  Branches     1247     1333       +86     
===========================================
+ Hits         1886     2561      +675     
+ Misses       3342     2619      -723     
- Partials      155      212       +57     
Flag Coverage Δ
unittests 47.45% <64.35%> (+12.42%) ⬆️

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.

@afourney
Copy link
Member

Thanks @davorrunje. Generally this looks really good. I've not had time to test it, I've only reviewed the diff.

If it's passing the tests, and the notebook works, then I think we're in good shape.

One thing I am desperate to do is figure our a way not to require a Bing key -- but I'm also not about to fudge user_agents and scrape against tos. So, any suggestions for how to provide an alternate search engine experience open to all, but without violating tos, would be very welcome.

@davorrunje
Copy link
Collaborator Author

Thanks @davorrunje. Generally this looks really good. I've not had time to test it, I've only reviewed the diff.

If it's passing the tests, and the notebook works, then I think we're in good shape.

One thing I am desperate to do is figure our a way not to require a Bing key -- but I'm also not about to fudge user_agents and scrape against tos. So, any suggestions for how to provide an alternate search engine experience open to all, but without violating tos, would be very welcome.

I am still working on this because I need to change it a bit for my application, this was just something I wanted to discuss with you. We can easily mock Bing response for the test. I can add a bit more tests as I am studying how it works.

@davorrunje davorrunje marked this pull request as ready for review January 27, 2024 23:28
@afourney
Copy link
Member

A natural thing to do would be to add the ability to register new handlers for web search, and perhaps also for different mime-types -- similar to how browsers work today. Changing the default search engine should be easy.

WebSurfer is really just a proof of concept -- a placeholder for something that I believe is necessary (and something that many benchmarking scenarios demanded)

@davorrunje
Copy link
Collaborator Author

Oh, this is a lifesaver for me. It is definitely needed for many applications.

Copy link
Contributor

@AkashKulkarni4444 AkashKulkarni4444 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@sonichi sonichi added this pull request to the merge queue Feb 10, 2024
Merged via the queue into main with commit 8089058 Feb 10, 2024
56 of 60 checks passed
@sonichi sonichi deleted the fix-websurfer-functions branch February 10, 2024 05:43
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* refactoring web surfer to use function decorators

* limited pytest version

* bug fix in test

* bug fixes

* refactoring

* Fix web_surfer tests.

---------

Co-authored-by: Chi Wang <wang.chi@microsoft.com>
Co-authored-by: Adam Fourney <adamfo@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants