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.

Sign up for GitHub

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

Jump to bottom

fix(connectors): make sqlalchemy non-optional and check yfinance imports #561

Merged
merged 1 commit into from
Sep 14, 2023
Merged

fix(connectors): make sqlalchemy non-optional and check yfinance imports #561

merged 1 commit into from
Sep 14, 2023

Conversation

mspronesti
Copy link
Contributor

@mspronesti mspronesti commented Sep 14, 2023 •
edited by coderabbitai bot
Loading

Hi @gventuri,
this PR aims at solving #558. Please notice however that there are something that don't add up to me in the implementation of yahoo finance and that it might be the case to check (unrelated to this PR, perhaps in another?).

Summary by CodeRabbit

Refactor:

  • Enhanced the usage of yfinance package in yahoo_finance.py. The changes ensure that a Ticker object is created only once during initialization, improving performance and reducing unnecessary object creation.
  • Added error handling for the absence of yfinance package. This provides a clear error message if the required package is not installed.

Bug Fix:

  • Fixed an issue where multiple Ticker objects were unnecessarily created in the head and execute methods. Now, these methods utilize the Ticker object created during initialization, leading to more efficient code execution.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 14, 2023 •
edited
Loading

Walkthrough

The changes primarily focus on optimizing the usage of yfinance package in the yahoo_finance.py file. The code now checks for the availability of the package, and reuses a single Ticker object instead of creating new ones, enhancing performance and maintainability.

Changes

File Summary
.../connectors/yahoo_finance.py Removed redundant import statement. Added check for yfinance package availability. Optimized __init__, head, and execute methods to reuse a Ticker object.

🐇💻

"In the land of code where the shadows lie,

We refactor, optimize, under the cloudless sky.

No more redundant imports, no more needless strain,

Just one Ticker object, to rule the domain!

So here's to the coder, with their keyboard might,

Making software better, byte by byte." 🎉🥕

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.

coderabbitai[bot]
coderabbitai bot reviewed Sep 14, 2023
View reviewed changes
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 ebb4c02 and 3aabd24 commits.
Files ignored due to filter (1)
  • pyproject.toml
Files selected for processing (1)
  • pandasai/connectors/yahoo_finance.py (5 hunks)
Additional comments (Suppressed): 5
pandasai/connectors/yahoo_finance.py (5)

  • 1-4: The import statement for yfinance has been removed from the top of the file. This is in line with the PR summary and seems to be a good practice to ensure that the code fails gracefully when the required package is not installed.

  • 16-23: The __init__ method now includes a try-except block to check for the availability of the yfinance package and raises an error if it's not installed. This is a good practice as it ensures that the code fails gracefully when the required package is not installed.

  • 36-36: The Ticker object from yfinance is now initialized in the __init__ method using the _config.table value, and stored in self.ticker. This change is in line with the PR summary and seems to be a good practice as it allows the Ticker object to be reused across methods, reducing unnecessary overhead and potential inconsistencies.

  • 46-47: The head method now uses the Ticker object stored in self.ticker to retrieve the head data. This is a good practice as it reuses the Ticker object initialized in the __init__ method, reducing unnecessary overhead and potential inconsistencies.

  • 114-117: The execute method now uses the Ticker object stored in self.ticker to retrieve the stock data. This is a good practice as it reuses the Ticker object initialized in the __init__ method, reducing unnecessary overhead and potential inconsistencies.

@gventuri
Copy link
Collaborator

@mspronesti thanks a lot, definitely better. Glad to discuss about the yahoo finance implementation. Feel free to open a discussion or an issue for that!

@gventuri gventuri merged commit aad466d into Sinaptik-AI:main Sep 14, 2023
9 checks passed
gventuri pushed a commit that referenced this pull request Sep 18, 2023
@nautics889
feat: add output_type parameter (#519) (#562)
69e2b14 
* fix(connectors): make sqlalchemy non-optional and check yfinance imports (#561)

* feat: use duckdb for the cache

* Release v1.2.2

* feat: `output_type` parameter (#519)

* (feat): update prompt template in `GeneratePythonCodePrompt`, add
  `output_type_hint` variable to be interpolated
* (feat): update `.chat()` method for `SmartDataframe` and
  `SmartDatalake`, add optional `output_type` parameter
* (feat): add `get_output_type_hint()` in `GeneratePythonCodePrompt`
  class
* (feat): add "output_type_hint" to `default_values` when forming
  prompt's template context
* (tests): update tests in `TestGeneratePythonCodePrompt`
* (tests): add tests for checking `output_type` interpotaion to a
  prompt

* refactor: `output_type` parameter (#519)

* (refactor): update setting value for `{output_type_hint}` in prompt
  class

* fix: `output_type` parameter (#519)

* (tests): fix error in `TestGeneratePythonCodePrompt` with confused
  actual prompt's content and excepted prompt's content (which led to
  tests being failed)
* (refactor): update test method `test_str_with_args()` with
  `parametrize` decorator, remove duplication of code (DRY)

* tests: `output_type` parameter (#519)

* (tests): parametrizing `test_run_passing_output_type()` with different
  output types.

* refactor: `output_type` parameter (#519)

* (refactor): move output types and their hints to a separate python
  module
* (feat): validation for inappropriate output type and value
* (tests): update tests accordingly, add a new test method to check if
  the message about incorrect output type is added to logs

* chore: `output_type` parameter (#519)

* (chore): remove unused lines

* refactor: `output_type` parameter (#519)

* (refactor): pass `output_type_hint` in the `default_values`, don't
  bother with setting this template variable in prompt's `__init__()`
* (fix): correct templates examples, add them as a part of
  `output_type_hint`
* (refactor): use `df_type()` (utility functions) to get rid from
  imports of third packages in _output_types.py
* (refactor): add logging to the factory-function
  `output_type_factory()` to enhance verbosity for behaviour inspection

---------
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Try-catch Yahoo Finance and SQLAlchemy imports
2 participants
@mspronesti @gventuri