-
Notifications
You must be signed in to change notification settings - Fork 471
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-3546] Push down image pre-processing to blas/mkl #1843
base: main
Are you sure you want to change the base?
Conversation
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.
Hi anishsapkota
Welcome to Opensource,
I suggest you start with doing the following:
- Add the .h file with a function definition.
- Add the native function in NativeHelper.java
- Add a test verifying behavior of your new function, you do this by adding a new folder in src/test/java/org/apache/sysds/test/functions and call it native, inside this make a java file named something appropriate.
Furthermore, I suggest initially to make something that just hooks up and call the cpp without actually doing much to verify the binding and then after this make the MKL work.
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.
This is nice progress,
I understand based on your comment that commenting out other code this was the only way for you to make it work.
But we should strive to find a way to make all of it work (your point as well)
perhaps the limitation is that you do not have BLAS installed as well, and maybe there is a requirement that you need to install both to compile the library?
Alternatively we can add a new cpp file called systemds_img.cpp and header file that you hook into. But then you need to make a new NativeHelper class instance that then hooks up to the new file. In general i would be interested if there was a nice way of doing such a thing.
Thanks
src/test/java/org/apache/sysds/test/functions/native/NativeBindingTestWithDgemm.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/sysds/test/functions/native/NativeBindingTestWithDgemm.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/sysds/test/functions/native/NativeBindingTestWithDgemm.java
Outdated
Show resolved
Hide resolved
Hi @anishsapkota, How is it going? is there uncommitted progress or do you need any further comments? |
…pen blas, img_translate implemented
…ingTestWithDgemm.java this was just for initial testing to verify the binding, can be deleted now.
Hi @Baunsgaard , Apart from this, I have created a new file systemds_img.cpp and add the methods that I have implemented there, added nativ function tests to JavaTest.yaml and created a new Java Class ImgNativeHelper that extends NativeHelper. |
src/test/java/org/apache/sysds/test/functions/nativ/ImgUtilsTest.java
Outdated
Show resolved
Hide resolved
Hi @Baunsgaard , regarding the performance results, should I be comparing the performance between OpenBlas and Intel MKL ? or with the existing DML implementation? |
You need to compare the results between all possible ones you have. |
Regarding the tests, I need to allow them to run since this is your first commit it does not allow them to automatically run. |
java.lang.UnsatisfiedLinkError: /github/workspace/src/main/cpp/lib/libsystemds_openblas-Linux-x86_64.so: libopenblas.so.0: cannot open shared object file: No such file or directory I still can't figure it out why it can't find the lib. could you please review the static block from my last commit ? |
well, i guess we need to cleanup some of the resources. what we need to do is to remove cached files from install of our docker image to reduce its size via docker/testsysds.Dockerfile Then include MKL and OpenBLAS in the image rather than installing them in the build script of cpp and make sure we do not keep around anything not needed in our install. But this is out of scope of this PR, i would prefer if we make it pass as it is and do the Docker refinements some other time. |
@Baunsgaard @j143
@Baunsgaard should I comment out "mkl test case" so that we can merge? or how should I proceed now? |
lets go with the Blas test then and add some TODO that reference this PR about the MKL tests, then I will try to make it work at a later time, since i need it for other things. |
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.
I like it, nice implementation.
My main comment is the comparison is not fair. There are some things we can do to improve it if you want (read the detailed comments for more).
Before we merge it we need to address all the comments, but you do not need to do this before your presentation Monday, and it is up to you if you even want to.
Thanks!
path: target/jacoco.exec | ||
retention-days: 1 | ||
- name: Checkout Repository | ||
uses: actions/checkout@v4 |
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.
we need to revert here, since the change is only syntax.
|
||
apt-get update | ||
apt-get install libopenblas-dev -y | ||
fi |
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.
add comment on MKL install.
cout << endl; | ||
} | ||
cout << "\n"<< endl; | ||
} |
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.
i suggest removing this debugging printImage.
cblas_dcopy(end_x - start_x, &img_in[(x_in) + (y_in) * in_w], | ||
1, &img_out[y * out_w + start_x + static_cast<int>(offset_x)], 1); | ||
} | ||
} |
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.
indentation.
return img_out; | ||
} | ||
|
||
void img_translate(double* img_in, double offset_x, double offset_y, |
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.
cast the offsets in the first line rather than every call to offset_x and y, and maybe consider changing the API to use int offsets.
|
||
import java.io.File; | ||
|
||
public class ImgNativeHelper extends NativeHelper { |
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.
you can add doc on the Helper class itself.
i would suggest a hint at where to find the CPP files.
System.load(System.getProperty("user.dir")+"/src/main/cpp/lib/libsystemds_" + blasType + "-Darwin-x86_64.dylib"); | ||
} | ||
} catch (Exception e) { | ||
e.printStackTrace(); |
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.
i would like this to throw the exception, not just print stack trace.
you can wrap the exception in a throw new RuntimeException(e);
assertArrayEquals(expectedOutput, img_out, 1e-9); // Compare arrays with a small epsilon | ||
} | ||
|
||
public void runTests(String blasType) { |
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.
i understand why you designed it this way, but please make the test integrate with JUnit,
To do this remove the runTests
method and make all your static calls annotated with @ Test.
Then make the imgNativeHelper a field in this class that the constructor instantiate.
Then finally the class needs to be changed into a parameterized class, that calls the constructor selecting either BLAS or MKL.
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.
Hi @anishsapkota , there are a plenty of tests in the neighboring folders you can take inspiration from!
|
||
import java.util.Random; | ||
|
||
import static org.apache.sysds.utils.ImgNativeHelper.*; |
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.
no wildcard imports allowed.
} | ||
|
||
public void runBlasTests(boolean sparse,int n, int seed) { | ||
double spSparse = 0.1; |
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.
indentation
Hi @Baunsgaard , could you be more specific, how can I make the comparison fairer? I will be incorporating your previous suggestions of calculating std and analysing dml execution time using -stats flag. Do you have any further suggestions? |
The fairest comparison would be to integrate the native instruction as a DML supported command. It does not take long to make. But requires you to know where to do it. For now the stats comparison is close enough. |
could you please let me know which files I should be looking at, so that I can implement this as well ? |
DML Implementations Benchmark sample with -stats flagimage_rotate SystemDS Statistics: image_cutout SystemDS Statistics: image_crop New w/h=409.0/409.0 image_translate SystemDS Statistics: |
In the results you have above the time of the res = read($1)
print(sum(res))
for(i in 1:$2) {
res2 = OPERATION(res)
}
print(sum(res2)) This reads the file in the first argument and then repeats the operation the number of times specified in the second argument. Because of the way we execute in SystemDS the sum before the loop make sure res is actually read from disk. The downside to this approach is that it is impossible/hard for you to get the individual calls execution time. |
Hi @anishsapkota -- let's move this forward, request the reviewer to commit this much by addressing the comments (which seem minor).
after that you can add further functionality. This would remove the burden on you & merge conflicts. Thanks, Janardhan |
Hi @anishsapkota -- thanks for your contribution. do you need some help here? |
Hi @Baunsgaard , this PR does seems fine. only minor comments are there. Is it okay to take this in? |
Hi @j143 , it can be taken in but we need to be careful, and go through the code, since some of the changes (for instance GitHub actions) contain changes that should not be made. |
Hi! Sorry that I'm a bit late to the discussion. Our current Intel MKL support ends at version 2019.5 (you noticed the problems with dnn ops not compiling). I would be surprised if all magically worked by just using OneMKL and OneDNN. We should probably put the native ops testing into a separate test suite and use a Docker container that supports it. Furthermore, moving to OneAPI would change external behavior and require a major version bump. Regards, Mark |
marking this PR for next release because there are some concerns pointed by @corepointer
|
This is where i intend to add the native bindings for intel MKL image processing.