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

WIP - Cache #124

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

WIP - Cache #124

wants to merge 7 commits into from

Conversation

TristanFecteau
Copy link
Contributor

Closes #111

left to do:
data_source_fmp.get_historical_daily_prices()
TESTS
get_moments is now in market_utils (not only used in runner)
is_market_open is now ONLY in market_utils (it was duplicated in runner)
changed get_prices to allow date query and delta query and to work with the cache functionnalities
cache already gets tested with ourt test_strategy methods
@TristanFecteau TristanFecteau added the task Regular piece of work label Mar 29, 2022
@TristanFecteau TristanFecteau added this to the MVP 2 milestone Mar 29, 2022
@TristanFecteau TristanFecteau self-assigned this Mar 29, 2022
@@ -1,3 +1,5 @@
from bullets.utils.market_utils import is_market_open

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the blank line here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

from datetime import date, timedelta

from bullets.utils.market_utils import is_market_open, get_date_in_x_market_days_away, get_moments

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also remove the blank lines here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

start_date = start_timestamp

if end_timestamp is None and delta is None:
raise ValueError("End timestamp and delta cannot be None, one or the other has to be entered.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we maybe have a default delta of 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, changed


prices = []
moments = []
has_stored = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_stored seems like a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

from bullets.data_storage.endpoints.cache_statement import CacheStatement


class CacheEndpoint(Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure CacheEndpoint is the best name for this enum, it might make people think that we have endpoint with prices cached. Maybe LocalCache would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally right, changed

also added null checking to our main store method for prices
@SamiMammouche SamiMammouche changed the title Cache WIP Cache May 15, 2022
@SamiMammouche SamiMammouche changed the title WIP Cache WIP - Cache May 15, 2022
else:
stock_date = datetime.strptime(price_point.date, "%Y-%m-%d %H:%M:%S")

if stock_date == timestamp:
Copy link
Contributor

@laurentfrenette laurentfrenette May 27, 2022

Choose a reason for hiding this comment

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

Ici on fait une recherche O(n) pour trouvé la donné qui match la date dans le cache.

Présentement le cache est stocké un peu n'importe comment pour un stock, c'est-à-dire sans être trié par date. Si on insérait la donné au bon endroit quand on écrit dans le cache, ça faciliterait énormement la recherche puisqu'on pourrait faire une recherche binaire (O(log(n))).

En ce moment y'a pas beaucoup de gain, mais imaginons qu'on a des millions de donné en cache, ça augmenterait pas mal la performance de la recherche dans le cache
@TristanFecteau

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactement, aussi, on pourrait enregistrer les valeurs nulles de nos requêtes et les mettre dans la cache, comme ça on aurait pas à re-requêter la cache lorsqu'une valeur nulle n'est pas trouvée. Ex : on requête fmp du 6 janvier au 17 janvier et le 12 et le 13 le marché est fermé, donc présentement on aura pas de valeur en cache pour le 12 et le 13 et donc la stratégie va re-requêter fmp pour le 12 et le 13 durant son exécution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent! Bon point.

Je vais créer une story sous l'epic cache pour ça

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Regular piece of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework caching
3 participants