-
Notifications
You must be signed in to change notification settings - Fork 3
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
Read/write in parallel #91
Conversation
R/stores.R
Outdated
# For some reason, the crul::HttpClient fails in parallel settings | ||
# (when used inside foreach %dopar% loops). This alternative | ||
# with HttpRequest and AsyncVaried seems to work. | ||
# Reference: https://docs.ropensci.org/crul/articles/async.html |
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.
Is there maybe an argument to switch to a more traditional http client here? Seems like there's a lot of complexity coming together.
Maybe another option is to have one simple pathway for metadata gets and another non-caching parallelized get utility for iterating over chunks?
Looks like a great step forward. Big question is whether foreach is the right parallelization option? I've gotten a lot of mileage out of pbapply, which offers an out of the box progress bar for *apply functions. Looks like you are just doubling down on doParallel? Looks like they haven't had any development in two years and he last change was a package maintainer update. The other thing is what this ends up looking like from a user-facing API and UX point of view. It would be nice to havea vignette showing what the expected use pattern is. Once this is merged, maybe I can work that up as a test of what you did here? |
I am not tied to
Yes definitely we will need documentation of the usage. The tests are currently the only place that shows this pizzarr/tests/testthat/test-parallel.R Line 50 in b978a5c
|
Cool -- I'm not attached to any one parallelization scheme but it does seem that |
Unfortunately, the switch from
crul::HttpClient
tocrul::HttpRequest + crul::AsyncVaried
breaks the VCR request mocking due to ropensci/vcr#246Fixes #83
Related to #31
To get tests passing again, depends on ropensci/crul#180