Example of possible makefile for mlpack examples #75

shrit · 2020-04-07T21:50:38Z

Signed-off-by: Omar Shrit shrit@lri.fr

shrit · 2020-04-07T21:52:56Z

@rcurtin I think this is a possible example for a Makefile. I only made it for one example, if it is good, I will copy it everywhere with minor modifications.

birm · 2020-04-08T02:07:59Z

Maybe we could add this makefile to our CI tests.

zoq · 2020-04-08T19:12:51Z

Do we want to keep the current folder structure or do we want to put everything in the root directory? Depending on the decision, I think a single Makefile as all we need?

shrit · 2020-04-08T20:39:23Z

@zoq I would prefer to have separate Makefile by example. This will reduce the compilation time, and will be much easier to manage and debug. This is only a prototype if you find it well I will only copy it for all examples.

zoq · 2020-04-08T21:39:48Z

Maybe we can make it work for other examples? Like point users to this makefile and we make sure it works across all examples, and a user could call make example-name and that's it?

shrit · 2020-04-08T22:04:36Z

@zoq I agree, that is a good point, this will reduce non-useful duplication of Makefiles. Let us have @rcurtin opinion on this?

rcurtin · 2020-04-09T02:22:00Z

@shrit thanks for writing this up! I think I would agree that we should have one Makefile in each directory. My impression is that a user will come along and look at an individual directory, not at the whole project. But it seems like it would be easy to have the best of both worlds... just have a top-level Makefile call out recursively to all the subdirectories?

This is a really generic Makefile, which is nice because we could just drop it in other directories, but it might be a little hard to understand for people not too familiar with the (really weird!) Makefile language. In some sense even the Makefile itself is an example, so I'd hope to keep it as simple as we can. But I'm not sure exactly where the right level is. The configurable parameters at the top are really nice, but maybe we can hardcode the sources and compilation commands, so we get something like this?

all: mnist_simple

# Probably best to keep the non-optional parts of linking hard-coded in the command?
# Maybe good to add $(INCLFLAGS) containing any -I directories in case mlpack is not installed but only built locally?
mnist_simple:
    # If you're not using the Armadillo wrapper, replace `-larmadillo` with linker commands for the
    # BLAS and LAPACK libraries that you are using.
    $(CXX) -o mnist_simple mnist_simple.cpp $(CXXFLAGS) $(LDFLAGS) -lmlpack -larmadillo -lboost_serialization

clean:
    @rm mnist_simple

Might be good to put a disclaimer at the top or something, too, that says "you might need to modify this to get it to work on your system, but it should work in most places", or something like that. It's just an example after all, not meant to build in every possible place. 😄

Anyway, let me know what you think, these are just my opinions. We could go with something else if desired.

shrit · 2020-04-11T10:48:22Z

I agree, one makefile by example seems to be a good idea, with a recursive call for the entire project.
I have made it generic because I do not want users to touch the Target and OBJS. They can change only source files, and add libraries if they want to add new dependencies. I will try to remove some redundant stuff and see what we can get.

rcurtin

Hey @shrit, looks good! Thanks for simplifying. I left a couple comments about the experiences I had while trying to use it; let me know what you think. Personally I think this looks good overall; want to deploy it to the other directories too? 👍

mnist_simple/Makefile

rcurtin · 2020-04-26T21:44:25Z

mnist_simple/Makefile

+default: all
+
+$(TARGET): $(OBJS)
+	$(CXX) $(CXXFLAGS) $(INCLFLAGS) $(LIBS) $(LDFLAGS) $(OBJS) -o $(TARGET)


To make this work right on my system (I have to specify -L/home/ryan/src/mlpack/build/lib/ in LDFLAGS and -I/home/ryan/src/mlpack/build/include/ -I/home/ryan/src/mlpack/build/deps/ensmallen-2.11.5/include/ in INCLFLAGS), I had to move $(OBJS) -o $(TARGET) to the beginning of the command:

