mlpack Header only initiative #3091

shrit · 2021-11-13T20:00:55Z

This is a first attempt to move mlpack to a header-only, I am not managing to push anything further to this PR, I think we can merge this one first and then open another one for the rest.
This is only to make the reviewing process easier.

Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin

(requesting changes just so mlpack-bot doesn't auto-approve and this gets accidentally merged before it's ready)

shrit · 2021-11-20T16:40:59Z

I will have to check for these compilation errors, since I do not remember having any on my machine

Signed-off-by: Omar Shrit <omar@shrit.me>

shrit · 2021-11-20T23:15:49Z

@rcurtin, do you have an idea what is happening with the macOS builds?
The fix is simple, we only need to remove the math:: and declare that we are using the mlpack::math namespace.
However, I do not understand why the error is happening in the first place, this will require more refactoring everywhere, sure I can do that but I can not understand the reason, let me know if you have an idea about it.

rcurtin · 2021-12-01T00:45:17Z

@rcurtin, do you have an idea what is happening with the macOS builds? The fix is simple, we only need to remove the math:: and declare that we are using the mlpack::math namespace. However, I do not understand why the error is happening in the first place, this will require more refactoring everywhere, sure I can do that but I can not understand the reason, let me know if you have an idea about it.

Just to write what we figured out while digging into this together today, the issue appears to be caused by various using namespaces hidden in some header files throughout the codebase. I think if we fix those, the MacOS build should succeed... I hope! 😄

Signed-off-by: Omar Shrit <omar@shrit.me>

shrit · 2022-01-23T19:43:53Z

@rcurtin 👌
The patch works perfectly. It compiles without any issue locally, let us see if this passes on the CI
I am not going to add anything further for this PR, let us merge this one and then I will open another one for the remaining parts.
Whenever you have time one review should be fine.

rcurtin · 2022-01-24T17:15:22Z

Yeah, I have seen that, therefore I commented everything related to STB to check if it is compiling, and it was passing locally. The ideal solution is to find a header-only library that supports image loading, otherwise, it will become a nightmare to make STB header-only and in this case, we will have to ship our own modified STB with mlpack, which will add maintenance overhead
In fact, if we can find a header-only library for image loading this will simplify the code and remove a lot of #ifdef macros for finding STB, it will be like catch and it will update automatically with github action.
Also, I do not think the replacement will be that hard, maybe we need to rewrite 2 functions?

Which functions to rewrite? Maybe I can do that.

@shubham1206agra let's hold off on that one---I think that we can make STB work here. Maybe someday we can refactor our data loading utilities into a separate library.

rcurtin

Nice, great to see this all builds! I only have a couple small cleanup comments, and then I agree, we should go ahead and merge it. 👍

src/mlpack/core/data/image_info_impl.hpp

rcurtin · 2022-01-24T17:17:13Z

src/mlpack/core/data/save_image.hpp

@@ -1,5 +1,5 @@
 /**
- * @file core/data/save_image.cpp
+ * @file core/data/save_image.hpp


This comment still applies---do you think we can handle it before merge?

rcurtin · 2022-01-24T17:18:06Z

src/mlpack/core/math/lin_alg_impl.hpp

+ */
+inline void RandVector(arma::vec& v)
+{
+  v.zeros();


Agreed, I think we can remove this line (I know it was not part of the purpose of your PR, but it's a cleanup we could do anyway).

rcurtin · 2022-01-24T17:18:49Z

src/mlpack/methods/det/dtree_impl.hpp

@@ -95,7 +93,7 @@ void ExtractSplits(std::vector<std::pair<ElemType, size_t>>& splitVec,
                   const size_t minLeafSize)
 {
  // It's common sense, but we also use it in a check later.
-  Log::Assert(minLeafSize > 0);
+  mlpack::Log::Assert(minLeafSize > 0);


I am not sure, but I think that you can revert most of the changes to this file, since these functions are now in the mlpack::det namespace.

src/mlpack/methods/reinforcement_learning/q_networks/categorical_dqn.hpp

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Signed-off-by: Omar Shrit <omar@shrit.me>

src/mlpack/core/data/load_image_impl.hpp

src/mlpack/core/data/save_image.hpp

src/mlpack/methods/reinforcement_learning/q_networks/simple_dqn.hpp

Signed-off-by: Omar Shrit <omar@shrit.me>

src/mlpack/methods/det/dtree_impl.hpp

rcurtin

Everything looks good to me here---I think if you handle the comments in dtree_impl.hpp then the build will succeed, and once the build succeeds then from my side everything seems good and I think we should merge it. 👍

Signed-off-by: Omar Shrit <omar@shrit.me>

shrit · 2022-02-04T10:35:30Z

@rcurtin All tests are passing now, the main issue is the memory check

Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin

Awesome, the check is looking good; hopefully the comments are helpful. 👍

CMake/TestForSTB.cmake

CMake/stb/CMakeLists.txt

CMake/stb/main.cpp

CMake/stb/blib.hpp

rcurtin · 2022-04-13T23:10:05Z

@shrit I spent some time today trying to debug the CMake checks, and after my debugging I put them together into two patches you could apply to this branch with git am:

https://gist.github.com/rcurtin/4e54a7163533045cf4297dd91ba03383
https://gist.github.com/rcurtin/4fddb8784868b8e2f70f6aa985f8ac79

I tested this CMake script on both working and non-working versions of STB, and got the expected output. Once you apply these, I guess we'll have to update the version of STB in the mlpack jenkins Dockerfile (but that's easy enough).

Take a look and let me know what you think. 👍

Signed-off-by: Omar Shrit <omar@shrit.me>

shrit · 2022-04-16T15:23:17Z

@rcurtin Great. Thank you for the patch, much better than what I have done before.
We should merge this one now, nothing to add here.

rcurtin · 2022-04-16T17:54:07Z

@mlpack-jenkins test this please

src/mlpack/core/data/save_image.hpp

rcurtin

Everything seems fine to me now! It will be great to get this merged. It looks like the Go job is failing perhaps because gonum uploaded a failing version (I see the failure also in #3186).

Co-authored-by: Marcus Edel <marcus.edel@fu-berlin.de>

shrit · 2022-04-16T19:40:41Z

Yeah, the same for the python build.
I will merge it tomorrow morning.

zoq

Looks good to me.

shrit added 12 commits November 13, 2021 18:04

Make save image header only

31d5d6a

Signed-off-by: Omar Shrit <omar@shrit.me>

Finish inlining the data dir

1c3cb3e

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix comments and compilation bugs

0019156

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix the compilation warning related to include implementations

526971a

Signed-off-by: Omar Shrit <omar@shrit.me>

move the impl from .cpp to .impl

a8f6487

Signed-off-by: Omar Shrit <omar@shrit.me>

Adding missing headers

0894812

Signed-off-by: Omar Shrit <omar@shrit.me>

inline random_basis

3e165bf

Signed-off-by: Omar Shrit <omar@shrit.me>

remove epanechnikov_kernel

7558d38

Signed-off-by: Omar Shrit <omar@shrit.me>

Finishing the epanechnikov_kernel

7433fae

Signed-off-by: Omar Shrit <omar@shrit.me>

Deleting epanechnikov_kernel and adding pspectrum_string_kernel

c2c17b5

Signed-off-by: Omar Shrit <omar@shrit.me>

Finish the pspectrum_string_kernel

71ebcbc

Signed-off-by: Omar Shrit <omar@shrit.me>

Adding a missing std string

da95b27

Signed-off-by: Omar Shrit <omar@shrit.me>

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Nov 13, 2021

zoq added c: methods t: added feature and removed s: unanswered s: unlabeled labels Nov 15, 2021

conradsnicta approved these changes Nov 16, 2021

View reviewed changes

rcurtin requested changes Nov 16, 2021

View reviewed changes

shrit added 5 commits November 20, 2021 17:06

Let us if this resolves the binding issues

82fc871

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix the gmm error

e62d097

Signed-off-by: Omar Shrit <omar@shrit.me>

Adding missing math/random headers

880ea8f

Signed-off-by: Omar Shrit <omar@shrit.me>

Remove the mlpack:: namespace

66772db

Signed-off-by: Omar Shrit <omar@shrit.me>

Adjust namespace in preprocess

29daacd

Signed-off-by: Omar Shrit <omar@shrit.me>

Clean partly the namespace. Let us see if this resolve the macOS issues

6d57ec3

Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin reviewed Jan 24, 2022

View reviewed changes

shrit and others added 3 commits January 24, 2022 19:51

Update src/mlpack/core/data/image_info_impl.hpp

6793b9e

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update src/mlpack/core/data/image_info_impl.hpp

8d291a4

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Adjust namespace of mlpack::Log

8248409

Signed-off-by: Omar Shrit <omar@shrit.me>

zoq reviewed Jan 24, 2022

View reviewed changes

src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/core/data/save_image.hpp Outdated Show resolved Hide resolved

src/mlpack/methods/reinforcement_learning/q_networks/simple_dqn.hpp Outdated Show resolved Hide resolved

shrit added 2 commits January 24, 2022 22:15

Refactor save_image into save_image_impl finally done!!

58ffb75

Signed-off-by: Omar Shrit <omar@shrit.me>

Finish this PR

453a017

Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin reviewed Feb 1, 2022

View reviewed changes

src/mlpack/methods/det/dtree_impl.hpp Show resolved Hide resolved

rcurtin reviewed Feb 1, 2022

View reviewed changes

src/mlpack/methods/det/dtree_impl.hpp Outdated Show resolved Hide resolved

rcurtin reviewed Feb 1, 2022

View reviewed changes

src/mlpack/methods/det/dtree_impl.hpp Outdated Show resolved Hide resolved

rcurtin reviewed Feb 1, 2022

View reviewed changes

Adding the missing starts, shitty regexp

44d9748

Signed-off-by: Omar Shrit <omar@shrit.me>

Let us see inital try for STB test

8868c12

Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin reviewed Mar 15, 2022

View reviewed changes

CMake/TestForSTB.cmake Outdated Show resolved Hide resolved

CMake/TestForSTB.cmake Outdated Show resolved Hide resolved

CMake/stb/CMakeLists.txt Outdated Show resolved Hide resolved

CMake/stb/main.cpp Outdated Show resolved Hide resolved

CMake/stb/blib.hpp Outdated Show resolved Hide resolved

Apply @rcurtin modification to check STB version

eefb010

Signed-off-by: Omar Shrit <omar@shrit.me>

zoq reviewed Apr 16, 2022

View reviewed changes

src/mlpack/core/data/save_image.hpp Outdated Show resolved Hide resolved

rcurtin approved these changes Apr 16, 2022

View reviewed changes

mlpack-bot bot removed the s: needs review label Apr 16, 2022

Update src/mlpack/core/data/save_image.hpp

10f722b

Co-authored-by: Marcus Edel <marcus.edel@fu-berlin.de>

zoq approved these changes Apr 17, 2022

View reviewed changes

shrit merged commit fdc7af7 into mlpack:master Apr 17, 2022

shubham1206agra mentioned this pull request Apr 27, 2022

Header Only #2 #3195

Merged

rcurtin mentioned this pull request Jun 21, 2022

Adapt virtually everything else in mlpack to header-only #3233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mlpack Header only initiative #3091

mlpack Header only initiative #3091

shrit commented Nov 13, 2021

rcurtin left a comment

shrit commented Nov 20, 2021 •

edited

shrit commented Nov 20, 2021

rcurtin commented Dec 1, 2021

shrit commented Jan 23, 2022

rcurtin commented Jan 24, 2022

rcurtin left a comment

rcurtin Jan 24, 2022

rcurtin Jan 24, 2022

rcurtin Jan 24, 2022

rcurtin left a comment

shrit commented Feb 4, 2022

rcurtin left a comment

rcurtin commented Apr 13, 2022 •

edited

shrit commented Apr 16, 2022 •

edited

rcurtin commented Apr 16, 2022

rcurtin left a comment

shrit commented Apr 16, 2022

zoq left a comment

mlpack Header only initiative #3091

mlpack Header only initiative #3091

Conversation

shrit commented Nov 13, 2021

rcurtin left a comment

Choose a reason for hiding this comment

shrit commented Nov 20, 2021 • edited

shrit commented Nov 20, 2021

rcurtin commented Dec 1, 2021

shrit commented Jan 23, 2022

rcurtin commented Jan 24, 2022

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin Jan 24, 2022

Choose a reason for hiding this comment

rcurtin Jan 24, 2022

Choose a reason for hiding this comment

rcurtin Jan 24, 2022

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

shrit commented Feb 4, 2022

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin commented Apr 13, 2022 • edited

shrit commented Apr 16, 2022 • edited

rcurtin commented Apr 16, 2022

rcurtin left a comment

Choose a reason for hiding this comment

shrit commented Apr 16, 2022

zoq left a comment

Choose a reason for hiding this comment

shrit commented Nov 20, 2021 •

edited

rcurtin commented Apr 13, 2022 •

edited

shrit commented Apr 16, 2022 •

edited