Add Sample Research Assistant App built with AutoGen #196 #509
Conversation
victordibia
requested review from
pcdeadeasy,
afourney and
gagb
and removed request for
pcdeadeasy
Codecov Report
@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 42.59% 41.75% -0.84%
==========================================
Files 21 21
Lines 2592 2632 +40
Branches 581 592 +11
==========================================
- Hits 1104 1099 -5
- Misses 1398 1442 +44
- Partials 90 91 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
| Also, if you plan to use bing search, set the `BING_API_KEY` | ||
|
|
||
| ```bash | ||
| export OAI_CONFIG_LIST=/path/to/OAI_CONFIG_LIST |
There was a problem hiding this comment.
Sample app should use config instructions that are consistent with the rest of autogen repo.
There was a problem hiding this comment.
Can you clarify what consistency issues you see?
There was a problem hiding this comment.
Sure:
AutoGen:
Set OAI_CONFIG_LIST env to the content of the json with models etc
or
Ensure there is a file called OAI_CONFIG_LIST in the file_location
RA:
Set OAI_CONFIG_LIST env to the path of the json
Even when I set the path to ~/OAI_CONFIG_LIST the code failed with "No api key error". That file exists on my machine.
If the current way is better, AutoGen repo should change the instructions?
There was a problem hiding this comment.
Ahhh. I think that documentation is wrong. That's what was confusing me.
There is no condition where it should be set to the file path. ..It would never work. It only works when set to the file contents.
The Readme in the backend folder is correct, while the Readme in the root is incorrect. The correct Readme states:
Create a file called `OAI_CONFIG_LIST` in `backend` with the following (JSON) format.
AutoGen can read this file and use it to initialize the OpenAI API.
| @@ -0,0 +1,5 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is present in the repo but never referenced in the readme. Delete or refer
There was a problem hiding this comment.
Definitely do not delete that file. It greatly simplifies getting the environment set up. Better to document it I guess.
| "title": "Coder Only", | ||
| "description": "A two agent workflow with a coder and an assistant", | ||
| }, | ||
| # { |
There was a problem hiding this comment.
Why are these lines commented out?
There was a problem hiding this comment.
Because we only demonstrate the Coder workflow.
There was a problem hiding this comment.
Hmm, but the code contains files for the planner workflow? Commented lines without any context can cause confusion.
There was a problem hiding this comment.
@gagb, the planner code never got tested. This code is a sample and as is. We could add a comment saying this code demonstrate a planner workflow.
There was a problem hiding this comment.
Either more testing + uncomment or no testing + more clarifying documentation is okay with me. Both reduce confusion.
| @@ -0,0 +1,50 @@ | |||
| ## FastAPI Backend for AutoGen Research Assistant | |||
There was a problem hiding this comment.
There is inconsistency and overlap between the README in root of the app and the backend/ folder.
| allow_headers=["*"], | ||
| ) | ||
|
|
||
| root_file_path = os.path.dirname(os.path.abspath(__file__)) |
There was a problem hiding this comment.
Global variables in python should be capitalized. Refactor.
| files_static_root = os.path.join(root_file_path, "files/") | ||
| static_folder_root = os.path.join(root_file_path, "ui") | ||
|
|
||
| os.makedirs(files_static_root, exist_ok=True) |
There was a problem hiding this comment.
Add documentation for these statements.
|
|
||
|
|
||
| # Example usage: | ||
| path_to_config_list = "./OAI_CONFIG_LIST" |
There was a problem hiding this comment.
Inconsistent with instructions in the README. Uses a hardcoded path.
|
|
||
| # Example usage: | ||
| path_to_config_list = "./OAI_CONFIG_LIST" | ||
| resulting_config = create_llm_config(path_to_config_list) |
There was a problem hiding this comment.
If this is a global variable, capitalize and define on the top. Is it even being used?
| ra_type=ra_type, | ||
| silent=False, | ||
| # agent_on_receive=pretty_print_agent_message, | ||
| path_to_config_list="./OAI_CONFIG_LIST", |
There was a problem hiding this comment.
Hardcoded path. Inconsistent with instructions in the README.
There was a problem hiding this comment.
This is a quirk of autogen's config_list_from_json. It actually doesn't matter what this file path is. If there's an environment variable, Autogen will use it.
There was a problem hiding this comment.
I don't understand your argument. README asks to set a env variable but the code instead is using a hard coded path. This will confuse users.
There was a problem hiding this comment.
Yeah, I mean we can remove this probably. My point was that if there's an OAI_CONFIG_LIST in os.environ, it won't even look at the filesystem. Having it in the environment supersedes everything else, so this is likely superfluous.
There was a problem hiding this comment.
Yes this is another reason consistency with AutoGen repo is important.
|
Per the AutoGen contributing guidelines we need to add test cases. https://microsoft.github.io/autogen/docs/Contribute |
| @@ -0,0 +1,88 @@ | |||
| # ARA - AutoGen Research Assistant | |||
|
|
|||
| ARA is an Autogen-powered AI research assistant that can converse with you to help you conduct research, write and execute code, run saved skills, learn new skills by demonstration, and adapt in response to your interactions. | |||
There was a problem hiding this comment.
Clarify that this is a prototype
|
|
||
| ARA is an Autogen-powered AI research assistant that can converse with you to help you conduct research, write and execute code, run saved skills, learn new skills by demonstration, and adapt in response to your interactions. | ||
|
|
||
| Project Structure: |
|
|
||
| To update the web ui, navigate to the frontend directory, make changes and rebuild the ui. | ||
|
|
||
| ## Capabilities |
There was a problem hiding this comment.
Highlight how to switch agent workflow. Add example of how to add a new workflow.
There was a problem hiding this comment.
We have not tested the code paths when we switch the workflow. So we probably don’t want to highlight that?
There was a problem hiding this comment.
I think we should be? @samershi what do you think?
There was a problem hiding this comment.
What other agent workflows do we have built-in at the moment?
There was a problem hiding this comment.
We do not have any others. The planner agent is commented out, and very untested. The Group Chat agent is in another branch, and... needs work.
There was a problem hiding this comment.
@samershi, IMO that workflow testing will block the PR. We can add it as an issue for the future iterations of the RA?
There was a problem hiding this comment.
I'd just remove all mention of the Planner workflow... comments and otherwise. It's not tested. We abandoned it quickly back in September. Having it here adds work on testing something we already opted not to use. We can add in the ability to switch workflows in a later PR, when we add GroupChat.
|
|
||
| ```bash | ||
| cd backend | ||
| uvicorn main:app --reload --port 8000 |
There was a problem hiding this comment.
clarify the usage of --reload
|
|
||
| config_list = autogen.config_list_from_json(path_to_config_list) | ||
| llm_config = { | ||
| # "request_timeout": 600, |
There was a problem hiding this comment.
Because OpenAI v0.2 does not have this parameter.
There was a problem hiding this comment.
Okay the comment should clarify that.
| import ast | ||
| from datetime import datetime | ||
|
|
||
| from autogen.code_utils import extract_code |
There was a problem hiding this comment.
Name of this file overlaps with autogen.code_utils which is confusing
There was a problem hiding this comment.
Maybe even the code is an overlap since I might have seen a comment somewhere that parts of it were borrowed from AutoGen. That said, the codes exists in completely different modules and shouldn't be an issue?
There was a problem hiding this comment.
Since this is a sample app we should follow best principles and reduce redundant code. Naming conflict did create confusion for me. But I agree that it does not affect functionality.
| import autogen # Assuming you have the autogen module imported or installed | ||
| from autogen import oai | ||
| from typing import Dict, List, Optional, Tuple, Union | ||
|
|
There was a problem hiding this comment.
add documentation for what kind of utils this file contains
| import utils.utils | ||
| from utils import generate_oai_reply, copy_utils | ||
|
|
||
| from enum import Enum |
There was a problem hiding this comment.
In built python modules should be imported before external/project modules?
|
@victordibia and @pcdeadeasy, I have too much on my plate until Wednesday to push commits to this PR beyond reviewing it but lmk (here) if any of my review comments aren't clear or don't make sense. |
* added litellm+ollama cookbook * fix annotations * fix type * fix for ruff * fix for ruff * resolve comments * add to index.rst * reverting index.rst * fix to ignore type --------- Co-authored-by: prankur <[email protected]> Co-authored-by: Eric Zhu <[email protected]>
Why are these changes needed?
This PR provides an example of a research assistant web application (React) built with AutoGen.
Related issue number
#196
Checks