Skip to content
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

Merged
merged 10 commits into from
Aug 16, 2017

Conversation

Yancey1989
Copy link
Contributor

Fixed #3413

@Yancey1989 Yancey1989 requested a review from typhoonzero August 11, 2017 10:10
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -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}
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

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.

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
Copy link
Contributor

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.

Copy link
Contributor Author

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/...

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@tensor-tang tensor-tang Aug 14, 2017

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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'
Copy link
Contributor

@tensor-tang tensor-tang Aug 15, 2017

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

  1. Users could either use make install or pip install to install and then use paddle after that.
  2. The default install path is determined by cmake and pip, we shouldn't interfere with it. Let cmake and pip do their work.
  3. Maybe add some documentation about how to use --prefix parameter when installing, and the default installation behaviour?

Copy link
Contributor

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 ~

Copy link
Contributor Author

@Yancey1989 Yancey1989 Aug 15, 2017

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'
Copy link
Contributor

@tensor-tang tensor-tang Aug 15, 2017

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.

@luotao1
Copy link
Contributor

luotao1 commented Aug 15, 2017

去掉两个local后,我在本地测是可以的。import paddle, py_paddle, paddle.v2.framework.core都可以。
安装路径在下面:

 /home/luotao/.jumbo/bin/paddle
  /home/luotao/.jumbo/bin/paddle_merge_model
  /home/luotao/.jumbo/bin/paddle_pserver_main
  /home/luotao/.jumbo/bin/paddle_trainer
  /home/luotao/.jumbo/bin/paddle_usage
  /home/luotao/.jumbo/lib/libiomp5.so
  /home/luotao/.jumbo/lib/libmkldnn.so
  /home/luotao/.jumbo/lib/libmkldnn.so.0
  /home/luotao/.jumbo/lib/libmklml_intel.so
  /home/luotao/.jumbo/lib/python2.7/site-packages/paddle/__init__.py
  /home/luotao/.jumbo/lib/python2.7/site-packages/paddle/__init__.pyc
  /home/luotao/.jumbo/lib/python2.7/site-packages/paddle/proto/DataConfig_pb2.py
  /home/luotao/.jumbo/lib/python2.7/site-packages/paddle/proto/DataConfig_pb2.pyc
...

@tensor-tang
Copy link
Contributor

That's clean and perfect!
Thx @luotao1.

@Yancey1989
Copy link
Contributor Author

Thanks @luotao1 !

# install PaddlePaddle Python modules.
sudo pip install <path to install>/opt/paddle/share/wheels/*.whl
```
- Install PaddlePaddle with `make install`
Copy link
Contributor

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.

Copy link
Contributor Author

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
```
Copy link
Contributor

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
```
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

@typhoonzero typhoonzero left a 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.

@luotao1
Copy link
Contributor

luotao1 commented Aug 16, 2017

@Yancey1989

I think that ubuntu_install_en.rst and ubuntu_install_cn.rst can be removed.

I think we can use pip install instead of make install, so I deleted the make install

why you delete make install? If users build Paddle from source, they should use make install and then pip install.

@Yancey1989
Copy link
Contributor Author

Thanks @luotao1 , now all libraries and binary files are packaged into the whl pakcage, so maybe we can use pip install to install PaddlePaddle? If i don't understand enough, please told me ... thanks again.

@tensor-tang
Copy link
Contributor

For common users, they can directly pip install the released whl.
For developers, they could download, build and make install from the released source code.

I think we should give this choice to users and developers.

@Yancey1989
Copy link
Contributor Author

@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 pip install and make install, thanks again!

@Yancey1989
Copy link
Contributor Author

I create a new issue #3524 to discuss whether we can retain make install, we can discuss that one.
I will merge this PR and recover the document about installing PaddlePaddle, because of we need to fix executing paddle train/pserver failed in the latest Docker image,

@Yancey1989 Yancey1989 merged commit 0f86881 into PaddlePaddle:develop Aug 16, 2017
@Yancey1989 Yancey1989 deleted the fix_paddle_path branch August 16, 2017 08:09
heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants