Use local installations for benchmarking #53
Conversation
Also remove some unnecessary previous variables.
| @@ -37,7 +37,7 @@ class ALLKNN(object): | |||
| @param path - Path to the mlpack executable. | |||
| @param verbose - Display informational messages. | |||
| ''' | |||
| def __init__(self, dataset, timeout=0, path=os.environ["WEKA_CLASSPATH"], | |||
| def __init__(self, dataset, timeout=0, path=os.environ["JAVAPATH"], | |||
There was a problem hiding this comment.
Not sure, what to do here, back then WEKA_CLASSPATH was something like JAVAPATH/weka.jar I guess, we could adjust the methods to use e.g. cmd = shlex.split("java -classpath " + self.path + "/weka.jar" + ":methods/weka" + " AllKnn " + inputCmd + " " + options) or we could introduce another environment variable that includes weka.jar.
There was a problem hiding this comment.
Sorry, this one fell off my radar a little, thanks for the reminder. I agree with your solution, better to have a JAVAPATH and then specifically add $JAVAPATH/weka.jar to the classpath instead of one different path for every Java library. Let me implement that now...
Makefile
Outdated
| export LD_LIBRARY_PATH=$(shell echo $(LIBPATH)) | ||
| # Set PYTHONPATH correctly. | ||
| PYVERSION=$(shell python3 -c 'import sys; print("python" + sys.version[0:3])') | ||
| export PYTHONPATH=$(shell pwd)/libraries/lib/$(shell echo $(PYVERSION))/dist-packages:$(shell pwd)/libraries/lib/$(shell echo $(PYVERSION))/site-packages |
There was a problem hiding this comment.
Do you think it's a good idea to append to the existing PYTHONPATH?
export PYTHONPATH=$(shell printenv PYTHONPATH):$(shell pwd)/libraries/lib/$(shell echo $(PYVERSION))/dist-packages:$(shell pwd)/libraries/lib/$(shell echo $(PYVERSION))/site-packages
|
I think once we decided what to do with the weka environment variable, this is ready to be merged. |
|
@mlpack-jenkins test this |
|
I think this is ready to be merged, once jenkins is reconfigured the test should pass. |
|
@mlpack-jenkins test this I have a problem running the tests locally on slake that I haven't figured out yet. It looks like Jenkins runs the tests fine, but there may be a couple to debug and it may take me a few more iterations to get it all right. |
|
@mlpack-jenkins test this |
| export MATLAB_BIN="" | ||
|
|
||
| # Export the MATLABPATH environment variable. | ||
| export MATLABPATH=$(shell pwd)/methods/matlab/ |
There was a problem hiding this comment.
Not sure what your plans are, but I think we have to set the MATLABPATH environment variable to add the methods/matlab folder to the MATLAB search path.
|
I tested the code on slake and besides that the hlearn installations fails, it looks like everything else works as expected. |
methods/weka/allknn.py
Outdated
| @@ -58,13 +58,14 @@ def RunMetrics(self, options): | |||
| # If the dataset contains two files then the second file is the query file. | |||
| # In this case we add this to the command line. | |||
| if len(self.dataset) == 2: | |||
| inputCmd = "-r " + self.dataset[0] + " -q " + self.dataset[1] + " " + options | |||
| inputCmd = "-r " + self.dataset[0] + " -q " + self.dataset[1] + " " + | |||
| options | |||
There was a problem hiding this comment.
This results in a weird syntax error, it works without the break inputCmd = "-r " + self.dataset[0] + " -q " + self.dataset[1] + " " + options
|
HLearn doesn't work with newer versions of ghc, so I think that we should uninclude it from the benchmarks for now until upstream is updated (not sure when that will be). I can't easily make it compile with ghc 8, and even compilation with ghc 7 has problems---it seems like other Haskell dependencies weren't compiled with |
|
I agree, uninclude it for now, maybe @mikeizbicki could tell us more about if there are any plans to support ghc 8 as well. |
|
Looks like things are very busy for him: mikeizbicki/HLearn#84 but if it ever happens, definitely we can reinclude it then. |
|
Bad news, I think we have to remove mlpy too---the reason is that mlpy depends on pygsl which in turn depends on libgsl 1, but Debian now provides only libgsl 2. I tried a local installation of libgsl 1.16, but ld loading order issues (I think) cause python3 to segfault after the computation now. mlpy is entirely unmaintained and dead now, so I think it is not a huge loss. I won't remove it in this PR, but we can do that later, and I think we will have failing tests until that is done. |
|
Oh, okay, if it's unmaintained, I guess the chances are almost null that we would see an updated version. |
|
Ok, I think I am ready to merge this, if you think it is okay. Once that is done, I'll update Jenkins to build all of the libraries locally, and then I can start fixing whichever tests are broken. Then it will be much easier to merge in new libraries and the other PRs. |
|
Sounds good for me. |
This sets the environment variables correctly so that the libraries installed in
libraries/are used for benchmarking.I removed a lot of the now-unnecessary variables from the Makefile.
I tested this (though incompletely) on my local system enough to convince myself it at least mostly works. It'll receive more testing as I get it up on Jenkins (but that will have to happen after merge).