hit_api

[mikemahoney218]

hit_national_map_api.R

* I suggest using `httr::RETRY("GET", ...)`  rather then a tryCatch and counters, this will greatly simplify your logic. For example:

#Set UP
url = httr::parse_url("https://elevation.nationalmap.gov/arcgis/rest/services/3DEPElevation/ImageServer/exportImage")

query_arg <- list(
      bboxSR = 4326,
      imageSR = 4326,
      size = paste(8000, 8000, sep = ","),
      format = "tiff",
      pixelType = "F32",
      noDataInterpretation = "esriNoDataMatchAny",
      interpolation = "+RSP_BilinearInterpolation",
      f = "json"
    )

## 
first_corner <- bbox@bl
second_corner <- bbox@tr

bbox_arg <- list(bbox = paste(
    min(first_corner@lng, second_corner@lng),
    min(first_corner@lat, second_corner@lat),
    max(second_corner@lng, first_corner@lng),
    max(second_corner@lat, first_corner@lat),
    sep = ","
  ))

####======
#^all the above could be replaces with paste(st_bbox(...), collapse = ",")

You have a lot of logic wrapping your httr calls which is essentially:

res <- httr::GET(url, query = c(bbox_arg, query_arg))
content = httr::content(res, type = "application/json")

An identical, but way to build in some of that error catching and retrying would be to change it to:

res2 <- httr::RETRY("GET", url, 
                   query = c(bbox_arg, query_arg), 
                   times = 10, pause_cap = 10)

[mikemahoney218]

I am almost entirely certain that httr::RETRY never returns for one of these endpoints, for reasons I never got to the bottom of -- worth finding the commit instead of running into the same issue again

[mikemahoney218]

Found the commit at least, but past me was not kind enough to leave comments about the issue
f535f4b

[mikemahoney218]

Ok, found (part of) why I removed httr::RETRY initially. There's a real performance impact when using RETRY for reasons I don't entirely understand -- there's a huge performance hit to RETRY vs my manual backoff implementation, and RETRY occasionally fails with very unfriendly (to me and the debugger) stacks (while it's downright rare to see similar failures in the current version)

[mikemahoney218]

(top is retry rewrite, bottom is mine)

[mikemahoney218]

Implemented httr::RETRY on branch:
https://github.com/mikemahoney218/terrainr/tree/httr_retry

CI has not been kind to this implementation (which I'm legitimately surprised by, since "it ran on my machine"). Notably, the tests are failing on different steps -- the code essentially "works", just is incredibly flaky:

• NAIP server, code line 3
• NAIP server, code line 23

Even ignoring the test suite failures, it's worth noting the time difference between the partial test suite runs with httr::RETRY versus without. I pushed a quick commit to reviewer_comments in order to get closer comparisons for the timing (adjust for server load etc) -- while it feels rude to benchmark someone else's public API, I'm fine with engineering a "natural experiment" :)

The timings below only reflect R CMD check, which isn't cached between builds.

• Partial test suite runs (httr::RETRY):
  • 4 min 5s
  • 4 min 27s
  • 4 min 11s
  • At the moment, the only suite to not fail is already over 17 minutes.
• Full test suite runs (manual backoff):
  • 1 min 53s
  • 2 min 14s
  • 2 min 9s
  • 3 min 5s

I run the same backoff protocol as httr does, so it shouldn't be related to that.

I'm pretty sure that part of the reason for both the additional failures and the increased run time is that get_href is solving for both res and body -- the two combined get 15 tries, as if the first stage fails there's no point in advancing to the second. That said, you still see a significant time hit just by changing the last GET to RETRY (which really should be an in-place substitution) so 🤷 I honestly don't know. I'm going to stick with the thing that works.