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

Major change in the installation and testing of packages using multiple python versions #789

Merged
merged 64 commits into from Apr 18, 2018

Conversation

@lekshmideepu
Copy link
Contributor

@lekshmideepu lekshmideepu commented Jul 20, 2017

This PR consists of a number of changes in the installation and testing processes:

  1. Using multiple python versions. #763
  2. Using the container based whitelist packages from Travis. #758
  3. Explicitly defining the CMake options as the system unable to recognise the python libraries.
  4. Some code changes to adapt to both python2 and python3
  5. Moved the package (cppcheck and CLANG_FORMAT) installation from build.sh to yaml. #759
  6. Changes in the message passing numbers for the parse log.
…ravis python images,multiple python versions
@lekshmideepu lekshmideepu requested a review from jougs Jul 20, 2017
@lekshmideepu lekshmideepu force-pushed the lekshmideepu:trustycontainer branch from 603e5d4 to 9eb3d80 Jul 20, 2017
Copy link
Contributor

@jougs jougs left a comment

Many thanks for this contribution. I have added several questions and comments, which I'd like to have answered before approving a merge.

.travis.yml Outdated
# Change directory back to the NEST source code directory.
- cd $SOURCEDIR
# terminaltables is required by parse_travis_log.py to create the build summary.
- pip install cython scipy matplotlib terminaltables mpi4py

This comment has been minimized.

@jougs

jougs Jul 23, 2017
Contributor

scipy, matplotlib and mpi4py are already installed above in the apt block. Is pip installing these packages a requirement so that the test setup works with the different Python versions?

Can you please either remove the packages either here (my preferred solution if that works with testing different Python versions) or above, so they are not installed multiple times? Also, could cython and terminaltables be installed using apt-get?

This comment has been minimized.

@lekshmideepu

lekshmideepu Jul 24, 2017
Author Contributor

@jougs : Thanks for the review comments. Oh yes, sorry for not removing the same from the apt block. I modified this part.
The version of those packages were bit old and it was affecting our tests. That is the reason why I have installed the same including mpi4py using pip.
Package terminaltables is not yet available in the apt package whitelist. I have already opened an issue for that.

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017
Member

If scipy is install from pip, I'm not sure I understand why we are using the deprecated version, nor why it is still present on line 77

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 5, 2018
Author Contributor

No more using the deprecated version. Since the version of some packages like scipy is old in the source package whitelist, I am installing those via pip

