Enable paging for betydb functions #94

dlebauer · 2017-04-14T21:30:55Z

This PR enables paging for betydb_* functions in order to avoid server timeout.

Description

The BETYdb API is fairly slow (PecanProject/bety#419), and with very large requests (> 500k records) functions can fail due to server timeouts. Although this is not an issue for the default database (betydb.org) it is becoming an issue for other instances of BETYdb.

These changes use a for loop to divide large requests into smaller requests (currently in chunks of 5000 rows) using the limit and offset parameters to iterate through.

Related Issue

slow queries via API Queries slow on millions of records PecanProject/bety#419
paging through: API for periodic BETYdb Import terraref/reference-data#111

Example

I've added new tests just to make sure that we are getting the right number of requested records! More examples can be seen in the tests file.

Here is an example I've used to test the effect of limit on timings:

options(
  betydb_url = "https://www.betydb.org/",
  betydb_api_version = "beta")

limits <- c(1, 10, 50, 100, 500, 1000, 5000, 10000)
times <- list()
for(i in seq_along(limits)){
  times[[i]] <- data.frame(limit = limits[i], time = as.vector(system.time(betydb_search("SLA", limit = limits[i]))['elapsed']))
} 

times <- dplyr::bind_rows(times)

library(ggplot2)
ggplot(data = times)+
  geom_point(aes(limit, time)) + 
  scale_x_log10('limit (# records)') + scale_y_log10('time (s)') + 
  theme_bw()

update fork

…in order to prevent 504 timeout errors

dlebauer · 2017-04-14T21:31:19Z

@infotroph please review

Merge branch 'master' of https://github.com/dlebauer/traits Conflicts: tests/testthat/test-betydb.R

codecov-io · 2017-04-14T22:06:47Z

Codecov Report

Merging #94 into master will increase coverage by 8.63%.
The diff coverage is 89.02%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   15.44%   24.08%   +8.63%     
==========================================
  Files          17       17              
  Lines         602      681      +79     
==========================================
+ Hits           93      164      +71     
- Misses        509      517       +8

Impacted Files	Coverage Δ
R/betydb.R	`93.49% <89.02%> (-3.18%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63e9916...3cb72fd. Read the comment docs.

dlebauer · 2017-04-14T22:17:58Z

@sckott I would appreciate any advice on writing better tests ...

I expected that the tests I wrote would cover most of the code that I wrote and was pretty surprised to these results https://codecov.io/gh/ropensci/traits/pull/94/src/R/betydb.R that seem to indicate most of the new lines in betydb.R are not covered.

sckott · 2017-04-17T16:10:55Z

you don't have any requests with limit > 200 correct? seems like that block of lines 179-224 only invoked if limit > 200

dlebauer · 2017-04-17T16:31:18Z

aha. Thanks ... I was trying to minimize the testing time; will revise accordingly

sckott · 2017-04-17T16:34:26Z

def. do minimize testing time, just having one test that hits that bit of the code is good i think

updated tests to use smaller per-call limit

change warning to message tests: fixed errors testing API responses

infotroph

Apologies for incomplete comments -- I'll need to come back to this, but submitting now to save my comments

infotroph · 2017-04-18T09:19:15Z

R/betydb.R

-  lst <- jsonlite::fromJSON(txt, simplifyVector = TRUE, flatten = TRUE)
+
+  api_version <- ifelse(grepl('/beta/api', url), 'beta', 'v0')
+  if(!exists('per_call_limit')) {


Is the intention to test whether ... contains a parameter named per_call_limit? I don't think this will do that -- parameters in ... aren't assigned into the function environment.

actually, the whole reason I added the per_call_limit as a parameter rather than a fixed number is so that it can be set to a smaller number in the testing. So in the testing I make per_call_limit <<- 20 a global variable ...

5000 is a reasonable limit when the package is being used and 20 is sufficient for testing

It is kind of hacky, but is the difference b/w tests that take seconds vs minutes (e.g. so that I can test with iterations of [0,1,n] and remainders [0,n].

infotroph · 2017-04-18T09:28:01Z

R/betydb.R

+    if(args$limit <= per_call_limit){
+      txt <- betydb_http(url, args, key, user, pwd, ...)
+      lst <- jsonlite::fromJSON(txt, simplifyVector = TRUE, flatten = TRUE)
+    } else if (args$limit > per_call_limit){ # divide large requests (aka page)


This could be could be an unconditional else.

This approach makes it more clear and explicit the case under which we begin paging. the final else on line 264 below captures if args$limit captures cases where !args$limit <= per_call_limit | args$limit > per_call limit. (e.g. if args$limit can't be coerced to a numeric value)

infotroph · 2017-04-18T09:41:18Z

tests/testthat/test-betydb.R

  expect_is(get.out, "response")
+  expect_true(grepl("OK", get.out[["headers"]]$status))


Or expect_match(httr::headers(get.out)$status, "OK" ) ?

infotroph · 2017-04-18T09:49:10Z

tests/testthat/test-betydb.R

+    betydb_api_version = "beta",
+    betydb_key = "eI6TMmBl3IAb7v4ToWYzR0nZYY07shLiCikvT6Lv",
+    warn=-1 ## suppress warnings that we did not get all data
+  )


Beware that option changes are NOT local to this test block -- they'll stay in effect for other tests until the end of the file. See test_that("URL & version options work" above for one approach to cleanup.

replaced w/ on.exit(reset_opts(opts))

infotroph · 2017-04-18T10:10:47Z

tests/testthat/test-betydb.R

+  betydb_url = "https://www.betydb.org/",
+  betydb_api_version = "v0",
+  warnings = 0)
+


Ah, OK. This works, but on.exit(reset_opts(opts)) is still arguably safer: It both resets all previous options, and unsets any newly set options that didn't exist before this test.

recent changes to allow paging

better opts handling change from `expect_true(grepl ...` to `expect_match`

Include 'betydb_experiment()', exactly the same as 'betyb_site()', but replaces 'sites' with 'experiments' endpoint

Include method for querying experiment by id

Fix betydb_experiment() function

Paging fix

dlebauer · 2017-10-23T19:59:07Z

@sckott with the latest updates this branch is now passing. Do you have a schedule for pushing new versions to CRAN? Would be nice to be able to use a stable release in the training materials I am writing.

sckott · 2017-10-23T21:28:56Z

we can target for next milestone https://github.com/ropensci/traits/milestone/4

some of those issues i may move to next milestone, so could be pretty soon.

added betydb_experiment

sckott · 2018-03-12T21:25:18Z

merged, sorry about the long wait

@infotroph

added (Chris Black) (@infotroph) to author list based on contributions to the betydb interface (especially ropensci#94, ropensci#88, ropensci#82)

dlebauer added 3 commits April 12, 2017 13:25

Merge pull request #3 from ropensci/master

25c51af

update fork

adding ability to page through queries that return many many results …

d38bcab

…in order to prevent 504 timeout errors

Finished first draft of paging for betydb functions; added tests

5ebfe85

dlebauer added 4 commits April 14, 2017 21:33

use smaller default limit for testing

b4d7c53

use smaller default limit for testing

ddc6f64

reduced default paging limit further

a4c189b

Merge branch 'master' of https://github.com/dlebauer/traits Conflicts: tests/testthat/test-betydb.R

fixed broken tests

56f83da

dlebauer added 2 commits April 17, 2017 20:05

ensure iterations are correctly used when per_call_limit is not default

e641d6d

updated tests to use smaller per-call limit

betydb fns: handle results w/ no counts in warning

5430cb8

change warning to message tests: fixed errors testing API responses

infotroph suggested changes Apr 18, 2017

View reviewed changes

dlebauer mentioned this pull request Apr 18, 2017

recent changes to allow paging terraref/traits#1

Merged

dlebauer and others added 13 commits April 18, 2017 14:43

Merge pull request #1 from dlebauer/master

62dfcdf

recent changes to allow paging

addressed comments by @infotroph

c85ce97

better opts handling change from `expect_true(grepl ...` to `expect_match`

Merge https://github.com/terraref/traits

8e9bbf3

Merge https://github.com/terraref/traits

0d57a5d

Merge branch 'master' of https://github.com/dlebauer/traits

e679920

Merge branch 'master' into master

8774d5a

merge with ropensci-master

28720dd

merge dlebauer-traits

3de88e5

added import progress bar from utils

a0b8d80

add import progress bar to NAMESPACE

706694c

Include method for querying experiment by id

335f627

Include 'betydb_experiment()', exactly the same as 'betyb_site()', but replaces 'sites' with 'experiments' endpoint

Merge pull request #2 from nickheyek/patch-1

918d285

Include method for querying experiment by id

ran document() to add betydb_experiment documentation and NAMESPACE

129a56a

nheyek and others added 5 commits July 17, 2017 11:01

Fix betydb_experiment() function

11eed94

Merge pull request #3 from nickheyek/patch-1

53de01f

Fix betydb_experiment() function

Merge branch 'master' into lebauer-master

ff9a706

httr::header not in namespace

363821d

pass call limit as an option

ef244c4

infotroph mentioned this pull request Oct 8, 2017

Paging fix dlebauer/traits#4

Merged

Merge pull request #4 from infotroph/paging-fix

3cb72fd

Paging fix

infotroph mentioned this pull request Oct 10, 2017

Support Bety api v1 #103

Closed

sckott added this to the v0.4 milestone Oct 23, 2017

dlebauer added 5 commits October 23, 2017 16:32

merge from ropensci-master

2487ed4

merge from dlebauer/traits

56544b2

Merge branch 'ropensci-master'

5c5797e

Merge pull request #5 from terraref/master

9f224b3

added betydb_experiment

added space to restart CI build

af2a05f

sckott merged commit 8fe9631 into ropensci:master Mar 12, 2018

dlebauer added a commit to dlebauer/traits that referenced this pull request Mar 16, 2018

add Chris Black to authors

c145789

added (Chris Black) (@infotroph) to author list based on contributions to the betydb interface (especially ropensci#94, ropensci#88, ropensci#82)

dlebauer mentioned this pull request Mar 16, 2018

add Chris Black to authors #106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable paging for betydb functions #94

Enable paging for betydb functions #94

dlebauer commented Apr 14, 2017 •

edited

Loading

dlebauer commented Apr 14, 2017

codecov-io commented Apr 14, 2017 •

edited

Loading

dlebauer commented Apr 14, 2017

sckott commented Apr 17, 2017

dlebauer commented Apr 17, 2017

sckott commented Apr 17, 2017

infotroph left a comment

infotroph Apr 18, 2017

dlebauer Apr 18, 2017

infotroph Apr 18, 2017

dlebauer Apr 18, 2017

infotroph Apr 18, 2017

dlebauer Apr 18, 2017

infotroph Apr 18, 2017

dlebauer Apr 18, 2017

infotroph Apr 18, 2017

dlebauer commented Oct 23, 2017

sckott commented Oct 23, 2017

sckott commented Mar 12, 2018

		expect_is(get.out, "response")
		expect_true(grepl("OK", get.out[["headers"]]$status))

Enable paging for betydb functions #94

Enable paging for betydb functions #94

Conversation

dlebauer commented Apr 14, 2017 • edited Loading

Description

Related Issue

Example

dlebauer commented Apr 14, 2017

codecov-io commented Apr 14, 2017 • edited Loading

Codecov Report

dlebauer commented Apr 14, 2017

sckott commented Apr 17, 2017

dlebauer commented Apr 17, 2017

sckott commented Apr 17, 2017

infotroph 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlebauer commented Oct 23, 2017

sckott commented Oct 23, 2017

sckott commented Mar 12, 2018

dlebauer commented Apr 14, 2017 •

edited

Loading

codecov-io commented Apr 14, 2017 •

edited

Loading