Fix load of models that depend on non thread-safe dependencies

[paulojrp]

A problem was detected when loading TensorFlow models in different threads inside the same JVM. That happened after load a TensorFlow model and then try to import a new TensorFlow model. This was caused by a dependency of TensorFlow (protobuf) that was being reloaded but it already existed in the JVM (through the 1st thread).

The workaround was to share the problematic module (Tensorflow) across the sub-interpreters of Python. This is a workaround for the issues with CPython extensions.

[paulojrp]

at the moment I only tested this with UTs, before landing this I want to make sure that it works in a real environment

[TravisBuddy]

Travis tests have failed

Hey @paulojrp,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: b182d090-fa53-11e8-8526-c1031fffd6c8

[pedrorijo91]

I suppose this is related with #26 ?

[pedrorijo91]

kudos for the explanation! IMHO I would prefer to keep the explanation generic instead of base it on the specific case we caught (and then with a reference to the specific github issue), but it's a super minor nitpick, and please let anyone else comment on this before changing anything

[pedrorijo91]

A quick review seems good. will make a deeper review monday. But there are tests failing

[nmldiegues]

A clear disadvantage of this approach is that it never "releases" models already loaded. E.g., consider a JVM where you load and close 10000 models sequentially; this will eventually go OOM.
I'm thinking: could you maybe cache the model loading in the ClassificationPythonModel, and in the "close" just "null out" the variable (for Python GC to kick in)? That would probably make this solution perfect.

Quick note: the docker image used in travis is not set up with tensorflow, but I see you added as UT the regression test for it. Nevertheless there are some failed UTs. I'll let you take a look at it.

[paulojrp]

I suppose this is related with #26 ?

yes this commit fixed that issue

[paulojrp]

A clear disadvantage of this approach is that it never "releases" models already loaded. E.g., consider a JVM where you load and close 10000 models sequentially; this will eventually go OOM.
I'm thinking: could you maybe cache the model loading in the ClassificationPythonModel, and in the "close" just "null out" the variable (for Python GC to kick in)? That would probably make this solution perfect.

Quick note: the docker image used in travis is not set up with tensorflow, but I see you added as UT the regression test for it. Nevertheless there are some failed UTs. I'll let you take a look at it.

Another problem is the names of to methods responsible for the classification of events. The implementation of the providers is assuming that their names are constantes ("classify" and "getClassification"). This logic won't work if we only use one python interpreter. I will explore another solutions.

[codecov]

Codecov Report

Merging #27 into master will decrease coverage by 50.96%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##             master      #27       +/-   ##
=============================================
- Coverage       100%   49.03%   -50.97%     
- Complexity       22       37       +15     
=============================================
  Files             6       11        +5     
  Lines            42      208      +166     
  Branches          0        8        +8     
=============================================
+ Hits             42      102       +60     
- Misses            0      104      +104     
- Partials          0        2        +2

Impacted Files	Coverage Δ	Complexity Δ
...zai/openml/python/modules/SharedModulesParser.java	100% <100%> (ø)	7 <7> (?)
...eedzai/openml/python/jep/instance/JepInstance.java	86.48% <100%> (ø)	6 <1> (?)
...nml/python/jep/instance/AbstractJepEvaluation.java	66.66% <0%> (ø)	2% <0%> (?)
...edzai/openml/python/ClassificationPythonModel.java	0% <0%> (ø)	0% <0%> (?)
...n/AbstractClassificationPythonModelLoaderImpl.java	0% <0%> (ø)	0% <0%> (?)

---

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 7f2d92f...1337706. Read the comment docs.

[nmldiegues]

This is pretty nice @paulojrp . Of course it has the downside that it requires code changes for every new library with the same problem, but it is an acceptable trade-off. I trust that you ran the UT regression of #26 and it passed?

I just have 1 suggestion: Please make the list of packages added to shared modules read dynamically (environment variable? a resource file inside the openml jar? please consider remote spark executions when devising the solution) --- this will make users autonomous to overcome this problem for a new library in the future; please add a note to the README about the #26 and this solution

[pedrorijo91]

overall looks great! :) just a few minor style issues. Feel free to disagree with them

[pedrorijo91]

I think it should be 'you will need to create a (...)'. @krisztinaknagy to validate this one

[pedrorijo91]

is there any official documentation for this python-packages.xml?

[paulojrp]

bellow you can find an example of the contents of this file

[pedrorijo91]

actually, I would expect a UT like the one described in #26 . Is it too hard to automate?

[TravisBuddy]

Hey @paulojrp,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: d0ff2700-ff96-11e8-88a0-5d55312123ee

[nmldiegues]

@pedrorijo91 for adding that UT we'd need tensorflow to be added to the Docker image used in Travis (and then to the base openml image used inside Feedzai). I'm not sure we want to keep adding all the stuff to the images, they'll grow endlessly.

@paulojrp Reviewed, mostly minor comments: please check the coverage and improve it, and tackle my comments. Thanks!

[pedrorijo91]

can't we simply create a custom model that loads python stuff @nmldiegues ? anyway, if it's too time/effort-consuming, let's be pragmatic and go without it :)

[TravisBuddy]

Hey @paulojrp,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: ade3d770-ffaf-11e8-88a0-5d55312123ee

[paulojrp]

@nmldiegues I created a new UT but the coverage is still decreasing a lot, I don't see how this is related with my modifications.

@pedrorijo91 to reproduce this problem we only had to load a python file that imports tensorflow. The problem with a unit-test for this case is that we would have to modify the docker image to also install tensorflow. we could do it but then what would happen when new packages with these problems are found? (modify the image to also check that). Don't forget that we don't have an openml provider for tensorflow.
Also this modification was tested manually by loading an tensorflow model and by using it to score events.

[pedrorijo91]

all good on my side then :)