radanalyticsio / jiminy-predictor

a predictor service for a spark based recommendation app
Apache License 2.0
2 stars 4 forks source link

Guard against invalid user id in top-k requests #28

Closed ruivieira closed 6 years ago

ruivieira commented 6 years ago

When requesting top-k predictions for an invalid user id (see #27), MatrixFactorizationModel crashes and Spark ALS stops providing predictions.

Since Spark's ALS model has no built-in facility to catch this, a safeguard must be built at the application logic level. The solution proposed in this PR consists of:

This prevents crashing the ALS model at the cost of one call to an RDD of ids.

Example (after patching):

Using user id=900000 (invalid) we issue

curl --request POST \
   --url http://0.0.0.0:8080/predictions/ranks \
   --header 'content-type: application/json' \
   --data '{"user": 900000, "topk": 25}'

we get the server response

{
  "prediction": {
    "id": "ea632652c61c4508bed702bfecad32c7",
    "products": [],
    "topk": 25,
    "user": 900000
  }
}

the log alerts us:

2018-02-17 21:46:23,947 - jiminy-predictor - ERROR - Requesting rankings for invalid user id=900000

Fetching the prediction with

curl --request GET --url http://0.0.0.0:8080/predictions/ranks/ea632652c61c4508bed702bfecad32c7

Returning the response:

{
  "id": "ea632652c61c4508bed702bfecad32c7",
  "products": [],
  "topk": 25,
  "user": 900000
}

but without crashing the Spark model.

ruivieira commented 6 years ago

@elmiko ptal, thank you!

elmiko commented 6 years ago

thanks @ruivieira! this looks like a nice fix for now. it would be ideal if we could return an error on that initial request, but i realize that may not be feasible given our current code paths.

maybe in the future we should upgrade the responses to have some sort of error reporting mechanism through the call to get the prediction. like, instead of simply returning an empty list, we could just return an error object or something. i dunno, we probably need to think more about that.

elmiko commented 6 years ago

closes #27