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 backfill study function #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

clementzach
Copy link

@clementzach clementzach commented Oct 17, 2022

This PR adds a helper function that allows a user to easily download data from a whole study.

One question: Is there a reason that BACKFILL_WINDOW is set to be 5? It seems like that slows down the process of downloading data a good amount.

In my experience, we should be able to have a default value of BACKFILL_WINDOW that is at least 100 or so.

@biblicabeebli
Copy link
Member

@tokeetim would you like me to do some review here to speed the process along?

@tokeefe
Copy link
Contributor

tokeefe commented Oct 18, 2022

@biblicabeebli Yes, please! Thank you!

@biblicabeebli
Copy link
Member

Crap I just finished the modernizing/cleanup/python-3.8-fixes refactor I was working on and then looked at this. I totally forgot this pull existed.

@clementzach the refactor for the sync file was just to move it to a file called sync (should maintain import compatibility) from the /sync/__init__.py. I could probably pull in your code additions in manually but that then it wouldn't be attributed to you. Would you like me to do that (because it's been..... a while....) or would you like to handle the merge/rebase?

I'm still new to this codebase, I will start using it, I don't know why a longer window would speed up/slow down requests. One change I did make internally that may affect speed was to up an internal chunking value. Otherwise, I intend to try and get into this code a little more for speed and, eventually, a v2 download endpoint that pulls actually-compressed data.

(it was once compressed! ... but there was a server-destroying memory not-exactly-a-leak-but-kinda-a-leak and I had to turn it off...)

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