-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Fix invalid paddle binary file path #3421
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
paddle/scripts/submit_local.sh.in
Outdated
| ' 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
| 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"
/usrI 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
| from setuptools import setup, Distribution | ||
| class BinaryDistribution(Distribution): | ||
| def has_ext_modules(foo): | ||
| return True |
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 installorpip installto 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
--prefixparameter 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_BINARY_DIR}/paddle/scripts/paddle'] | ||
|
|
||
| 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?
typhoonzero
left a comment
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.
|
@Yancey1989 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