-
Notifications
You must be signed in to change notification settings - Fork 159
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
Auto polygon has paid #476
Conversation
My review is in progress 📖 - I will have feedback for you in a few minutes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed your code and found 12 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
@@ -91,5 +91,4 @@ def on_trading_iteration(self): | |||
backtesting_end, | |||
benchmark_asset="SPY", | |||
polygon_api_key="YOUR_POLYGON_API_KEY_HERE", # Add your polygon API key here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security issue identified: The Polygon API key is hardcoded directly in the code as 'YOUR_POLYGON_API_KEY_HERE'. Hardcoding sensitive information like API keys is a security risk. If an attacker gains access to the codebase, they could extract the API key. To resolve this, remove the API key from the code and instead read it from an environment variable or a separate configuration file that is not committed to version control.
@@ -189,7 +196,7 @@ def get_price_data_from_polygon( | |||
|
|||
return df_all | |||
|
|||
def validate_cache(force_cache_update: bool, asset: Asset, cache_file: Path, api_key: str, paid: bool): | |||
def validate_cache(force_cache_update: bool, asset: Asset, cache_file: Path, api_key: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,6 +1,6 @@ | |||
import datetime | |||
import logging | |||
import warnings | |||
from termcolor import colored | |||
from asyncio.log import logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -773,7 +773,7 @@ def run_backtest( | |||
sell_trading_fees=[], | |||
api_key=None, | |||
polygon_api_key=None, | |||
polygon_has_paid_subscription=False, | |||
polygon_has_paid_subscription=None, # Depricated, this is now automatic. Remove in future versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colored_message = colored( | ||
"Polygon rate limit reached. Sleeping for 1 minute before trying again. If you want to avoid this, consider a paid subscription with Polygon at https://polygon.io/?utm_source=affiliate&utm_campaign=lumi10 Please use the full link to give us credit for the sale, it helps support this project. You can use the coupon code 'LUMI10' for 10% off.", | ||
"red", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Polygon rate limit reached. Sleeping for 1 minute before trying again. If you want to avoid this, consider a paid subscription with Polygon at https://polygon.io/?utm_source=affiliate&utm_campaign=lumi10 Please use the full link to give us credit for the sale, it helps support this project. You can use the coupon code 'LUMI10' for 10% off.", | ||
"red", | ||
) | ||
logging.error(colored_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -951,6 +948,17 @@ def run_backtest( | |||
if stats_file is None: | |||
stats_file = f"{logdir}/{basename}_stats.csv" | |||
|
|||
# Check if polygon_has_paid_subscription is set (it is deprecated and will be removed in the future) | |||
if polygon_has_paid_subscription is not None: | |||
colored_warning = colored("The parameter `polygon_has_paid_subscription` is deprecated and will be removed in the future. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,6 +1,6 @@ | |||
import datetime | |||
import logging | |||
import warnings | |||
from termcolor import colored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
def _get(self, *args, **kwargs): | ||
logging.info( | ||
f"\nSleeping {PolygonClient.WAIT_SECONDS} seconds while getting data from Polygon " | ||
"to avoid hitting the rate limit; " | ||
"consider a paid Polygon subscription for faster results.\n" | ||
"If you want to avoid this, consider a paid subscription with Polygon at https://polygon.io/?utm_source=affiliate&utm_campaign=lumi10 (please use the full link to give us credit for the sale, it helps support this project). You can use the coupon code 'LUMI10' for 10% off.\n" | ||
) | ||
time.sleep(PolygonClient.WAIT_SECONDS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force_cache_update = validate_cache(force_cache_update, asset, cache_file, api_key, has_paid_subscription) | ||
|
||
force_cache_update = validate_cache(force_cache_update, asset, cache_file, api_key) | ||
df_all = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -123,7 +120,7 @@ def get_price_data_from_polygon( | |||
# Check if we need to get more data | |||
missing_dates = get_missing_dates(df_all, asset, start, end) | |||
if not missing_dates: | |||
# TODO: Do this upstream so we don't called repeatedly for known-to-be-missing bars. | |||
# TODO: Do this upstream so we don't repeatedly call for known-to-be-missing bars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -5,7 +5,7 @@ | |||
|
|||
setuptools.setup( | |||
name="lumibot", | |||
version="3.5.8", | |||
version="3.5.9", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the version number of the package has been incremented in the setup.py file. However, there is no information in the pull request summary about what changes have been made that justify this version increment. Could you please update the pull request summary to include this information? This will help to ensure that the versioning of the package is managed properly.
No description provided.