-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add Majel's Place concept to Paddle #2091
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
Conversation
Dockerfile
Outdated
curl sed grep graphviz libjpeg-dev zlib1g-dev \ | ||
python-numpy python-matplotlib gcc g++ \ | ||
automake locales clang-format-3.8 swig doxygen cmake \ | ||
liblapack-dev liblapacke-dev libboost-dev \ |
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.
@wangkuiyi It's better to replace boost using some light-weight implementation https://github.com/mapbox/variant since it's too heavy.
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.
https://github.com/PaddlePaddle/Paddle/blob/develop/cmake/external/glog.cmake#L29 It's a good way to integrate external project via CMake.
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.
About boost
I listed all boost packages that Majel depends on:
$ for i in $(du -a | grep '\.h$' | cut -f 2); do \
grep 'boost::' $i; \
done | \
grep -o 'boost::[a-zA-Z_]*' | sort | uniq
boost::apply_visitor
boost::bad_get
boost::get
boost::python
boost::shared_ptr
boost::static_visitor
boost::variant
I am afraid that we cannot replace all of them using some lighter weighted versions?
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 and agree that we should use CMake and its external projects. I didn't do it in this PR because I want to figure out what is the command line we need to build Majel.
@@ -0,0 +1,10 @@ | |||
CCFLAGS = -std=c++11 -I/work/paddle |
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.
After this PR is merged, I will add a CMakeLists.txt.
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.
LGTM
// needed for variant equality comparison | ||
inline bool operator==(const GpuPlace& o) const { return device == o.device; } | ||
|
||
inline bool operator!=(const GpuPlace& o) const { return !(*this == o); } |
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.
Why the above "==" operation check equal as device == o.device
, but here checks if it's the same instance?
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 are right! Is Greg nearby and can you ask him about the problem?
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.
My bad, I thought *this
is this
. In this case, it's taking the neg of result from == operator.
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.
@wangkuiyi @helinwang I think it's correct. Because *this == o
will trigger inline bool operator==(const GpuPlace& o)
So it's still compare device id.
Fixes #2090
Note: Please review and merge #2040 before this. In order to build the ported Majel code, I am using the development Docker image.