-
Notifications
You must be signed in to change notification settings - Fork 28k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-5814][MLLIB][GRAPHX] Remove JBLAS from runtime #4699
Conversation
Test build #27757 has started for PR 4699 at commit
|
<dependency> | ||
<groupId>net.sourceforge.f2j</groupId> | ||
<artifactId>arpack_combined_all</artifactId> | ||
<version>0.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be runtime-only scope? is this basically a pure Java backend to netlib? I checked the .jar and it seems to be all Java. I double-checked and this is BSD licensed. Since license issues are front of mind in this change, add a line for this new lib in the LICENSE
file under the other BSD dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some netlib-java BLAS routines contains org.netlib.intW
, which is not part of com.github.fommil.netlib
but arpack_combined_all
. In the netlib:core
pom file (https://repo1.maven.org/maven2/com/github/fommil/netlib/core/1.1.2/core-1.1.2.pom), arpack_combined_all
is specified as a dependency but I cannot run the tests without explicitly specifying in the pom. Maybe it is because that it also include arpack_combined_all:javadoc
as a dependency, sbt/maven doesn't resolve the dependency correctly.
Breeze already depends on netlib-java and arpack_combined_all. Should we already have something under LICENSE?
The changes look like what I'd expect. I haven't reviewed every replacement. The only real risk in merging for 1.3 is that there is a typo in the translation, but let's see what tests say. |
Test build #27757 has finished for PR 4699 at commit
|
Test FAILed. |
Added mima excludes. (I don't know why it complains in this PR.) |
Test build #27763 has started for PR 4699 at commit
|
Are these legit mina problems with another patch, or just false positives? Should we send a hot fix to exclude all of these? (There are quite a few of them) |
@andrewor14 I didn't track changes in |
Ok I stopped seeing them fail in more recent builds so I'm just going to leave it as is. |
Test build #27763 has finished for PR 4699 at commit
|
Test PASSed. |
Test build #27774 has started for PR 4699 at commit
|
Test build #27774 has finished for PR 4699 at commit
|
Test PASSed. |
I went through the changes twice. The math should be the same as before. SVDPlusPlus should get better performance by allocating less temp objects. I suggest merging this into branch-1.3. @pwendell |
@srowen Just realized that we cannot merge this into branch-1.3 because we still need to return |
Oh right, of course. Yes, PR coming right up... |
Test build #28464 has started for PR 4699 at commit
|
Test build #28464 has finished for PR 4699 at commit
|
Test PASSed. |
<version>${jblas.version}</version> | ||
<groupId>com.github.fommil.netlib</groupId> | ||
<artifactId>core</artifactId> | ||
<version>1.1.2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we manage the version centrally in the parent POM? this is repeated otherwise in the mllib POM.
Looking good. I didn't verify each translation to netlib, but eyeballing them, it looked like what I'd expect. that's what the tests will confirm. |
Test build #28484 has started for PR 4699 at commit
|
Test build #28484 has finished for PR 4699 at commit
|
Test PASSed. |
Merged into master. Thanks for reviewing! |
The issue is discussed in https://issues.apache.org/jira/browse/SPARK-5669. Replacing all JBLAS usage by netlib-java gives us a simpler dependency tree and less license issues to worry about. I didn't touch the test scope in this PR. The user guide is not modified to avoid merge conflicts with branch-1.3. @srowen @ankurdave @pwendell