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

Faster Python unittest #3210

Merged
merged 10 commits into from
Aug 5, 2017

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Aug 3, 2017

  • Do not use virtualenv in Docker
  • Split Python unittest into many target, make them parallel

* Do not use `virtualenv` in Docker
* Split unittest into many target, make them parallel
* because it uses `numpy.flip` in `test_image.py`
CMakeLists.txt Outdated
@@ -55,6 +55,7 @@ option(WITH_C_API "Compile PaddlePaddle with C-API(Prediction)" OFF)
option(WITH_GOLANG "Compile PaddlePaddle with GOLANG" OFF)
option(GLIDE_INSTALL "Download and install go dependencies " ON)
option(USE_NNPACK "Compile PaddlePaddle with NNPACK library" OFF)
option(UNITTEST_USE_VIRTUALENV "Python unittest with virtualenv" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need VirtualEnv when we are using Docker?

Copy link
Collaborator Author

@reyoung reyoung Aug 4, 2017

Choose a reason for hiding this comment

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

This PR is not to use Python virtualenv in Docker. Because in docker, Python packages are managed by Docker.

Not use virtualenv in the unit tests will make unit tests faster in docker.

@@ -49,29 +49,27 @@ cmake .. \
-DCUDNN_ROOT=/usr/ \
-DWITH_STYLE_CHECK=${WITH_STYLE_CHECK:-OFF} \
-DWITH_TESTING=${WITH_TESTING:-OFF} \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DUNITTEST_USE_VIRTUALENV=OFF
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should here refer to the flag defined in CMakeLists.txt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -22,7 +22,7 @@ setup(name="py_paddle",
package_data={'py_paddle':['*.py','_swig_paddle.so']},
install_requires = [
'nltk>=3.2.2',
'numpy>=1.8.0', # The numpy is required.
'numpy>=1.12.0', # The numpy is required.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a comment here explaining why 1.12 is required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

========================================
Building in /paddle/build ...
============================================
Building and installing in /paddle/build ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between building and installing? I think we used to build only here because the installation is postponed in building the production Docker image. Do we have to run make install here so could we run Python test? Or, could we run Python test without make install?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we do not use virtualenv for Python unit-test in Docker. Then we must install Paddle into that container. So we cannot run Python tests without make install.

Also, the script before split make and make install in two steps, but always make install later. So just call make install once is simpler.

Copy link
Collaborator Author

@reyoung reyoung Aug 4, 2017

Choose a reason for hiding this comment

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

What is the difference between building and installing? I think we used to build only here because the installation is postponed in building the production Docker image.

  1. make install always be invoked in current develop branch, https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/scripts/docker/build.sh#L72 .
  2. I think make install logic is part of our unit test too. We must ensure make install command can correctly install Paddle in container. And unit-tests should be based on installed Paddle.

@reyoung reyoung force-pushed the feature/fast_python_unittest branch 3 times, most recently from 83d8cbf to 985bf51 Compare August 5, 2017 08:07
* User must install Paddle python package before unittest.
* Or use docker to build Paddle
@reyoung reyoung force-pushed the feature/fast_python_unittest branch from 985bf51 to a720d21 Compare August 5, 2017 08:11
@reyoung reyoung merged commit 03a38b3 into PaddlePaddle:develop Aug 5, 2017
@reyoung reyoung deleted the feature/fast_python_unittest branch August 5, 2017 09:04
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.

2 participants