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 req_chunk() #307

Closed
wants to merge 43 commits into from
Closed

Add req_chunk() #307

wants to merge 43 commits into from

Conversation

mgirlich
Copy link
Collaborator

@mgirlich mgirlich commented Sep 5, 2023

Closes #303.

@mgirlich
Copy link
Collaborator Author

mgirlich commented Sep 5, 2023

@hadley It would also make sense to add support for paginate_req_perform_continue() (see #306). Should we also add low level helpers? As this is pretty trivial I wasn't sure whether low level helpers are really worth it
It would also make sense to add some option to control what do to in case of errors.

R/chunk_req_perform.R Outdated Show resolved Hide resolved
R/chunk_req_perform.R Outdated Show resolved Hide resolved
R/chunk_req_perform.R Outdated Show resolved Hide resolved
R/chunk_req_perform.R Outdated Show resolved Hide resolved
R/chunk_req_perform.R Outdated Show resolved Hide resolved
R/chunk_req_perform.R Outdated Show resolved Hide resolved
Comment on lines 59 to 66
#' n <- length(requests)
#' paths <- character(n)
#' responses <- list()
#' for (i in seq2(1, n)) {
#' paths[[i]] <- tempfile()
#' responses[[i]] <- req_perform(requests[[i]], path = paths[[i]])
#' }
#' }
Copy link
Member

Choose a reason for hiding this comment

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

It's interesting to think about using multi_req_perform() here.

R/chunk_req_perform.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/req-perform-multi.R Outdated Show resolved Hide resolved
R/req-perform-multi.R Outdated Show resolved Hide resolved
@mgirlich
Copy link
Collaborator Author

@hadley Now req_chunk() and req_paginate() both use the new "multi" policy and can both be performed via the new req_perform_multi(). Overall, I think this goes in the right direction now.
What do you think of this now?

#'
#' responses <- req_perform_multi(req)
#' }
req_chunk <- function(req,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be nice to have a helper to create a list of all requests so that one can perform them in parallel.
This would also work for two pagination patterns (pagination via offset and via page index). But this should be handled in a separate PR.

},
cancel_on_error = cancel_on_error,
error_message = "When parsing response {i}.",
error_class = "httr2_parse_failure",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or would you prefer to have the same class when the request fails and when the parsing fails? I.e. <httr2_failure> in both cases.

out <- out[seq2(1, i)]
}

vctrs::list_unchop(out)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't work well in case cancel_on_error = FALSE and there actually is an error. Should we only unchop if cancel_on_error = TRUE?

Copy link
Member

Choose a reason for hiding this comment

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

Or only unchop the results corresponding to requests that succeeded?

@mgirlich mgirlich changed the title Add chunk_req_perform() Add req_chunk() Oct 2, 2023
#' )
#'
#' responses <- req_perform_multi(req_pokemon)
req_perform_multi <- function(req,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to think about the naming of functions. There currently is multi_req_perform() to perform a list of requests in parallel. It might make sense to rename this to something like reqs_perform_parallel() (though I'm not quite sure we want to add the prefix reqs).

@@ -0,0 +1,100 @@
#' Chunk a request
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or should this be Batch a request and the function name req_batch()?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like batch better. req_ is probably not quite the right prefix (since it doesn't return a request) but I don't see a better alternative.

#Conflicts:
#	NEWS.md
#	R/paginate.R
#	man/paginate_req_perform.Rd
#	man/req_paginate.Rd
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I'll do another review once we've sorted out #335 because I'm getting confused by the different names.

#' If you need more control use a combination of [req_perform()] and
#' [paginate_next_request()] to iterate through the pages yourself.
#'
#' [req_next_multi()] to iterate through the pages yourself.
Copy link
Member

Choose a reason for hiding this comment

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

Or should this be resp_next_request()?

@@ -0,0 +1,100 @@
#' Chunk a request
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like batch better. req_ is probably not quite the right prefix (since it doesn't return a request) but I don't see a better alternative.

out <- out[seq2(1, i)]
}

vctrs::list_unchop(out)
Copy link
Member

Choose a reason for hiding this comment

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

Or only unchop the results corresponding to requests that succeeded?

tryCatch(
f(...),
error = function(cnd) {
cli::cli_abort(
Copy link
Member

Choose a reason for hiding this comment

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

Fits on one line?

@hadley hadley mentioned this pull request Oct 23, 2023
4 tasks
@hadley
Copy link
Member

hadley commented Oct 24, 2023

Superseded by #353

@hadley hadley closed this Oct 24, 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.

Add helper to perform a request in chunks
2 participants