tarakc02 / ratelimitr

Rate limiting for R functions
Other
40 stars 0 forks source link

Does this /how could this work for a generally rate limited API? #4

Closed stephlocke closed 7 years ago

stephlocke commented 7 years ago

Hi @tarakc02, neat concept!

For HIBPwned, I need to rate limit overall usage to once per 1.5s so if I use different functions they all need to adhere to the same rate limit. It's not clear if this is a use case ratelimitr could cover right now?

tarakc02 commented 7 years ago

Thanks @stephlocke! That's a good point, right now the limit_rate function is constrained to only be able to manage rate limiting for a single function, so if you have a group of functions that have to collectively remain under a rate limit, limit_rate won't do the job. However, the implementation of limit_rate uses a token_dispenser (see the source code for limit_rate), which is a flexible enough concept to meet your use-case. The token_dispenser has just one method, request, which is guaranteed to return TRUE, possibly after a delay.

I'm in the process right now of exporting token_dispenser and writing some documentation pages, but briefly: in your case, I would create a new token_dispenser with my_dispenser <- token_dispenser(n = 1, period = 1.5). Then any function that should be subject to the rate limit can first ask for a token, by having the line request(my_dispenser) as the first line of the function. Any function that has received a token in this way is now free to make the API call. Since the request function returns TRUE, you can do something like:

if (request(my_dispenser)) make_api_call(...)

I hope this makes sense! Like I said, I'm in the process of exporting the necessary functions and writing the documentation, so this should become easier and more clear soon.

tarakc02 commented 7 years ago

Ok, so I added a vignette based on this question, check it out and see if it makes sense. Let me know if you still have questions, thanks!

tarakc02 commented 7 years ago

Re-opening the issue because although what's laid out in the vignette works, I think I can make it more convenient by adding this ability to the limit_rate function.

tarakc02 commented 7 years ago

Hi @stephlocke, I've added a section to the readme: Multiple functions sharing one rate limit. I realized it would be a lot easier to just add that functionality to the limit_rate function rather than messing around with token dispensers and whatnot. Is this what you're trying to accomplish? Thanks!

stephlocke commented 7 years ago

So from reading the README, as a package dev who wants all their functions to adhere to a core rate limit I would:

  1. Write (unexported) functions that perform API calls
  2. Write an (unexported) rate limit object that includes a list of the functions to be rate limited
  3. Write exported functions that call the unexported function via the rate limit object?
#' Hello X!
#' @inheritparams myLimitedFunc
#' @export
myFunc<-function(x){limited$myLimitedFunc(x)}

#' Unexported version
#' @param x Thing to say hello to
myLimitedFunc<-function(x){ paste0("Hello ",x,"!")}

#' My rate limiter
limited <- limit_rate(list(myLimitedFunc=myLimitedFunc), rate(n = 3, period = 1))

* Written from scratch without any testing or checking

tarakc02 commented 7 years ago

Yup, that looks right to me (also without having tested/checked). I don't know if it applies to your case, but I'm working on an API client for Mapzen search, and though there are multiple api call functions corresponding to the various search endpoints, I only rate-limit one function (httr's GET), as every higher level api call function ends up calling the rate-limited GET function at some point (example: https://github.com/tarakc02/rmapzen/blob/accessors/R/mz-search-main.R).

stephlocke commented 7 years ago

Excellent point! I could rate limit the GET in my existing helper func in HIBPwned. I think it makes a good practice to do it the way you're suggesting.

tarakc02 commented 7 years ago

Great! Glad it made sense, I'll close this issue. FYI, I noticed that there "external pointer" errors when using limit_rate from another package (which is a bummer, since that is the main way I imagine it will be used). I re-wrote the stuff that used to rely on Rcpp and just pushed the update, so the latest version should work properly (see Issue 6 for more information).

stephlocke commented 7 years ago

I seem to get a little bit of variability with my unit tests using the rate limit, but I now have a branch set up using ratelimitr. Will see if I can repro issue for test. Let me know if/when you hit CRAN! https://github.com/censornet/HIBPwned/tree/ratelimitr

tarakc02 commented 7 years ago

Awesome, thanks for the update! I too just noticed that the rate limited function occasionally goes too fast (on my tests, it has been too fast by about 1E-11 to 1E-17 seconds, which makes me thinking I'm still missing a spot where I should be rounding . . .). I'm going to open a new issue for that. As a side note, thanks for your package -- I didn't know about HaveIBeenPwned before, it's been a wake-up call . . ..