$(TARGET): $(OBJS) $(CXX) $(CXXFLAGS) $(OBJS) -o $(TARGET) $(INCLFLAGS) $(LDFLAGS) $(LIBS)

shrit · 2020-04-27T14:50:28Z

I think it is ready to be deployed on other examples.

rcurtin · 2020-04-28T02:16:54Z

mnist_vae_cnn/Makefile

+# LDFLAGS := -L/path/to/mlpack/library/    # if installed locally
+INCLFLAGS := -I . #Add header directories for any includes that aren't on the default compiler search path.
+
+OBJS := $(SRC:.cpp=.o)


Maybe do we need to specify a separate rule for the .cpp -> .o compilation? The .cpps are being compiled without the INCLFLAGS or CXXFLAGS:

$ head -18 Makefile # This is a simple Makefile used to build the example source code. # This example might requires some modifications in order to work correctly on # your system. # If you're not using the Armadillo wrapper, replace `armadillo` with linker commands # for the BLAS and LAPACK libraries that you are using. TARGET := mnist_simple SRC := mnist_simple.cpp LIBS_NAME := armadillo mlpack boost_serialization CXX := g++ CXXFLAGS := -std=c++11 -Wall -Wextra -O3 -DNDEBUG # Use these CXXFLAGS instead if you want to compile with debugging symbols and without optimizations. # CXXFLAGS := -std=c++11 -Wall -Wextra -g -O0 LDFLAGS := -fopenmp LDFLAGS := -L/home/ryan/src/mlpack/build/lib/ # if installed locally INCLFLAGS := -I/home/ryan/src/mlpack/build/include/ -I/home/ryan/src/mlpack/build/deps/ensmallen-2.11.5/include/ #Add header directories for any includes that aren't on the default compiler search path. $ make g++ -std=c++11 -Wall -Wextra -O3 -DNDEBUG -c -o mnist_simple.o mnist_simple.cpp mnist_simple.cpp:16:10: fatal error: mlpack/core.hpp: No such file or directory 16 | #include <mlpack/core.hpp> | ^~~~~~~~~~~~~~~~~ compilation terminated. make: *** [<builtin>: mnist_simple.o] Error 1

shrit · 2020-04-28T22:33:33Z

@rcurtin This one seems to be working, I kept one rule. If it seems good to you, I will deploy these modifications on other examples

rcurtin · 2020-04-29T02:07:10Z

Nice, that seems to work. It seems also like you could use += instead and CXXFLAGS everywhere? Here's a slightly modified version that might be cleaner:

CXX := g++
CXXFLAGS += -std=c++11 -Wall -Wextra -O3 -DNDEBUG -fopenmp
# Use these CXXFLAGS instead if you want to compile with debugging symbols and without optimizations.
# CXXFLAGS += -std=c++11 -Wall -Wextra -g -O0

LDFLAGS := -fopenmp -L/home/ryan/src/mlpack/build/lib/    # if installed locally
INCLFLAGS := -I/home/ryan/src/mlpack/build/include/ -I/home/ryan/src/mlpack/build/deps/ensmallen-2.11.5/include/ #Add header directories for any includes that aren't on the default compiler search path.

CXXFLAGS += $(INCLFLAGS)

(Of course, you would want to include the default INCLFLAGS and LDFLAGS that you have already.)

I think that using += everywhere could mean that I could set some CXXFLAGS in my environment and have those propagate to the Makefile, which is nice:

$ CXXFLAGS="-fail" make
g++ -fail -std=c++11 -Wall -Wextra -O3 -DNDEBUG -fopenmp -I/home/ryan/src/mlpack/build/include/ -I/home/ryan/src/mlpack/build/deps/ensmallen-2.11.5/include/    -c -o mnist_simple.o mnist_simple.cpp
g++: error: unrecognized command line option ‘-fail’; did you mean ‘-fnil’?
make: *** [<builtin>: mnist_simple.o] Error 1

Anyway, up to you what you want to do. Overall it looks great to me---want to apply that the changes to all the other Makefiles? I think this PR is ready once you do that. 👍

Signed-off-by: Omar Shrit <shrit@lri.fr>

Co-Authored-By: Ryan Curtin <ryan@ratml.org>

Signed-off-by: Omar Shrit <shrit@lri.fr>

shrit · 2020-04-30T22:21:29Z

@rcurtin I think we can merge it now,

rcurtin

Awesome, thanks @shrit! It looks good to me and works on my setup. My last super minor comment: maybe it's better to use += for the LDFLAGS too? Up to you if you want to make the change.

Signed-off-by: Omar Shrit <shrit@lri.fr>

zoq · 2020-05-01T20:39:50Z

lstm_electricity_consumption/Makefile

@@ -0,0 +1,36 @@
+# This is a simple Makefile used to build the example source code.
+# This example might requires some modifications in order to work correctly on


I think we are using the same style guidelines here as for the mlpack repo, so maybe we should adjust the line length here; see https://github.com/mlpack/mlpack/wiki/DesignGuidelines#line-length-and-wrapping for more details.

Agreed, I need to apply this on all files

lstm_electricity_consumption/Makefile

zoq · 2020-05-01T20:43:38Z

lstm_electricity_consumption/Makefile

@@ -0,0 +1,36 @@
+# This is a simple Makefile used to build the example source code.


Wondering if we should print a similar message in the build step, not sure the first thing a user will do is to look into the Makefile for more information. Let me know what you think.

I am not sure, If any system supports posix and have gcc installed, the user should not have any issue with this makefiles. The only possible modifications are setting LD_FLAGS and INCLUDES if required

I guess if there are issues we will see them, so fine with me to go with the current version.

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

mlpack-bot

Second approval provided automatically after 24 hours. 👍

Signed-off-by: Omar Shrit <shrit@lri.fr>

shrit · 2020-05-03T20:30:08Z

@rcurtin we can merge now

zoq

Thanks for putting everything together!

rcurtin · 2020-05-03T21:33:43Z

Thanks @shrit! 👍

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Apr 7, 2020

birm added c: examples and removed s: unanswered s: unlabeled labels Apr 8, 2020

rcurtin reviewed Apr 26, 2020

View reviewed changes

rcurtin reviewed Apr 28, 2020

View reviewed changes

kartikdutt18 mentioned this pull request Apr 28, 2020

Simplifying the Examples repository. #66

Closed

7 tasks

shrit and others added 10 commits April 29, 2020 11:40

Example of possible makefile for mlpack examples

2c25e40

Signed-off-by: Omar Shrit <shrit@lri.fr>

Simplify Makefile, Add some comments

c328374

Signed-off-by: Omar Shrit <shrit@lri.fr>

Update mnist_simple/Makefile

6893f7f

Co-Authored-By: Ryan Curtin <ryan@ratml.org>

Use two CXXFLAGS with comments

fd5c734

Co-Authored-By: Ryan Curtin <ryan@ratml.org>

Adding comments.

1b3135f

Signed-off-by: Omar Shrit <shrit@lri.fr>

Add makefile to mnist_batch_norm

d7461ac

Signed-off-by: Omar Shrit <shrit@lri.fr>

Add object files to clean list

7865ed4

Signed-off-by: Omar Shrit <shrit@lri.fr>

Adding makefiles for mnist

d033e97

Signed-off-by: Omar Shrit <shrit@lri.fr>

Adding makefiles for mnist vae cnn

5231297

Signed-off-by: Omar Shrit <shrit@lri.fr>

Adding makefiles for lstm_electricty consumption

a24eee2

Signed-off-by: Omar Shrit <shrit@lri.fr>

shrit added 3 commits April 29, 2020 11:40

Adding makefiles for lstm stock prediction

a8677cd

Signed-off-by: Omar Shrit <shrit@lri.fr>

Use implicit CPPFLAGS vairable for headers locations

88c641d

Signed-off-by: Omar Shrit <shrit@lri.fr>

