Skip to content
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

[SYSTEMDS-2839] IntelMKL Integration Extension #1887

Closed
wants to merge 10 commits into from

Conversation

anishsapkota
Copy link

This is where I intend to explore if intel MKL -> oneApi is a viable update.

Copy link
Contributor

@Baunsgaard Baunsgaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anishsapkota

I like the PR, but there are some things to fix before we can merge it.

Have you verified if the automated tests actually verify behavior?
if not can you add this test case to the file: .github/workflows/javaTests.yml
simply add a new line on something like line 72.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer if we do not throw these binaries out. They are there to avoid having to compile on new installs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Baunsgaard ,
the binaries are thrown out automatically when I run build.sh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and build, does not replace them with new ones?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake Error at CMakeLists.txt:56 (find_package):
By not providing "FindSEAL.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "SEAL", but
CMake did not find one.

Could not find a package configuration file provided by "SEAL" (requested
version 3.7) with any of the following names:

SEALConfig.cmake
seal-config.cmake

Add the installation prefix of "SEAL" to CMAKE_PREFIX_PATH or set
"SEAL_DIR" to a directory containing one of the above files. If "SEAL"
provides a separate development package or SDK, be sure it has been
installed.

I get these errors and the whole he folder inc. the flies inside it get removed after build

// -------------------------------------------------------------------
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
/*JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you could not make it work without commenting it out.

But can we do a second try? or in the PR at least not comment it out.

One alternative strategy is to compile a separate file systemds_img.cpp ? maybe.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on it !

@@ -416,4 +416,21 @@ public static native boolean conv2dSparse(int apos, int alen, int[] aix, double[
// different tradeoffs. In current implementation, we always use GetPrimitiveArrayCritical as it has proven to be
// fastest. We can revisit this decision later and hence I would not recommend removing this method.
private static native void setMaxNumThreads(int numThreads);

//test interface for native binding
public static native void testNativeBindingWithDgemm(char transa, char transb, int m, int n, int k, double alpha, double[] A, int lda, double[] B, int ldb, double beta, double[] C, int ldc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you go with the separate cpp file idea, then we can move these lines to another file.

}
System.out.println();
}*/
System.out.println("Execution time: " + executionTime + " ms");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print statements in the tests.

@Baunsgaard
Copy link
Contributor

I think it would be great if you chose one of the Pr's of #1887 and #1843 and continue on that one, while closing the other one.
Thanks.

@anishsapkota
Copy link
Author

I think it would be great if you chose one of the Pr's of #1887 and #1843 and continue on that one, while closing the other one.
Thanks.

I would continue with 1843 and make the necessary changes as suggested by you

@Baunsgaard
Copy link
Contributor

Closed and continue on #1843

@Baunsgaard Baunsgaard closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants