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

MKL RandIntEngine #222

Merged
merged 40 commits into from
Aug 16, 2023
Merged

Conversation

OlhaBabicheva
Copy link
Contributor

@OlhaBabicheva OlhaBabicheva commented Apr 4, 2023

  • Added fill_with_ints function (it uses mkl library in order to generate ints) to RandintEngine class
    Results obtained using neighbor.py benchmark (average time from 15 runs):

Results for original code:
image

Results for changed code (with MKL):
image

Results for changed code (without MKL):
image

@mszarma mszarma requested a review from rusty1s April 5, 2023 07:47
CHANGELOG.md Outdated Show resolved Hide resolved
@rusty1s rusty1s enabled auto-merge (squash) April 28, 2023 09:41
@rusty1s
Copy link
Member

rusty1s commented Apr 28, 2023

@OlhaBabicheva Can we look at the test failures?

@rusty1s
Copy link
Member

rusty1s commented Apr 28, 2023

Looks like it cannot find #include "mkl_vsl.h"?

@rusty1s rusty1s disabled auto-merge April 28, 2023 09:51
@OlhaBabicheva
Copy link
Contributor Author

Looks like it cannot find #include "mkl_vsl.h"?

I changed it recently to #include <mkl.h> (I saw that we use the same include in matmul_kernel.cpp ) ). However I'm still getting error during C++ testing (fatal error: mkl.h: No such file or directory)

@rusty1s
Copy link
Member

rusty1s commented May 4, 2023

@DamianSzwichtenberg Do you have any advice here?

@DamianSzwichtenberg
Copy link
Member

@DamianSzwichtenberg Do you have any advice here?

@rusty1s @OlhaBabicheva Yes, I think this specific error occurs, because test_rand_engine.cpp includes mkl directly via nested includes. So the compiler does not know, where this file is. A possible solution for that could be:

if(MKL_INCLUDE_FOUND)
    target_include_directories(${name} PRIVATE ${BLAS_INCLUDE_DIR})
endif()

put here.

@OlhaBabicheva
Copy link
Contributor Author

Any updates on this one?

I'm still working on this issue with C++ Testing but in another repo (to avoid spamming here):
image

@rusty1s
Copy link
Member

rusty1s commented May 23, 2023

Ok, let me know if I can help you with anything.

@OlhaBabicheva
Copy link
Contributor Author

@rusty1s Just wanted to let you know, that I fixed the building step (I had to change mkl to newer version in cpp_testing.yml), however there is one test (test 19) is failing

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #222 (f87fada) into master (0291934) will decrease coverage by 0.85%.
Report is 1 commits behind head on master.
The diff coverage is 68.00%.

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
- Coverage   83.69%   82.84%   -0.85%     
==========================================
  Files          28       28              
  Lines         883      921      +38     
==========================================
+ Hits          739      763      +24     
- Misses        144      158      +14     
Files Changed Coverage Δ
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp 83.22% <53.84%> (-5.51%) ⬇️
pyg_lib/csrc/random/cpu/rand_engine.h 94.44% <81.81%> (-5.56%) ⬇️
pyg_lib/csrc/sampler/cpu/mapper.h 89.18% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

.github/workflows/cpp_testing.yml Show resolved Hide resolved
pyg_lib/csrc/random/cpu/rand_engine.h Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@DamianSzwichtenberg
Copy link
Member

Cpp benchmark results:

----------------------------------------------------------------------------------
Benchmark                                        Time             CPU   Iterations
----------------------------------------------------------------------------------
BenchmarkRandEngineWithMKL/256                 186 ns          186 ns      3776603
BenchmarkRandEngineWithMKL/512                 374 ns          374 ns      1861909
BenchmarkRandEngineWithMKL/1024                600 ns          600 ns      1170031
BenchmarkRandEngineWithMKL/2048               1117 ns         1116 ns       626010
BenchmarkRandEngineWithMKL/4096               2199 ns         2199 ns       316950
BenchmarkRandEngineWithPrefetching/256        1972 ns         1972 ns       354124
BenchmarkRandEngineWithPrefetching/512        3951 ns         3951 ns       177682
BenchmarkRandEngineWithPrefetching/1024       7880 ns         7879 ns        88383
BenchmarkRandEngineWithPrefetching/2048      15779 ns        15779 ns        44113
BenchmarkRandEngineWithPrefetching/4096      31520 ns        31519 ns        22161

@OlhaBabicheva can you please re-do benchmarks from neighbor.py?

CHANGELOG.md Outdated Show resolved Hide resolved
@DamianSzwichtenberg DamianSzwichtenberg merged commit 4f34071 into pyg-team:master Aug 16, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants