Luca/fix raw predict issue 635

[lucagiovagnoli]

Code change
This is a one line change that fixes issue #635 where rawPredict should be (-m, m).
(also found another small bug where tree_limit wasn't being serialized in the store)

Added tests
I noticed there were no tests for mleap-xgboost-runtime so I added a bunch of them using the same "agaricus" dataset from dmlc-xgboost:

1. Booster and mleap XGBoostClassifier results match (tests for (-m, m))
2. XGBoostClassification works before and after serialization.
3. XGBoostRuntime schema assertions based on the NodeShape

@talalryz @voganrc @ancasarb

[ancasarb]

are you sure Provided.spark isn't enough? it seems like that for the other parity tests?

[lucagiovagnoli]

So I actually meant to get your input on these dependency changes.
Both Provided.spark or Test.spark achieve the same thing here because I am using spark things only in tests.

I think that putting those dependencies with a test scope makes the build faster, since the dependencies are only brought in when running tests, not when building. I read online:

• test:

Dependencies with maven dependency scope test are not needed to build and run the project. They are needed to compile and run the unit tests.

• provided:

Maven dependency scope provided is used during build and test the project.

so I assume a test scope is slightly lighter than provided? Again, the xgboost-runtime code has no dependencies on spark stuff, but its tests do.

Another option would be to avoid spark things in tests but I found spark has the best libsvm reader that lets me do
val dataFrame = spark.read.format("libsvm").load(this.getClass.getClassLoader.getResource(filePath).getFile)

and I can use the Dataframe -> to mleapFrame utils.

Let me know what you think!

[ancasarb]

That makes sense, thank you!

[ancasarb]

where do we use sparkTestkit?

[lucagiovagnoli]

If I don't bring that in, I cannot use:
import org.apache.spark.sql.mleap.TypeConverters

Do you think it is ok to use that in tests? (%"test" should not be included in the build)

[ancasarb]

I think that comes from mleap-spark-base, but spark-testkit should be ok too.

[ancasarb]

looks good overall, just had a couple of questions.

[talalryz]

This looks good to me.
Perhaps we should also test for XGBoostMultinomialClassificationModel

[lucagiovagnoli]

Thanks all for reviewing! I added a multinomial classifier test and fixed all copy-paste leftovers. Let me know what you think about using spark % test for these tests.

[talalryz]

This looks good to me, but it looks like the travis build failed.

[lucagiovagnoli]

it seems that my new tests passed successfully (https://travis-ci.org/combust/mleap/jobs/649255264#L9688) but the python ones failed. I'll try rerun these

[lucagiovagnoli]

I think this broke after the recent update of virtualenv to 20.0.0
See pypa/virtualenv#1551

EDIT: I fixed this in #641 so I expect these test to fail until we push that branch out

[talalryz]

A couple of small comments, but this looks good to me!

Thank you for adding the multinomial test as well!

[voganrc]

LGTM

[ancasarb]

Looks great, thank you!