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 session to download #1547

Merged
merged 1 commit into from
Jun 6, 2023
Merged

add session to download #1547

merged 1 commit into from
Jun 6, 2023

Conversation

bveber
Copy link
Contributor

@bveber bveber commented Jun 6, 2023

Allows user to pass a session object to the download method.

Fixes #1534

@bveber
Copy link
Contributor Author

bveber commented Jun 6, 2023

I have it working locally, but I'm not seeing the expected behavior from the sample code with the custom CachedLimiterSession class. I was expecting for a cache hit to bypass the rate-limiter, but in my testing I don't see any speedup for cache hits. I have verified the caching works without the rate-limiter (using only requests_cache.CacheMixin) but when I include requests_ratelimiter.LimiterMixin my response time is always bottlenecked by the limiter.

My problem is obviously external to this package but I'm curious if you've seen the same behavior.

@ValueRaider
Copy link
Collaborator

ValueRaider commented Jun 6, 2023

Sounds like logging should be able to help with that. Look how yfinance already uses logging via logger, and have it log the url + params to "debug". If url is changing then it will never hit cache. Can combine with this PR.

@bveber
Copy link
Contributor Author

bveber commented Jun 6, 2023

I think I resolved my caching issue. In my download method calls I was using the default end parameter which sets that value to the current time which bumps against this conditional check. Setting the end date to some value more than 30 minutes ago seems to have fixed it.

@ValueRaider ValueRaider merged commit c20211a into ranaroussi:dev Jun 6, 2023
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.

None yet

2 participants