rstudio / plumber

Turn your R code into a web API.
https://www.rplumber.io
Other
1.39k stars 256 forks source link

Error: object of type 'closure' is not subsettable #16

Closed jochenvanwylick closed 9 years ago

jochenvanwylick commented 9 years ago

Thanks for this cool library - exactly what I've been looking for. However - I've got a bit of an issue, I get the following error message:

{"error":["500 - Internal server error"],"message":["object of type 'closure' is not subsettable"]}

Or more detailed:

simpleError in data$PriceEUR: object of type 'closure' is not subsettable

Here's my code:

#' @get /predict
doPredict <- function(from, to, price, ndtf, type = "response") {
  getTree <- function(from, to) {
    varname <- paste0(from, "_", to);
    return(eval(parse(text=varname)));
  }

    tree <- getTree(from, to);

    # Creates a data frame from the parameters
    PriceEUR <- c(as.integer(price));
    NumDaysToFlight <- c(as.integer(ndtf));
    s  <- data.frame(PriceEUR, NumDaysToFlight);

    # Return the predicted value
    value <- predict(tree,1,s,type = type)
    return(value);
}

When I run this code locally - it works. Return type of the prediction call is an integer.

However, when called from the webserver, I get the error mentioned above. Am I doing something wrong or did I hit an issue?

trestletech commented 9 years ago

Thanks for the report. Can you share the GET request you're using to call this function?

trestletech commented 9 years ago