build.sh Outdated
tar xf clang+llvm-3.6.2-x86_64-linux-gnu-ubuntu-14.04.tar.xz
# Copy and not move because '.cache' may aleady contain other subdirectories and files.
cp -R clang+llvm-3.6.2-x86_64-linux-gnu-ubuntu-14.04/* $HOME/.cache
echo "MSGBLD0060: CLANG-FORMAT installation completed."

This comment has been minimized.

@jougs

jougs Jul 23, 2017
Contributor

Previously we required this exact version of clang-format. Is that not the case anymore? Where is clang-format installed now?

This comment has been minimized.

@lekshmideepu

lekshmideepu Jul 24, 2017
Author Contributor

@jougs : clang-format --version : clang-format version 3.5.0 (tags/RELEASE_350/final)
$ which clang-format : /usr/local/clang-3.5.0/bin/clang-format

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017
Member

I don't understand the answer... is the formatting with CLANG 3.5 the same as that of 3.6? If it is not the case, then a lot of tests will fail

This comment has been minimized.

@jougs

jougs Aug 3, 2017
Contributor

I agree with @Silmathoron. Our C++ Coding Guidelines explicitly point out that we require clang-format 3.6.

Can you please double-check if all checks run properly using this old version? One way to check would probably be to manually check all our source code files in a TravisCI debug build.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 5, 2018
Author Contributor

I revert it back to clang-format 3.6.2

@@ -42,6 +42,7 @@
'libltdl',
'.git',
'CMakeFiles',
'music',

This comment has been minimized.

@jougs

jougs Jul 23, 2017
Contributor

See above.

./autogen.sh
./configure --prefix=$HOME/.cache/music.install
make
make install

This comment has been minimized.

@jougs

jougs Jul 23, 2017
Contributor

Instead of excluding the music directory from copyright header checks below, I think it's cleaner to rm -rf the source directory after the make install step here.

This comment has been minimized.

@lekshmideepu

lekshmideepu Jul 24, 2017
Author Contributor

@jougs Thanks for the suggestion. I have added the same

conns = self.comm.allgather(conns, connstotal)
conns = filter(None, conns)
conns = self.comm.allgather(conns)
# conns = filter(None, conns)

This comment has been minimized.

@jougs

jougs Jul 23, 2017
Contributor

Please remove the commented out code. This also applies to the occurences below.

This comment has been minimized.

@lekshmideepu

lekshmideepu Jul 24, 2017
Author Contributor

@jougs : I have removed the commented out code.

Copy link
Member

@Silmathoron Silmathoron left a comment

Hey @lekshmideepu Thanks for trying to make the travis tests compatible with Py2/3.
However, some sections of the travis file seem to do the same thing, or to make the other unnecessary, at least from my point of view...

Could you clarify:

  1. the necessity of the deprecated branch,
  2. the problems with scipy and exactly which version you want (looks like in the end you use that of pip),
  3. why you do not use the include keyword in matrix?
.travis.yml Outdated
#https://blog.travis-ci.com/2017-06-19-trusty-updates-2017-Q2
group: edge
group: deprecated-2017Q2

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017
Member

From that page, this would only be valid until 9/1/17...
Is this still related to the python versions problem that was discussed during the VC?
Otherwise can we not fix whatever goes wrong instead of working with the deprecated image?

This comment has been minimized.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 5, 2018
Author Contributor

No more using the deprecated branch.

.travis.yml Outdated
matrix:
# Do notify immediately about it when a job of a build fails.
fast_finish: true
exclude:

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017
Member

This is a lot of lines for little done... would the following not work? Remove 3.4.4 from python on L46 and add

matrix:
    include:
        - env xTHREADING=1 xMPI=1 xGSL=1 xLTDL=1 xREADLINE=1 xPYTHON=1 xMUSIC=1 xSTATIC_ANALYSIS=1 CACHE_NAME=JOB
          python 3.4.4

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 5, 2018
Author Contributor

In fact, I spent a lot of time on the matrix include and exclude combinations. The above one simply doesn't work for some "strange" unknown reason. I also had a chat with travis team few months back and it seems their setup for include is a bit tricky.

.travis.yml Outdated
# Change directory back to the NEST source code directory.
- cd $SOURCEDIR
# terminaltables is required by parse_travis_log.py to create the build summary.
- pip install cython scipy matplotlib terminaltables mpi4py

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017
Member

If scipy is install from pip, I'm not sure I understand why we are using the deprecated version, nor why it is still present on line 77

build.sh Outdated
tar xf clang+llvm-3.6.2-x86_64-linux-gnu-ubuntu-14.04.tar.xz
# Copy and not move because '.cache' may aleady contain other subdirectories and files.
cp -R clang+llvm-3.6.2-x86_64-linux-gnu-ubuntu-14.04/* $HOME/.cache
echo "MSGBLD0060: CLANG-FORMAT installation completed."

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017
Member

I don't understand the answer... is the formatting with CLANG 3.5 the same as that of 3.6? If it is not the case, then a lot of tests will fail

make
make install
cd ..
rm -rf music

This comment has been minimized.

@jougs

jougs Aug 3, 2017
Contributor

Please add a newline at the end of the file.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 5, 2018
Author Contributor

done!

build.sh Outdated
tar xf clang+llvm-3.6.2-x86_64-linux-gnu-ubuntu-14.04.tar.xz
# Copy and not move because '.cache' may aleady contain other subdirectories and files.
cp -R clang+llvm-3.6.2-x86_64-linux-gnu-ubuntu-14.04/* $HOME/.cache
echo "MSGBLD0060: CLANG-FORMAT installation completed."

This comment has been minimized.

@jougs

jougs Aug 3, 2017
Contributor

I agree with @Silmathoron. Our C++ Coding Guidelines explicitly point out that we require clang-format 3.6.

Can you please double-check if all checks run properly using this old version? One way to check would probably be to manually check all our source code files in a TravisCI debug build.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Aug 9, 2017

See also #619

@jougs
Copy link
Contributor

@jougs jougs commented Sep 20, 2017

@lekshmideepu, @sdiazpier: Would it be possible / make sense to factor out the part fixing the MPI problems in test_disconnect.py and create another pull request from them? They are more or less independent of the changes to the CI setup, aren't they? Thanks!

@sdiazpier
Copy link
Contributor

@sdiazpier sdiazpier commented Sep 20, 2017

@lekshmideepu and @jougs I just started a new pull request with the changes that @lekshmideepu implemented in this PR to solve the issue with the testing of synapse deletion using mpi and pynest. @lekshmideepu could you please remove the modification in this PR?

@lekshmideepu
Copy link
Contributor Author

@lekshmideepu lekshmideepu commented Apr 9, 2018

@Silmathoron : Thanks. Probably I would try the matrix include in another PR once this is merged.

@lekshmideepu
Copy link
Contributor Author

@lekshmideepu lekshmideepu commented Apr 10, 2018

@jougs : Could you please take a look again?

Copy link
Contributor

@jougs jougs left a comment

Many thanks for taking care of all concerns and sorry for the long delay to re-review.

I have added some specific minor comments in the code. While I like the splitting of the installation of third-party software into separate scripts very much, I still have one small request concerning them: Can you please rename the *_install.sh scripts to install_*.sh and move them to the extras directory? I think that they might cause confusion in the root directory and the rename makes sure they appear in alphabetical order.

.travis.yml Outdated
# terminaltables is required by parse_travis_log.py to create the build summary.
- pip install cython scipy matplotlib terminaltables mpi4py
# Install boost libraries required for VERA++ static code analysis.
- sudo apt-get install -y libboost-filesystem-dev libboost-program-options-dev libboost-regex-dev libboost-wave-dev libboost-python-dev

This comment has been minimized.

@jougs

jougs Apr 16, 2018
Contributor

What is the reason for installing these Python libraries via pip and the Ubuntu packages manually using apt-get instead of adding all of them to the addons block above? Please either move them up and/or comment here on the reason.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 16, 2018
Author Contributor

Moved the boost library packages to the list of travis add ons source list which would save the bootstrapping time when compared to the installation from the ubuntu repo using apt-get. As few packages have an older version under the add ons list, installing those via pip

.travis.yml Outdated
@@ -172,4 +152,4 @@ deploy:
branch: master
local-dir: "$TRAVIS_BUILD_DIR/build/artefacts_upload"
upload-dir: "$TRAVIS_REPO_SLUG/$TRAVIS_BUILD_NUMBER/$TRAVIS_JOB_NUMBER"
acl: bucket_owner_full_control
acl: bucket_owner_full_control

This comment has been minimized.

@jougs

jougs Apr 16, 2018
Contributor

Please add a newline to the end of the file.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 16, 2018
Author Contributor

Done!

build.sh Outdated
export PYTHONPATH=$HOME/.cache/csa.install/lib/python2.7/site-packages:$PYTHONPATH
export LD_LIBRARY_PATH=$HOME/.cache/csa.install/lib:$LD_LIBRARY_PATH
elif [ "$TRAVIS_PYTHON_VERSION" == "3.4.4" ]; then
ls /usr/lib/x86_64-linux-gnu/

This comment has been minimized.

@jougs

jougs Apr 16, 2018
Contributor

I assume this line was for debugging. Please remove it.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 16, 2018
Author Contributor

Done!

build.sh Outdated
elif [ "$TRAVIS_PYTHON_VERSION" == "3.4.4" ]; then
ls /usr/lib/x86_64-linux-gnu/
export PYTHONPATH=/usr/lib/x86_64-linux-gnu/:$PYTHONPATH
#export PYTHONPATH=$HOME/.cache/csa.install/lib/python3.4/site-packages:$PYTHONPATH

This comment has been minimized.

@jougs

jougs Apr 16, 2018
Contributor

I assume this line was for debugging. Please remove it.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 16, 2018
Author Contributor

Done!

@@ -21,6 +21,9 @@

import nest
import unittest
import numpy as np

__author__ = 'naveau'

This comment has been minimized.

@jougs

jougs Apr 16, 2018
Contributor

Isn't this line duplicated now? Please l.28 see below.

This comment has been minimized.

@lekshmideepu

lekshmideepu Apr 16, 2018
Author Contributor

Done!

@lekshmideepu
Copy link
Contributor Author

@lekshmideepu lekshmideepu commented Apr 16, 2018

@jougs :Thanks for the comments. Hope I have answered all of them.

Copy link
Contributor

@jougs jougs left a comment

Many thanks for the fixes. I've sent you a small pull request with some additions and the deletion of the old installation scripts. I'll approve and merge this one once you have added the changes from my PR.

Remove old versions of scripts and small cleanup
@jougs
jougs approved these changes Apr 17, 2018
Copy link
Contributor

@jougs jougs left a comment

Very nice! I really appreciate these changes very much and am sure they will help us quite a bit in the future.

@lekshmideepu
Copy link
Contributor Author

@lekshmideepu lekshmideepu commented Apr 18, 2018

@jougs : Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.