-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Fix invalid paddle binary file path #3421
Conversation
paddle/scripts/submit_local.sh.in
Outdated
@@ -100,7 +100,7 @@ import setuptools | |||
print str(packaging.version.Version("@PADDLE_VERSION@")) | |||
' 2>/dev/null) | |||
BASEDIR=$(dirname "$0") | |||
pip install ${BASEDIR}/../opt/paddle/share/wheels/*-${PYTHON_PADDLE_VERSION}-*.whl | |||
pip install ${BASEDIR}/../../opt/paddle/share/wheels/*-${PYTHON_PADDLE_VERSION}-*.whl |
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.
If we all switch to install using pip install
this may not be needed?
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.
Done.
paddle/scripts/submit_local.sh.in
Outdated
@@ -112,13 +112,13 @@ fi | |||
|
|||
case "$1" in | |||
"train") | |||
${DEBUGGER} $MYDIR/../opt/paddle/bin/paddle_trainer ${@:2} | |||
${DEBUGGER} $MYDIR/../../opt/paddle/bin/paddle_trainer ${@:2} |
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.
If install with pip install
, the paddle
script will be under /usr/local/bin/paddle
but binaries are under somewhere like: /usr/lib/python2.7/site-packages/usr/opt/paddle/bin
.
So the correct way is to check both where paddle
installed and the python package installed, and find where the binaries are.
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.
Just for your reference:
I build and installed with root
, then pip install whl
, and got paddle
in /usr/bin
and the binaries are in /usr/lib64/python2.7/site-packages/usr/local/opt/paddle/bin/
.
Since paddle
is installed with scripts
, however other binaries are installed with data_files
in setup.py.in
.
I thought maybe them could be installed with the same way, then it should be much easier to find the correct binaries.
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.
Thanks @typhoonzero and @tensor-tang , I think the binary file paddle_train/paddle_pserver_main/...
is placed under two different directories in the Docker image just because we installed the deb package while building production Docker image.For now we can install paddle with whl
instead of deb
package, I'll update this PR and please review after a moment.
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.
Done.
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.
@tensor-tang , I tried to modify data_files
to the absolute path /usr/local/opt/paddle
, but maybe date_files
will always be installed under python packages directory, here is the document from https://docs.python.org/2.7/distutils/setupscript.html#installing-additional-files:
Each file name in files is interpreted relative to the setup.py script at the top of the package source distribution.
paddle/scripts/submit_local.sh.in
Outdated
IFS=';' items=($PYTHON_SITEPACKAGES_PATHES) | ||
for item in ${items[@]}; do | ||
if [ -f $item/usr/local/opt/paddle/bin/paddle_trainer ]; then | ||
PADDLE_BIN_PATH=$item/usr/local/opt/paddle/bin |
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.
PADDLE_BIN_PATH
should also check if binaries are under /usr/local/opt/...
As mentioned by @tensor-tang, you can change the binaries installation dir in setup.py.in
but either way is OK, it's up to you.
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.
As the comment #3421 (comment), may be date_files
does not support the real absolute path...
PADDLE_BIN_PATH should also check if binaries are under /usr/local/opt/...
I'm not sure why we also need to check /usr/local/opt/...
, *.whl
will never install binary file under /usr/local/opt/...
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 try to correct this path in other PR #3461
It may fix your issue, if works.
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.
Cool, thanks @tensor-tang , it works correctly. But for my test, the directory
should be configured as opt/paddle/bin
and the binary files will be installed under /usr/local/opt/paddle/bin
.
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.
configured as opt/paddle/bin and the binary files will be installed under /usr/local/opt/paddle/bin.
which means your sys.prefix is /usr/local
? That's weird.
Or some var were set in your env, before pip install
?
Could you please check print sys.prefix
? I tried login as root and user. The result are both /usr
.
BTW my OS CentOS7.2.
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.
Yep, it's also confusing for me, print sys.prefix
will print /usr
.
root@6a4bc12ae305:/# python -c "import sys; print sys.prefix"
/usr
I executed pip install *.whl
in production Docker image, the base image is ubuntu:16.04
.
And if the data_files was configured as local/opt/paddle/bin
, the binary files would be installed under /usr/local/local/opt/paddle/bin
, the duplicate local
directory.
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.
Can you try pip install --prefix='/usr' *.whl
in docker?
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.
Yep, if I specify the --prefix='/usr'
, paddle will be installed under /usr
.
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.
Thanks @Yancey1989
@@ -24,7 +24,7 @@ if '${CMAKE_SYSTEM_PROCESSOR}' not in ['arm', 'armv7-a', 'aarch64']: | |||
setup_requires+=["opencv-python"] | |||
|
|||
# the prefix is sys.prefix which should always be usr | |||
paddle_bin_dir = 'local/opt/paddle/bin' | |||
paddle_bin_dir = 'opt/paddle/bin' |
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.
This should cause difference path when use cmake install
.
cmake install
: /usr/local/opt/paddle/bin
pip install
: /usr/opt/paddle/bin
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.
Thanks @tensor-tang , but maybe it doesn't matter for the different path?
If users want to install paddle to a customer directory, he/she could set --prefix=<customer directory>
, but in the production Docker image, we just make it running correctly.
BTW, the directory in date_files
wouldn't be configured local/opt/paddle/bin
, this will caused paddle be installed under /usr/local/local/opt/paddle/bin
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.
@tensor-tang , paddle_bin_files
is executable files, so I move these to scripts
int setup.py.
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 think make install
and pip install
will install to different default location doesn't harm using.
- Users could either use
make install
orpip install
to install and then use paddle after that. - The default install path is determined by cmake and pip, we shouldn't interfere with it. Let cmake and pip do their work.
- Maybe add some documentation about how to use
--prefix
parameter when installing, and the default installation behaviour?
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.
but maybe it doesn't matter for the different path?
I am not sure about this, maybe @luotao1 @typhoonzero can answer this?
Maybe add some documentation about how to use --prefix parameter when installing, and the default installation behaviour?
Totally agreed~
Anyway, I am OK with the both ways, just giving a method for this case.
You can change them as you wish. I think the efficient way is the best~
Thanks @Yancey1989 @typhoonzero ~
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.
Done with the document for installing PaddlePaddle.
And thanks @typhoonzero and @tensor-tang !!
|
||
paddle_rt_lib_dir = 'local/lib' | ||
paddle_rt_lib_dir = 'lib' |
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.
Maybe this change would impact "pip install " without docker for user.
Since the unit test for this has not ready yet @luotao1
Anyway, if /usr/lib
and /usr/local/lib
are both in LD_LIBRARY_PATH
, then it won't be a problem for users.
去掉两个local后,我在本地测是可以的。import paddle, py_paddle, paddle.v2.framework.core都可以。
|
That's clean and perfect! |
Thanks @luotao1 ! |
# install PaddlePaddle Python modules. | ||
sudo pip install <path to install>/opt/paddle/share/wheels/*.whl | ||
``` | ||
- Install PaddlePaddle with `make install` |
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.
Must inform that choose one of these two methods, not both.
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 think we can use pip install
instead of make install
, so I deleted the make install
export PATH=<path to install>/bin:$PATH | ||
# install PaddlePaddle Python modules. | ||
sudo pip install <path to install>/opt/paddle/share/wheels/*.whl | ||
``` |
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.
export PATH=<path to install>/bin:$PATH
+ # install PaddlePaddle Python modules.
+ sudo pip install <path to install>/opt/paddle/share/wheels/*.whl
are no longer needed.
export PATH=<path to install>/bin:$PATH | ||
# install PaddlePaddle Python modules. | ||
sudo pip install <path to install>/opt/paddle/share/wheels/*.whl | ||
``` |
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.
+ # set PaddlePaddle installation path in ~/.bashrc
+ export PATH=<path to install>/bin:$PATH
+ # install PaddlePaddle Python modules.
+ sudo pip install <path to install>/opt/paddle/share/wheels/*.whl
are no longer needed.
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.
File ubuntu_install_en.rst
and ubuntu_install_cn.rst
seems no longer needed.
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'm not sure what ubuntu_install_en.rst
and ubuntu_install_cn.rst
used for, maybe @luotao1 knows more?
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++
maybe we can remove ubuntu_install_en.rst
in another PR.
I think that
why you delete |
Thanks @luotao1 , now all libraries and binary files are packaged into the whl pakcage, so maybe we can use |
For common users, they can directly I think we should give this choice to users and developers. |
@tensor-tang , for users, I think he/she can pip install from pypi, or use production Dcoker image directly, and only develops would like to download/develop/build/install PaddlePaddle. But i aggree with you, I will retain the two partment of |
I create a new issue #3524 to discuss whether we can retain |
Fixed #3413