Add cmake for majel by gangliao · Pull Request #2118 · PaddlePaddle/Paddle

gangliao · 2017-05-12T16:16:14Z

issue the command: ctest -V -R majel_tests

 ctest -V -R majel_tests
UpdateCTestConfiguration  from :/Users/liaogang/baidu/Paddle/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/Users/liaogang/baidu/Paddle/build/DartConfiguration.tcl
Test project /Users/liaogang/baidu/Paddle/build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 53
    Start 53: majel_tests

53: Test command: /Users/liaogang/baidu/Paddle/build/paddle/majel/test/majel_tests
53: Test timeout computed to be: 9.99988e+06
53: [==========] Running 3 tests from 1 test case.
53: [----------] Global test environment set-up.
53: [----------] 3 tests from Place
53: [ RUN      ] Place.Equality
53: [       OK ] Place.Equality (0 ms)
53: [ RUN      ] Place.Default
53: [       OK ] Place.Default (0 ms)
53: [ RUN      ] Place.Print
53: [       OK ] Place.Print (0 ms)
53: [----------] 3 tests from Place (0 ms total)
53:
53: [----------] Global test environment tear-down
53: [==========] 3 tests from 1 test case ran. (0 ms total)
53: [  PASSED  ] 3 tests.
1/1 Test #53: majel_tests ......................   Passed    0.00 sec

The following tests passed:
        majel_tests

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.01 sec

gangliao · 2017-05-12T16:27:24Z

paddle/majel/CMakeLists.txt

@@ -0,0 +1,15 @@
+set(MAJEL_CXX_FILES place.cpp)


PADDLE includes both CPU and GPU versions. Majel is built with GPU only if it equipped one.

If CMake detects the host machine equipped GPUs, it will invoke cuda_add_library to generate the library, vice visa.

NOTE: The suffix of unit test files should strictly limit to *.cpp, because it doesn't contain any CUDA syntaxes and semantics.

If CMake detects the host machine equipped GPUs, it will invoke cuda_add_library to generate the library, vice visa.

If we are going to port Majel to Paddle, I think we will have to enable GPU and CPU versions. Also, we'll have to be able to specify which one to build, as we do with Paddle. Also, I think we shouldn't assume that the development computer having a GPU means that the runtime environment would have GPUs.

NOTE: The suffix of unit test files should strictly limit to .cpp, because it doesn't contain any CUDA syntaxes and semantics.

I agree that if a source file doesn't include CUDA code, the suffix shouldn't be .cu. But I personally prefer .cc over .cpp following Google C++ Style Guide.

Explained in #2122

helinwang · 2017-05-12T16:33:17Z

我看到paddle/CMakeLists.txt也有更改。现在能够单独编译majel吗（不编译paddle，实在太慢了）？

gangliao · 2017-05-12T17:12:49Z

@helinwang @wangkuiyi 现在两种模式都支持了。

快速开发模式

cd Paddle/paddle/majel
mkdir build && cmake ..
make
ctest -V -R majel_tests

majel in paddle

cd Paddle
mkdir build && cmake ..
make
ctest -V -R majel_tests

wangkuiyi · 2017-05-12T17:14:48Z

paddle/majel/CMakeLists.txt

@@ -0,0 +1,15 @@
+set(MAJEL_CXX_FILES place.cpp)


If CMake detects the host machine equipped GPUs, it will invoke cuda_add_library to generate the library, vice visa.

If we are going to port Majel to Paddle, I think we will have to enable GPU and CPU versions. Also, we'll have to be able to specify which one to build, as we do with Paddle. Also, I think we shouldn't assume that the development computer having a GPU means that the runtime environment would have GPUs.

wangkuiyi · 2017-05-12T17:16:06Z

paddle/majel/CMakeLists.txt

@@ -0,0 +1,15 @@
+set(MAJEL_CXX_FILES place.cpp)


NOTE: The suffix of unit test files should strictly limit to .cpp, because it doesn't contain any CUDA syntaxes and semantics.

I agree that if a source file doesn't include CUDA code, the suffix shouldn't be .cu. But I personally prefer .cc over .cpp following Google C++ Style Guide.

wangkuiyi · 2017-05-12T17:19:29Z

paddle/majel/test/test_framework.cpp

@@ -0,0 +1,6 @@
+#include "gtest/gtest.h"


Why we need this file? I think we can just link -lgtest_main to have the main function?

Explained in #2122

wangkuiyi · 2017-05-12T17:21:20Z

paddle/majel/test/place_test.cpp

-#include "gtest/gtest.h"
 #include "majel/place.h"
 #include <sstream>
+#include "gtest/gtest.h"


Sorry that I forgot to reorder the include files following https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. You can do that in this PR.

I think the right order is:

#include "majel/place.h" #include <sstream> #include "gtest/gtest.h"

Explained in #2122

wangkuiyi · 2017-05-12T17:23:12Z

paddle/majel/CMakeLists.txt

@@ -0,0 +1,34 @@
+cmake_minimum_required(VERSION 3.0)
+
+if(GTEST_INCLUDE_DIR AND GTEST_LIBRARIES)


Given that we are building Docker container, we can assume in CMakeLists.txt that we have boost and gtest already installed and don't need conditions like this, isn't it?

Explained in #2122

helinwang · 2017-05-12T19:03:41Z

paddle/majel/CMakeLists.txt

+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
+
+    # enable gtest
+    set(THIRD_PARTY_PATH ${CMAKE_CURRENT_SOURCE_DIR}/third_party)


third_party要不要放到运行cmake的目录（比如./build/）里面，而不是${CMAKE_CURRENT_SOURCE_DIR}里面?这样用户如果删了./build/就肯定是一个clean build。

Explained in #2122

helinwang · 2017-05-12T19:12:02Z

paddle/CMakeLists.txt

 add_subdirectory(trainer)
 add_subdirectory(scripts)

+find_package(boost QUIET)


我记得design doc里说用轻量级的boost替代品，这里仍然是用的boost吗？

这里是boost，不是替代品。

Explained in #2122

helinwang · 2017-05-12T19:15:06Z

paddle/CMakeLists.txt

+
+if(Boost_FOUND)
+  include_directories(${Boost_INCLUDE_DIRS})
+  include_directories(${CMAKE_CURRENT_SOURCE_DIR})


我对cmake不是很熟悉，为何这里要把当前目录给include了？repo/paddle下面貌似没有source code呀。

Explained in #2122

helinwang · 2017-05-12T20:29:57Z

paddle/majel/CMakeLists.txt

+    # enable gtest
+    set(THIRD_PARTY_PATH ${CMAKE_CURRENT_SOURCE_DIR}/third_party)
+    set(WITH_TESTING ON)
+    include(external/gtest)


Could you explain where does "external" directory come from (have not found any directory named "external" after building majel)? This line works, but I don't understand how it works...

Explained in #2122

helinwang · 2017-05-12T20:42:44Z

paddle/majel/test/CMakeLists.txt

+file(GLOB_RECURSE ALL_TEST_FILES RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}" "*.cpp" "*.cc")
+
+add_executable(majel_tests ${ALL_TEST_FILES})
+add_dependencies(majel_tests majel)


请问为何既然有了

add_dependencies(majel_tests majel)

还需要

target_link_libraries(majel_tests ... majel )

感觉不是重复地指定依赖了？
我试了一下删掉add_dependencies，貌似也能编译。

Explained in #2122

wangkuiyi

I noticed that this PR is moving some source files. My recent work on malloc depends on the source tree structure, so I am approving and merging this PR. But please @gangliao take a look at the discussions and raise new issues if viable. Thanks!

gangliao added 4 commits May 13, 2017 00:11

add cmake for majel

b0f070e

move majel into paddle/majel

3a73455

remove gtest in dockerfile

9c501e9

precommit

92e0c89

gangliao commented May 12, 2017

View reviewed changes

gangliao requested review from JiayiFeng, QiJune, helinwang and wangkuiyi May 12, 2017 16:28

gangliao added 2 commits May 13, 2017 01:09

support build majel in subdir

d8c8f41

clang-format

05b3196

refine format

d444251

wangkuiyi reviewed May 12, 2017

View reviewed changes

helinwang reviewed May 12, 2017

View reviewed changes

wangkuiyi approved these changes May 12, 2017

View reviewed changes

wangkuiyi merged commit f9c21f5 into PaddlePaddle:develop May 12, 2017

gangliao mentioned this pull request May 13, 2017

Clear up CMake confusion under Majel Directory #2122

Closed

wangkuiyi mentioned this pull request May 15, 2017

CMake's find_package requires Boost capitalized #2148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cmake for majel#2118

Add cmake for majel#2118
wangkuiyi merged 7 commits intoPaddlePaddle:developfrom
gangliao:new_tensor

gangliao commented May 12, 2017

gangliao May 12, 2017 •

edited

Loading

wangkuiyi May 12, 2017 •

edited by gangliao

Loading

wangkuiyi May 12, 2017

gangliao May 13, 2017

helinwang commented May 12, 2017

gangliao commented May 12, 2017

wangkuiyi May 12, 2017 •

edited by gangliao

Loading

wangkuiyi May 12, 2017

wangkuiyi May 12, 2017

gangliao May 13, 2017

wangkuiyi May 12, 2017

gangliao May 13, 2017

wangkuiyi May 12, 2017

gangliao May 13, 2017

helinwang May 12, 2017 •

edited

Loading

gangliao May 13, 2017

helinwang May 12, 2017

wangkuiyi May 12, 2017

gangliao May 13, 2017

helinwang May 12, 2017

gangliao May 13, 2017

helinwang May 12, 2017

gangliao May 13, 2017

helinwang May 12, 2017

gangliao May 13, 2017

wangkuiyi left a comment

Labels

3 participants

		@@ -0,0 +1,34 @@
		cmake_minimum_required(VERSION 3.0)

		if(GTEST_INCLUDE_DIR AND GTEST_LIBRARIES)

Conversation

gangliao commented May 12, 2017

gangliao May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

wangkuiyi May 12, 2017 • edited by gangliao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang commented May 12, 2017

gangliao commented May 12, 2017

wangkuiyi May 12, 2017 • edited by gangliao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi left a comment

Choose a reason for hiding this comment

Labels

3 participants

gangliao May 12, 2017 •

edited

Loading

wangkuiyi May 12, 2017 •

edited by gangliao

Loading

wangkuiyi May 12, 2017 •

edited by gangliao

Loading

helinwang May 12, 2017 •

edited

Loading