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

Default is_chat_model to True in AzureOpenAI #563

Merged
merged 3 commits into from
Sep 17, 2023

Conversation

mspronesti
Copy link
Contributor

@mspronesti mspronesti commented Sep 15, 2023

Hi @gventuri,
as OpenAI has marked the completion API as "legacy" (to be deprecated within 9 months) I have defaulted is_chat_completion to True in AzureOpenAI. Also, please notice that the newest subscription in Azure OpenAI no longer allow to deploy completion-only models (such as davinci) and that the newest versions of turbo and gpt-4 (0613) no longer support the completion API.

Summary by CodeRabbit

Refactor:

  • Modified the __init__ method in azure_openai.py. The is_chat_model parameter is now required and defaults to True, enhancing clarity and predictability when creating an instance of the class.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 15, 2023

Walkthrough

The changes primarily focus on optimizing the usage of yfinance.Ticker object in yahoo_finance.py and modifying the default value of is_chat_model parameter in azure_openai.py. The modifications enhance code efficiency and enforce explicit parameter provision, respectively.

Changes

File Path Summary
.../connectors/yahoo_finance.py Optimized by importing yfinance package within the __init__ method and reusing yfinance.Ticker object across methods.
.../llm/azure_openai.py Modified is_chat_model parameter from optional to required with a default value of True.

🐇💻

In the land of code, where logic intertwines,

Changes are made, as the moonlight shines.

Efficiency found, in the reuse of objects,

And parameters tweaked, for better prospects.

Celebrate we must, these improvements small,

For they make a difference, after all! 🎉🥕


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: 0

Commits Files that changed from the base of the PR and between 0eca724 and 27e52a9 commits.
Files ignored due to filter (1)
  • pyproject.toml
Files selected for processing (2)
  • pandasai/connectors/yahoo_finance.py (5 hunks)
  • pandasai/llm/azure_openai.py (1 hunks)
Additional comments (Suppressed): 6
pandasai/llm/azure_openai.py (1)
  • 43-43: The is_chat_model parameter in the __init__ method has been changed from an optional boolean with a default value of False to a required boolean that defaults to True. This change implies that unless explicitly specified, all instances of AzureOpenAI will be treated as chat models by default. Ensure that this change is compatible with all existing uses of the AzureOpenAI class throughout the codebase.
pandasai/connectors/yahoo_finance.py (5)
  • 1-4: The import of yfinance has been removed from the top of the file and moved to the __init__ method. This change is likely made to handle the ImportError exception in a more controlled manner.

  • 16-23: The import of yfinance package is now inside a try-except block within the __init__ method. This is a good practice as it allows for handling the ImportError exception if the yfinance package is not installed, providing a clear error message to the user.

  • 36-36: The creation of the yfinance.Ticker object has been moved to the __init__ method. This allows the self.ticker attribute to be reused in both the head and execute methods, eliminating the need for creating a new yfinance.Ticker object each time these methods are called. This is a good practice as it improves code efficiency and readability.

  • 46-47: The head method now uses the self.ticker attribute to call the history method instead of creating a new yfinance.Ticker object. This is an improvement as it reuses the self.ticker attribute created during the initialization of the class, improving code efficiency.

  • 114-117: Similar to the head method, the execute method now uses the self.ticker attribute to call the history method instead of creating a new yfinance.Ticker object. This is an improvement as it reuses the self.ticker attribute created during the initialization of the class, improving code efficiency.

@codecov-commenter
Copy link

Codecov Report

Merging #563 (dc156b9) into main (0eca724) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main     #563   +/-   ##
=======================================
  Coverage   83.42%   83.42%           
=======================================
  Files          52       52           
  Lines        2594     2594           
=======================================
  Hits         2164     2164           
  Misses        430      430           
Files Changed Coverage Δ
pandasai/llm/azure_openai.py 89.36% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gventuri
Copy link
Collaborator

Thanks a lot for the fix @mspronesti, merging! So basically in a few months we'll need to remove the completion function both from OpenAI and from AzureOpenAI, I suppose!

@gventuri gventuri closed this Sep 17, 2023
@gventuri gventuri reopened this Sep 17, 2023
@gventuri gventuri merged commit 36d265a into Sinaptik-AI:main Sep 17, 2023
18 checks passed
gventuri pushed a commit that referenced this pull request Sep 25, 2023
* fix(connectors): make sqlalchemy non-optional and check yfinance imports

* chore(llm): default is_chat_model to True on AzureOpenAI
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.

3 participants