f/generic model support #24
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Diwank Singh Tomer <[email protected]>
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested.
- Reviewed the entire pull request up to 5079f33
- Looked at
387lines of code in9files - Took 2 minutes and 13 seconds to review
More info
- Skipped
1files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /agents-api/agents_api/routers/sessions/session.py:194:
- Assessed confidence :
90% - Grade:
0% - Comment:
Consider adding a check forNonesession data before setting the model in the settings. This could prevent potentialNoneTypeerrors. - Reasoning:
The PR modifies theforwardfunction in theBaseSessionclass to set the model from the session data. However, it does not handle the case when the session data isNone. This could potentially lead to aNoneTypeerror.
Workflow ID: wflow_D12y4kt0uxtFYdxZ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 439f465
- Looked at
13lines of code in1files - Took 4 minutes and 47 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /gateway/docker-compose.yml:5:
- Assessed confidence :
100% - Grade:
0% - Comment:
The PR description mentions updates to the .env.example file to support switching between different models. However, I don't see any changes in the .env.example file in the diff. Please ensure that the environment variables are correctly set up to support the new models. - Reasoning:
The PR description mentions that the .env.example file has been updated to switch between different models. However, there is no change in the .env.example file in the diff. It's important to ensure that the environment variables are correctly set up to support the new models.
Workflow ID: wflow_VGLnakxyL1LJajo0
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on e26ee10
- Looked at
52lines of code in2files - Took 2 minutes and 32 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. agents-api/agents_api/routers/sessions/session.py:66:
- Assessed confidence :
100% - Grade:
0% - Comment:
The 'content' field is being dumped into a JSON string without checking its type. This could lead to a failure if 'content' is not a dictionary or a list. Consider adding a check to ensure that 'content' is a dictionary or a list before attempting to dump it into a JSON string. - Reasoning:
In the file 'session.py', the 'content' field of the 'message' object is being dumped into a JSON string. This is a potential issue if the 'content' field is not a dictionary or a list, as 'json.dumps()' will fail for other data types. It would be better to add a check to ensure that 'content' is a dictionary or a list before attempting to dump it into a JSON string.
Workflow ID: wflow_vSal2jFZ04WgTjnU
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 9a14341
- Looked at
406lines of code in8files - Took 5 minutes and 14 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
7additional comments because they didn't meet confidence threshold of50%.
1. agents-api/agents_api/common/protocol/agents.py:11:
- Assessed confidence :
10% - Comment:
The removal of the ModelType literal is appropriate given the introduction of the model registry. This change helps to centralize the management of model types. - Reasoning:
The file 'agents.py' has been modified to remove the ModelType literal. This is likely because the model types are now being managed in the 'model_registry.py' file. This change seems logical and doesn't appear to introduce any issues.
2. agents-api/agents_api/common/protocol/entries.py:18:
- Assessed confidence :
50% - Comment:
The change from 'Any' to 'str' for the 'content' field is a tightening of the type requirements. Ensure that there are no parts of the codebase that rely on being able to assign non-string values to this field. - Reasoning:
The 'content' field in the 'Entry' class in 'entries.py' has been changed from 'Any' to 'str'. This change seems to be a tightening of the type requirements for this field. This could potentially introduce bugs if there are parts of the codebase that rely on being able to assign non-string values to this field. However, without knowing more about the codebase, it's hard to say for sure. The removal of the 'validate_content' method suggests that the author believes this stricter typing is sufficient to ensure the content is always a string.
3. agents-api/agents_api/models/agent/create_agent.py:23:
- Assessed confidence :
10% - Comment:
The replacement of the assertion with an if statement that raises an 'AgentModelNotValid' exception is a good change. It provides a more informative error message when an invalid model is provided and avoids using assertions for control flow. - Reasoning:
In the 'create_agent.py' file, the assertion that checks if the model is in the list of all available models has been replaced with an if statement that raises an 'AgentModelNotValid' exception if the model is not in the list. This is a good change as it provides a more informative error message when an invalid model is provided. It also removes the use of assertions for control flow, which is generally considered bad practice.
4. agents-api/agents_api/models/entry/proc_mem_context.py:9:
- Assessed confidence :
50% - Comment:
The change in the default value for the 'tools_confidence' parameter could potentially change the behavior of the 'proc_mem_context_query' function. Ensure that this change is intentional and doesn't introduce unexpected behavior. - Reasoning:
In the 'proc_mem_context.py' file, the default value for the 'tools_confidence' parameter in the 'proc_mem_context_query' function has been changed from 0.7 to 0. This could potentially change the behavior of the function, depending on how this parameter is used. Without more context, it's hard to say whether this is a bug or an intentional change.
5. agents-api/agents_api/routers/agents/routers.py:212:
- Assessed confidence :
10% - Comment:
The modification to directly call the 'create_agent_query' function without a try/except block is a good change. It simplifies the 'create_agent' function and moves the responsibility for validating the model to the 'create_agent_query' function. - Reasoning:
In the 'routers.py' file, the 'create_agent' function has been modified to directly call the 'create_agent_query' function without a try/except block. Previously, an 'AssertionError' would be caught and an 'AgentModelNotValid' exception would be raised. Now, the 'create_agent_query' function itself is responsible for raising the 'AgentModelNotValid' exception if the model is not valid. This is a good change as it simplifies the 'create_agent' function and moves the responsibility for validating the model to the 'create_agent_query' function.
6. agents-api/agents_api/routers/sessions/session.py:212:
- Assessed confidence :
10% - Comment:
The modification to return the result of the 'openai_client.chat.completions.create' function directly is a good change. It simplifies the 'generate' function and doesn't appear to introduce any issues. - Reasoning:
In the 'session.py' file, the 'generate' function in the 'BaseSession' class has been modified to return the result of the 'openai_client.chat.completions.create' function directly. Previously, this result was assigned to the 'res' variable, which was then returned. This change simplifies the function and doesn't appear to introduce any issues.
7. examples/discord-bot/main.py:71:
- Assessed confidence :
50% - Comment:
The change in the model used when creating the agent could potentially affect the behavior of the agent. Ensure that this change is intentional and doesn't introduce unexpected behavior. - Reasoning:
In the 'main.py' file, the 'init_agent' function has been modified to create an agent with the 'gpt-4-turbo-preview' model instead of the 'julep-ai/samantha-1-turbo' model. This change could potentially affect the behavior of the agent, depending on the differences between these two models. Without more context, it's hard to say whether this is a bug or an intentional change.
Workflow ID: wflow_Yq1RuioDOJhASyoJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on a50bf9a
- Looked at
324lines of code in13files - Took 3 minutes and 39 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
4additional comments because they didn't meet confidence threshold of50%.
1. .env.example:23:
- Assessed confidence :
80% - Grade:
0% - Comment:
The new environment variableMODEL_INFERENCE_URLis introduced in this PR. Please add comments or documentation explaining what this environment variable is used for and how it should be configured. - Reasoning:
The PR introduces a new environment variableMODEL_INFERENCE_URLin the.env.examplefile. However, there is no documentation or comments explaining what this environment variable is used for. It's important to document new environment variables to ensure they are used correctly.
2. .env.example:11:
- Assessed confidence :
80% - Grade:
0% - Comment:
The new environment variableOPENAI_API_KEYis introduced in this PR. Please add comments or documentation explaining what this environment variable is used for and how it should be configured. - Reasoning:
The PR introduces a new environment variableOPENAI_API_KEYin the.env.examplefile. However, there is no documentation or comments explaining what this environment variable is used for. It's important to document new environment variables to ensure they are used correctly.
3. .env.example:20:
- Assessed confidence :
80% - Grade:
0% - Comment:
The new environment variableMODEL_API_KEYis introduced in this PR. Please add comments or documentation explaining what this environment variable is used for and how it should be configured. - Reasoning:
The PR introduces a new environment variableMODEL_API_KEYin the.env.examplefile. However, there is no documentation or comments explaining what this environment variable is used for. It's important to document new environment variables to ensure they are used correctly.
4. .env.example:25:
- Assessed confidence :
80% - Grade:
0% - Comment:
The new environment variableMODEL_NAMEis introduced in this PR. Please add comments or documentation explaining what this environment variable is used for and how it should be configured. - Reasoning:
The PR introduces a new environment variableMODEL_NAMEin the.env.examplefile. However, there is no documentation or comments explaining what this environment variable is used for. It's important to document new environment variables to ensure they are used correctly.
Workflow ID: wflow_PXAxbhRtzOeBBOhS
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 262e92b
- Looked at
33lines of code in1files - Took 46 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /agents-api/agents_api/model_registry.py:111:
- Assessed confidence :
50% - Comment:
TheCLAUDE_MODELSdictionary is commented out in theALL_AVAILABLE_MODELSdictionary. If this is unintentional, please uncomment it. - Reasoning:
The CLAUDE_MODELS dictionary is commented out in the ALL_AVAILABLE_MODELS dictionary. This might be intentional, but it's worth pointing out in case it's an oversight.
Workflow ID: wflow_X1Y6mFAERy1txsNx
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
alt-glitch
previously approved these changes
Apr 13, 2024
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 0c59769
- Looked at
21lines of code in1files - Took 1 minute and 0 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_qM68KnDZofrx1nHD
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 1ba71b1
- Looked at
111lines of code in3files - Took 1 minute and 54 seconds to review
More info
- Skipped
1files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /agents-api/agents_api/models/agent/test_agent_queries.py:14:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider setting the model name as a variable that can be easily changed to allow for more flexible testing. - Reasoning:
The model name is hardcoded in the test files. This could lead to issues if the model name changes or if different models need to be tested. It would be better to set the model name as a variable that can be easily changed.
Workflow ID: wflow_IkasnsjSkcACtGoN
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on e4cd146
- Looked at
13lines of code in1files - Took 2 minutes and 2 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /agents-api/agents_api/model_registry.py:12:
- Assessed confidence :
100% - Grade:
0% - Comment:
There are several functions marked as 'TODO' in this file. These functions need to be implemented for the file to work correctly. Please ensure that these functions are implemented before merging the PR. - Reasoning:
The PR mentions that several files have been modified to handle the new models. However, the diff only shows changes in themodel_registry.pyfile. I need to check the other files mentioned in the PR description to ensure that they have been correctly modified to handle the new models. Also, themodel_registry.pyfile has several functions marked as 'TODO'. These functions need to be implemented for the file to work correctly.
Workflow ID: wflow_LHf1HJsQqlvePsGF
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on de6fe90
- Looked at
453lines of code in27files - Took 4 minutes and 12 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
3additional comments because they didn't meet confidence threshold of50%.
1. agents-api/agents_api/model_registry.py:112:
- Assessed confidence :
80% - Grade:
0% - Comment:
There are some unnecessary blank lines and inconsistent use of import statements in the code. Please consider removing the extra blank lines and organizing the import statements for better readability and consistency. This comment is applicable to multiple places in the codebase. - Reasoning:
The code seems to be well-structured and follows good practices. However, there are some minor issues related to code formatting and consistency. For example, in some files, there are unnecessary blank lines and inconsistent use of import statements. Also, some comments at the end of the file are not necessary and can be removed for better readability.
2. agents-api/agents_api/common/exceptions/agents.py:41:
- Assessed confidence :
70% - Grade:
0% - Comment:
Consider limiting the number of model names included in the error message for the 'AgentModelNotValid' exception. Concatenating all keys of the 'ALL_AVAILABLE_MODELS' dictionary might lead to a very long and unreadable error message if the dictionary is large. - Reasoning:
In the file 'agents-api/agents_api/common/exceptions/agents.py', there is a potential issue with the error message in the 'AgentModelNotValid' exception. The error message concatenates the keys of the 'ALL_AVAILABLE_MODELS' dictionary, which might not be the best approach if the dictionary is large. This could lead to a very long and unreadable error message.
3. agents-api/agents_api/routers/sessions/session.py:218:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider handling the case where the model is not in the 'JULEP_MODELS' dictionary in the 'generate' function. Currently, the 'extra_body' variable is set to None in this case, which could potentially lead to issues if the 'extra_body' variable is expected to be a dictionary in subsequent code. - Reasoning:
In the file 'agents-api/agents_api/routers/sessions/session.py', there is a potential issue with the 'generate' function. The function checks if the model is in the 'JULEP_MODELS' dictionary and if so, it sets the 'extra_body' variable to a dictionary with specific settings. However, if the model is not in the 'JULEP_MODELS' dictionary, the 'extra_body' variable is set to None. This could potentially lead to issues if the 'extra_body' variable is expected to be a dictionary in subsequent code.
Workflow ID: wflow_qmmEYKp9KNrAiq8M
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
creatorrr
commented
Apr 15, 2024
.gitignore
Outdated
| @@ -5,4 +5,9 @@ ngrok* | |||
| *.env | |||
| *.pyc | |||
| */node_modules/ | |||
| .devcontainer | |||
| node_modules/ | |||
| package-lock.json | |||
There was a problem hiding this comment.
why are these gitignored? be careful
|
Skipped PR review on d6df6ee because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away. Generated with ❤️ by ellipsis.dev |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR introduces support for a wider range of models in the agents API, includes a new model registry, modifies several files to handle the new models, adds new exceptions for invalid model names, updates the docker-compose file in the gateway, and reflects changes in test files.
Key points:
model_registry.py.docker-compose.ymlin the gateway.Generated with ❤️ by ellipsis.dev