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

feat: exception handling for api keys and models #197

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

alt-glitch
Copy link
Contributor

@alt-glitch alt-glitch commented Apr 16, 2024

  • prints out warning that OpenAI key is not defined in .env
  • throw exception to client while creating agent with openai models if OpenAI key is not defined in .env
  • defined validate_configuration method in model_registry to validate the availability and config (api key) of the model
  • remove validation from create_agent_query and added to create_agent in routers.py

Ellipsis 🚀 This PR description was created by Ellipsis for commit 1b14574.

Summary:

This PR introduces exception handling for undefined OpenAI keys, moves model validation from create_agent_query to create_agent, defines a validate_configuration method in model_registry.py, modifies the model_dump argument in session.py, with changes spread across multiple files.

Key points:

  • Introduced exception handling for undefined OpenAI keys.
  • Moved model validation from create_agent_query to create_agent in routers.py.
  • Defined validate_configuration method in model_registry.py for model availability and config validation.
  • In session.py, changed model_dump argument from mode='json' to `exclude='id'.
  • Changes spread across agents.py, env.py, model_registry.py, create_agent.py, routers.py, and session.py.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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!

  • Reviewed the entire pull request up to 109da71
  • Looked at 152 lines of code in 5 files
  • Took 1 minute and 21 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. agents-api/agents_api/common/exceptions/agents.py:57:
  • Assessed confidence : 100%
  • Grade: 0%
  • Comment:
    Please add a newline at the end of the file.
  • Reasoning:
    The changes in the PR seem to be well implemented. The author has added exception handling for the case when the OpenAI key is not defined in the environment variables. This is a good practice as it ensures that the application doesn't fail silently when the key is missing. The author has also moved the model validation from the create_agent_query function to the create_agent function in the router. This is a good move as it allows for the validation to be done before the query is constructed, potentially avoiding unnecessary database operations. However, I noticed that there is no newline at the end of the agents.py file. This is a minor issue but it's a good practice to always end a file with a newline.

Workflow ID: wflow_9TJxCKf0jt4jPXdD


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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!

  • Performed an incremental review on 884ab9e
  • Looked at 13 lines of code in 1 files
  • Took 1 minute and 39 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. agents-api/agents_api/model_registry.py:114:
  • Assessed confidence : 0%
  • Comment:
    The change to convert the keys of the dictionary ALL_AVAILABLE_MODELS to a list before passing it to the AgentModelNotValid exception is fine. It ensures that the keys are passed as a list to the exception, which might be expecting a list.
  • Reasoning:
    The change in the PR is to convert the keys of the dictionary ALL_AVAILABLE_MODELS to a list before passing it to the exception. This change is fine as it ensures that the keys are passed as a list to the exception, which might be expecting a list. There doesn't seem to be any logical, performance, or security issues with this change.

Workflow ID: wflow_PdZNcZ6y5FqTKYwv


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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!

  • Performed an incremental review on 1b14574
  • Looked at 13 lines of code in 1 files
  • Took 1 minute and 24 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. /agents-api/agents_api/routers/sessions/session.py:207:
  • Assessed confidence : 50%
  • Comment:
    This change excludes the 'id' field from the dumped model. Please ensure that this doesn't affect any functionality where the 'id' field is expected in the dumped model.
  • Reasoning:
    The change in this PR is related to the model_dump method call in the generate function. The 'exclude' parameter is set to 'id'. This change seems to be intended to exclude the 'id' field from the dumped model. However, it's not clear why this is necessary. I need to check the model_dump method and its usage to understand the implications of this change.

Workflow ID: wflow_UWHcwhJvi34JjCkS


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

@creatorrr creatorrr merged commit 62201dd into dev Apr 17, 2024
1 check passed
@creatorrr creatorrr deleted the f/openai-model-exceptions branch April 17, 2024 04:53
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.

2 participants