Replace CPPFALGS by incremental CXXFLAGS

7147651

Signed-off-by: Omar Shrit <shrit@lri.fr>

shrit force-pushed the makefiles branch from 4ba4169 to 7147651 Compare April 29, 2020 10:01

Apply modifications on all examples

55bab7b

Signed-off-by: Omar Shrit <shrit@lri.fr>

rcurtin mentioned this pull request Apr 30, 2020

CMakeLists.txt removed but no Makefile added? #80

Closed

rcurtin approved these changes May 1, 2020

View reviewed changes

Apply += on LDFLAGS

c72e987

Signed-off-by: Omar Shrit <shrit@lri.fr>

zoq reviewed May 1, 2020

View reviewed changes

shrit and others added 2 commits May 2, 2020 00:21

Use Capital letter

c285554

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

Move comment ahead of variable

55fb8ca

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

mlpack-bot bot approved these changes May 2, 2020

View reviewed changes

mlpack-bot bot removed the s: needs review label May 2, 2020

Apply mlpack comments style on makefiles

3cfd68f

Signed-off-by: Omar Shrit <shrit@lri.fr>

zoq approved these changes May 3, 2020

View reviewed changes

rcurtin merged commit 97d2707 into mlpack:master May 3, 2020

shrit deleted the makefiles branch May 3, 2020 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Example of possible makefile for mlpack examples #75

Example of possible makefile for mlpack examples #75

shrit commented Apr 7, 2020

shrit commented Apr 7, 2020

birm commented Apr 8, 2020

zoq commented Apr 8, 2020

shrit commented Apr 8, 2020 •

edited

Loading

zoq commented Apr 8, 2020

shrit commented Apr 8, 2020

rcurtin commented Apr 9, 2020

shrit commented Apr 11, 2020 •

edited

Loading

rcurtin left a comment

rcurtin Apr 26, 2020

shrit commented Apr 27, 2020

rcurtin Apr 28, 2020

shrit commented Apr 28, 2020

rcurtin commented Apr 29, 2020

shrit commented Apr 30, 2020

rcurtin left a comment

zoq May 1, 2020

shrit May 1, 2020

zoq May 1, 2020

shrit May 3, 2020

zoq May 3, 2020

mlpack-bot bot left a comment

shrit commented May 3, 2020

zoq left a comment

rcurtin commented May 3, 2020

		@@ -0,0 +1,36 @@
		# This is a simple Makefile used to build the example source code.
		# This example might requires some modifications in order to work correctly on

Example of possible makefile for mlpack examples #75

Example of possible makefile for mlpack examples #75

Conversation

shrit commented Apr 7, 2020

shrit commented Apr 7, 2020

birm commented Apr 8, 2020

zoq commented Apr 8, 2020

shrit commented Apr 8, 2020 • edited Loading

zoq commented Apr 8, 2020

shrit commented Apr 8, 2020

rcurtin commented Apr 9, 2020

shrit commented Apr 11, 2020 • edited Loading

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin Apr 26, 2020

Choose a reason for hiding this comment

shrit commented Apr 27, 2020

rcurtin Apr 28, 2020

Choose a reason for hiding this comment

shrit commented Apr 28, 2020

rcurtin commented Apr 29, 2020

shrit commented Apr 30, 2020

rcurtin left a comment

Choose a reason for hiding this comment

zoq May 1, 2020

Choose a reason for hiding this comment

shrit May 1, 2020

Choose a reason for hiding this comment

zoq May 1, 2020

Choose a reason for hiding this comment

shrit May 3, 2020

Choose a reason for hiding this comment

zoq May 3, 2020

Choose a reason for hiding this comment

mlpack-bot bot left a comment

Choose a reason for hiding this comment

shrit commented May 3, 2020

zoq left a comment

Choose a reason for hiding this comment

rcurtin commented May 3, 2020

shrit commented Apr 8, 2020 •

edited

Loading

shrit commented Apr 11, 2020 •

edited

Loading