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

Add openai key path #545

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Conversation

regmibijay
Copy link
Contributor

@regmibijay regmibijay commented Sep 10, 2023

Currently llm.OpenAI only accepts api_key where as openai module provides additional function of passing api_key_path which ensures api_key is read from this file.

This might be beneficial, if you are using PandasAI together with native openai workflows because of option of defining openai_key_file parameter once and both libraries using this path.

Summary by CodeRabbit

  • New Feature: Introduced an optional parameter api_key_path to the OpenAI class in pandasai/llm/openai.py. This allows users to provide a path to their OpenAI API key file, offering an alternative to directly using the api_token.
  • Test: Added a new test case test_type_with_key_path in tests/llms/test_openai.py to ensure that when api_key_path is provided, the type attribute of the OpenAI instance is correctly set to "openai".

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 10, 2023

Walkthrough

The changes introduce an optional parameter api_key_path to the OpenAI class, allowing users to provide a path to the API key file. The logic has been updated to handle both direct api_token and api_key_path. A new test case has been added to ensure the functionality works as expected.

Changes

File Summary
pandasai/llm/openai.py Introduced api_key_path as an optional parameter in OpenAI class. Updated logic to handle both api_token and api_key_path. Exception handling for missing parameters.
tests/llms/test_openai.py Added a new test case test_type_with_key_path to validate the new api_key_path functionality.

🐇💻

In the land of code, where logic intertwines,

A rabbit hops, leaving traces of lines.

With each hop, a change is made,

A new path for keys, beautifully laid.

Tests are written, to keep us on track,

Ensuring our code, has no crack.

So let's celebrate this change, so neat,

With a carrot cake, that's oh-so sweet! 🥕🍰


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Commits Files that changed from the base of the PR and between 0892944 and 6a07fdd commits.
Files selected for processing (2)
  • pandasai/llm/openai.py (2 hunks)
  • tests/llms/test_openai.py (1 hunks)
Additional comments (Suppressed): 2
pandasai/llm/openai.py (1)
  • 49-55: The constructor now accepts an additional optional parameter, api_key_path. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.
tests/llms/test_openai.py (1)
  • 31-33: Ensure that the proxy settings are correctly applied. The current test only creates an instance of OpenAI but does not verify if the proxy settings are used when making requests. Consider adding assertions to check this.

pandasai/llm/openai.py Show resolved Hide resolved
tests/llms/test_openai.py Show resolved Hide resolved
@gventuri
Copy link
Collaborator

@regmibijay thanks a lot for the change, merging :)

@gventuri gventuri merged commit e70f555 into Sinaptik-AI:main Sep 12, 2023
9 checks passed
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