(And/or an example of the format of one of the tree variables you're referencing in getTree.)

trestletech commented 9 years ago

Also beware that you're currently vulnerable to arbitrary code execution. I'd discourage anyone from evaling any input that they get from the user.

e.g. http://localhost:8000/predict?from=print(%22hello%22);%20a&to=2

will execute the print("hello") line (which could just as easily be unlink("/somedir", recursive=TRUE). I'd recommend perhaps treating the to and from inputs you get as a name. Perhaps you could put all your trees into a list and then pull them out using tree[[paste0(to, "_", from)]] which wouldn't require you to evaluate the user input.

jochenvanwylick commented 9 years ago

Sure - get call looks like:

http://--whatever--:8000/predict?from=AMS&to=YYZ&price=123&ndtf=12&type=response

The tree is a ctree from the partykit package. They're loaded in memory as variables.

jochenvanwylick commented 9 years ago

Regarding the eval: good point - thanks!

trestletech commented 9 years ago

I'm not familiar with that package. If you can share the code necessary to generate one of those variables and a sensible set of parameters for that variable, I'd be happy to investigate further.

jochenvanwylick commented 9 years ago

Here's an example file: https://dl.dropboxusercontent.com/u/1806585/AMS_SFO.rds

install.packages('partykit')
require('partykit')

Put this on top of your script - then load them in with readRDS(filename). Let me know when you got the file - then I'll remove it from dropbox please.

Thanks for the effort + the library by the way!

trestletech commented 9 years ago

I downloaded the file but I get this after I've loaded it in as AMS_SFO

> doPredict("AMS", "SFO", "123", "12", "response")
 Error in unames[factors] : invalid subscript type 'list' 
5 lapply(X = X, FUN = FUN, ...) 
4 sapply(unames[factors], function(x) isTRUE(all.equal(levels(object$data[[x]]), 
    levels(newdata[[x]])))) 
3 predict.party(tree, s, type = type) 
2 predict(tree, s, type = type) at test.R#20
1 doPredict("AMS", "SFO", "123", "12", "response") 
jochenvanwylick commented 9 years ago

OK, there was a problem with that particular tree. Here you go: https://dl.dropboxusercontent.com/u/1806585/example.zip . Mini reproduction environment.

Then, run host.R - and call URL http://localhost:8000/predict?price=123.0&ndtf=123 and observe:

{"error":["500 - Internal server error"],"message":["object of type 'closure' is not subsettable"]}

trestletech commented 9 years ago

The one problem that I see in your script is that you're assigning the variable into the Global environment. plumber is going to run the server within its own environment, so you'd probably be safer just using a regular assignment there:

AMS_ACC <- readRDS("~/Downloads/example/AMS_ACC.rds")

When I use this file it works for me.

library(partykit)

AMS_ACC <- readRDS("~/Downloads/example/AMS_ACC.rds")

#' @get /predict
doPredict <- function(from, to, price, ndtf, type = "response") {
  getTree <- function(from, to) {
    varname <- paste0(from, "_", to);
    return(eval(parse(text=varname)));
  }

  tree <- getTree(from, to);

  # Creates a data frame from the parameters
  PriceEUR <- c(as.integer(price));
  NumDaysToFlight <- c(as.integer(ndtf));
  s  <- data.frame(PriceEUR, NumDaysToFlight);

  # Return the predicted value
  value <- predict(tree,s,type = type)
  return(value);
}
p <- plumb("theAboveFile")
p$run()

Here's the curl command generated by Chrome

curl 'http://localhost:8000/predict?from=AMS&to=ACC&price=123&ndtf=12&type=response' -H 'Pragma: no-cache' -H 'DNT: 1' -H 'Accept-Encoding: gzip, deflate, sdch' -H 'Accept-Language: en-US,en;q=0.8' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.134 Safari/537.36' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8' -H 'Cache-Control: no-cache' -H Connection: keep-alive' --compressed

and the response:

[1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1.3182,1.3182,1.3182,1.1087,1.1087,1.3182,1.3182,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1,1,1.1087,1,1.1087,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1.3182,1.3182,1.3182,1,1.1087,1.3182,1.3182,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1,1,1.1087,1,1.1087,1.1087,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1.3182,1.3182,1.3182,1,1.1087,1.3182,1.3182,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1.1087,1,1,1.1087,1,1.1087,1.1087,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1.3182,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1.3182,1.3182,1.3182,1,1.1087,1.3182,1.3182,1.3182,1.1087,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1.1087,1,1.1087,1.1087,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1]

The file you sent over in functions.R is different from the one you originally provided and I'd expect would have some problems (since it doesn't convert the characters into integers/numerics, for instance).

jochenvanwylick commented 9 years ago

Hi - thanks! Reason that assign is in there is because I'm looping through a bunch of .rds files and loading them in memory. So just assigning works for this example, but not in my real-life example.

I removed those as.integer() things - because they didn't seem to be helping and I wanted to keep the example as simple as possible.

trestletech commented 9 years ago

Oh, I think it was the predict line. You have an extra 1 there that doesn't correspond to anything I see in the docs for that predict function.

I get the 'closure not subsettable' error when I have the 1 and it works when I don't.

trestletech commented 9 years ago

Also value <- predict_party(AMS_SFO,1,data,type = type) works.

jochenvanwylick commented 9 years ago

The response needs to be [1] 1 - not that series of ones. But how can I cache data in memory then for the predict call ? I want to have those trees ( 100+ ) in memory so that the predict call only takes it from memory and runs the prediction. Can I use the Plumber environment from the assign call to do that?

trestletech commented 9 years ago

Did you try value <- predict_party(AMS_SFO,1,data,type = type) to get a singular value? That produced a single number for me, but that's really a matter of just finding the right call with the partykit package.

I'd encourage you to just look at a list for the storage of the data so you don't have to evaluate arbitrary R code from the user. I don't know what structure you have them in now, but if you could read them all in and setup a structure like the following, I think you'd be better off (and wouldn't have to worry about the assigns).

data <- list()
data[["AMS_SFO"]] <- readRDS("whatever")
...

And the pull them out with data[[paste0(from, "_", to)]].

I'm going to mark this as closed for now since there doesn't seem to be a plumber issue here, but let me know if you feel differently as you progress.

jochenvanwylick commented 9 years ago

Hey - sorry for the late response: I was off for a long weekend. I'll look into your solution. Indeed - I'm not too worried about the partykit function call since that part is still quite volatile in our app. I think they've already switched to the c50 package now for tree generation.

The list seems like a great idea - I'll check that out and keep you posted.

jochenvanwylick commented 9 years ago

Confirmed that it works when using predict_party. Using the base - R predict call returns the object of type 'closure' is not subsettable error mentioned. But using predict_party doesn't.

It would be slightly more elegant to have the predict function work as well, since that function typically is overloaded by these prediction packages. That way, I can keep using predict, regardless of the underlying package used for generating the prediction tree. That way I don't have to change my code when another package is used to generate the trees ( like the c50 package - which is the case for us now ).

But ok - thanks for the (IMHO) simples way of adding a web interface to R code. I advertised your library a bit here: http://stackoverflow.com/questions/19991654/shiny-server-print-json-as-a-result-output/31454779#31454779

trestletech commented 9 years ago

Cool, thanks.

The predict function's behavior shouldn't be modified at all by plumber. If you are able to produce a minimally reproducible example showing that it does, please feel free to open another bug report!