Initial try on tar_download #9

noamross · 2020-08-11T16:48:32Z

Prework

I understand and agree to this repository's code of conduct.
I understand and agree to this repository's contributing guidelines.
I have already submitted an issue to the issue tracker to discuss my idea with the maintainer.

Summary

This is an initial try at tar_download(), which builds on top of tar_change(). tar_download() creates a file target which will be downloaded if the remote URL's modified time or eTag is updated.

This is a draft as I'm still working through a bug understanding some NSE issues, but I thought I would show progress so far for comment.

Related GitHub issues and pull requests

Ref: #

Checklist

This pull request is not a draft.

noamross · 2020-08-11T18:52:20Z

Should I not be using tar_change() but rather building on top of tar_change_pair(), which I think is the equivalent of tar_target_raw()? @wlandau

wlandau

Thanks for the PR, @noamross! I really appreciate your help with this archetype, especially because you are one of the folks who is probably going to use it the most. I agree with most of what you sketched out. I think the issues in my comments and some new tests will get us most of the way there.

wlandau · 2020-08-11T20:35:36Z

DESCRIPTION

@@ -32,7 +32,8 @@ Suggests:
  digest (>= 0.6.25),
  knitr (>= 1.28),
  rmarkdown (>= 2.1),
-  testthat (>= 2.3.2)
+  testthat (>= 2.3.2),
+  curl


Would you add a minimum version for curl?

wlandau · 2020-08-11T20:37:20Z

R/tar_download.R

+  stop_on_no_internet = FALSE,
+  ...
+) {
+  if(!requireNamespace("curl", quietly = TRUE)) {


Would you use tarchetypes::assert_package("curl", "tar_download() requires the the package 'curl' to be installed")?

wlandau · 2020-08-11T20:37:57Z

R/tar_download.R

+    stop("tar_download() requires the the package 'curl' to be installed")
+  }
+  handle <- handle %||% curl::new_handle()
+  tar_change(


As you pointed out, we should use tar_change_pair().

wlandau · 2020-08-11T20:40:52Z

R/tar_download.R

+  destdir = ".",
+  handle = NULL,
+  stop_on_no_internet = FALSE,
+  ...


Let's consider making these arguments formal. For other archetypes, I found this useful so I could change the defaults in a way that users notice.

wlandau · 2020-08-11T20:41:51Z

R/tar_download.R

+#' @export
+tar_download_file <- function(url, destfile, destdir, stop_on_no_internet = FALSE, handle = curl::new_handle())
+  {
+  if (!curl::has_internet()) {


Other archetypes might require an internet connection, so it may be nice to put this in a new assert_internet() in R/utils_assert.R.

Your assertion patterns don't currently allow for "warning only" options, which I want here. (The use-case being that in the absence of a connection the workflow should be able to continue with current versions of files.) Do you want to incorporate that into your assertion setup or should I just do something conditional here?

I see. How about a validate_internet() utility?

validate_internet <- function(assert_internet = FALSE) { trn(assert_internet, try_cancel_internet(), assert_internet()) } try_cancel_internet <- function(msg = NULL) { if (!curl::has_internet()) { warn_validate("no internet") tar_cancel() } } warn_validate <- function(...) { warning(warning_validate(...)) } warning_validate <- function(...) { structure( list(message = paste0(..., collapse = ""), call = NULL), class = c( "condition_validate", "condition_tarchetypes", "warning", "condition" ) ) }

I still need to test for internet to determine whether to cancel the target. Should this return a logical value?

Honestly for me atm this seems like a way to turn 5 lines of readable code into 20 harder-to-understand lines.

I just updated the code above to include a call to tar_cancel().

There are more functions, but they help us avoid nested if/else logic, and I think each function might be reusable for other archetypes. And warn_validate() and warning_validate() support custom conditions for warnings, which make exception handling and testing nicer.

wlandau · 2020-08-11T20:42:36Z

R/tar_download.R

+      stop("No internet. Cannot check url: ", url)
+    else
+      warning("No internet. Cannot check url: ", url)
+      tar_cancel()


Could we namespace calls to targets functions?

wlandau · 2020-08-11T20:43:46Z

R/tar_download.R

@@ -0,0 +1,104 @@
+#' @title Download a file from a remote source, checking for changes
+#'   first
+#' @description Create a target that downnloads a file if it has changed since


Just flagging this for proofreading when it comes time to polish things up.

wlandau · 2020-08-11T20:47:47Z

Should I not be using tar_change() but rather building on top of tar_change_pair(), which I think is the equivalent of tar_target_raw()? @wlandau

Exactly, the NSE issues you faced are the exact reason for tar_target_raw(). tar_change_pair() should probably be called tar_change_raw().

wlandau · 2020-08-12T18:54:15Z

R/tar_download.R

+tar_download <- function(
+  name,
+  url,
+  destfile = basename(url),


Do we need both destdir and destfile arguments? What about a single path argument that defaults to basename(url)?

The reason I like this split, is that, for me, it's a common pattern to download a lot of files from different sources to a data/ or downloads/ (or data/downloads), so this makes an easy pattern to iterate over and conceptually I separate where I put something from it's name. I don't feel strongly about it but a lot of functions split these concepts (e.g., rmarkdown::render).

wlandau · 2020-08-12T18:54:34Z

R/tar_download.R

+  destfile = basename(url),
+  destdir = ".",
+  handle = NULL,
+  stop_on_no_internet = FALSE,


Maybe assert_internet instead of stop_on_no_internet?

wlandau · 2020-08-14T04:04:07Z

Also, please feel free to add yourself as a contributor in the DESCRIPTION.

wlandau · 2020-09-15T03:11:09Z

On reflection, I think we might be better off thinking about input URLs as formats: ropensci/targets#154.

wlandau · 2020-09-15T04:12:03Z

Though if we do move to a format instead of an archetype, I'm not sure what to do about the curl handles. Not sure it's worth adding an option in tar_option_get() for something so specific.

wlandau · 2020-09-15T14:41:47Z

Yup, tar_download() still seems worth it because tar_target(format = "url") will not be able to accept custom curl handles.

wlandau · 2020-09-18T15:02:03Z

@noamross, after some reflection and refactoring, I decided to allow custom curl handles for the built-in format = "url" through the resources argument.

tar_target(
  url,
  "https://httpbin.org/etag/test",
  format = "url",
  resources = list(handle = curl::new_handle(...))
)

So I think the functionality in the current PR is now covered directly in targets.

I built URLs directly into targets to allow easier dynamic branching across URLs. So what do you think about a version of tar_download() that is shorthand for this?

list(
  tar_files(urls, rep("https://httpbin.org/etag/test", 2), format = "url"),
  tar_target(downloads, download.file(urls), pattern = map(urls))
)

Combine with some HPC, this could make parallel downloads a lot easier.

wlandau · 2020-10-17T22:03:58Z

Sorry, I didn't mean to close your PR @noamross. I don't know why that happened, and I don't know why I can't reopen it.

wlandau · 2020-10-17T22:07:35Z

I definitely don't remember closing it, especially not within the last few days, and I 100% meant to keep it open because of the alternative direction available to tar_download(). Looks like there is no commit hash tied to the "closed" message, so it wasn't closed due to a typo in a commit message. This is so odd.

petrbouchal · 2021-03-28T12:22:53Z

Just to say I would appreciate having this in {tarchetypes} - I almost started implementing my own before discovering this. By now this is one of the few archetypes/helpers I miss and would use frequently, ideally in conjunction with the new tar_timestamp().

Initial try on tar_download

91d1373

wlandau requested changes Aug 11, 2020

View reviewed changes

wlandau reviewed Aug 12, 2020

View reviewed changes

wlandau mentioned this pull request Sep 8, 2020

Target archetype that stores to S3 #11

Closed

3 tasks

This was referenced Sep 15, 2020

format = "url" ropensci/targets#155

Closed

format = "url" for remote data inputs ropensci/targets#156

Merged

wlandau closed this Oct 13, 2020

wlandau mentioned this pull request Oct 17, 2020

targets and tarchetypes ropensci/software-review#401

Closed

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial try on tar_download #9

Initial try on tar_download #9

noamross commented Aug 11, 2020

noamross commented Aug 11, 2020

wlandau left a comment

wlandau Aug 11, 2020

wlandau Aug 11, 2020

wlandau Aug 11, 2020

wlandau Aug 11, 2020

wlandau Aug 11, 2020

noamross Aug 12, 2020

wlandau Aug 12, 2020 •

edited

Loading

noamross Aug 12, 2020

noamross Aug 12, 2020

wlandau Aug 13, 2020

wlandau Aug 11, 2020

wlandau Aug 11, 2020

wlandau commented Aug 11, 2020

wlandau Aug 12, 2020

noamross Aug 12, 2020

wlandau Aug 12, 2020

wlandau commented Aug 14, 2020

wlandau commented Sep 15, 2020

wlandau commented Sep 15, 2020

wlandau commented Sep 15, 2020 •

edited

Loading

wlandau commented Sep 18, 2020 •

edited

Loading

wlandau commented Oct 17, 2020

wlandau commented Oct 17, 2020

petrbouchal commented Mar 28, 2021

Initial try on tar_download #9

Initial try on tar_download #9

Conversation

noamross commented Aug 11, 2020

Prework

Summary

Related GitHub issues and pull requests

Checklist

noamross commented Aug 11, 2020

wlandau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlandau Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlandau commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlandau commented Aug 14, 2020

wlandau commented Sep 15, 2020

wlandau commented Sep 15, 2020

wlandau commented Sep 15, 2020 • edited Loading

wlandau commented Sep 18, 2020 • edited Loading

wlandau commented Oct 17, 2020

wlandau commented Oct 17, 2020

petrbouchal commented Mar 28, 2021

wlandau Aug 12, 2020 •

edited

Loading

wlandau commented Sep 15, 2020 •

edited

Loading

wlandau commented Sep 18, 2020 •

edited

Loading