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

Migration to Python 3 #311

Merged
merged 50 commits into from
Sep 21, 2020
Merged

Migration to Python 3 #311

merged 50 commits into from
Sep 21, 2020

Conversation

jkrajniak
Copy link
Member

@jkrajniak jkrajniak commented Aug 28, 2020

I have migrated the code base to work with Python 3. I have tested with Python 3.8; Locally all tests passed without issue. I have also run simulation from examples/lennard_jones in both python 2 and 3. The results are identical. Nevertheless, it would be good if someone could do separate tests.
TODO:

  • adjust travis to build against python3, perhaps run tests for both python3.7 and python3.8
  • decide what to do with contrib/boost (I have removed it in this PR)

I think the best idea would be to move the current master to branch py2 and make master python3 only compatible.

Fix #227

@jkrajniak
Copy link
Member Author

@jkrajniak @junghans It seems my commit didn't trigger a build on Travis-C

vectorization test passed but still there is some issue with

  Could NOT find MPI (missing: MPI_CXX_FOUND CXX)
      Reason given by package: MPI component 'C' was requested, but language C is not enabled.  MPI component 'Fortran' was requested, but language Fortran is not enabled.

:(

@jkrajniak
Copy link
Member Author

In the error message from CMake

root@60ead7879b16 build]# /home/espressopp/espressopp/build/CMakeFiles/CMakeError.log
bash: /home/espressopp/espressopp/build/CMakeFiles/CMakeError.log: Permission denied
[root@60ead7879b16 build]# cat /home/espressopp/espressopp/build/CMakeFiles/CMakeError.log
The MPI test test_mpi for CXX in mode normal failed to compile with the following output:
Change Dir: /home/espressopp/espressopp/build/CMakeFiles/CMakeTmp

Run Build Command(s):/usr/bin/gmake cmTC_0b2c6/fast && /usr/bin/gmake  -f CMakeFiles/cmTC_0b2c6.dir/build.make CMakeFiles/cmTC_0b2c6.dir/build
gmake[1]: Entering directory '/home/espressopp/espressopp/build/CMakeFiles/CMakeTmp'
Building CXX object CMakeFiles/cmTC_0b2c6.dir/test_mpi.cpp.o
/usr/lib64/ccache/clang++  -D_GLIBCXX_ASSERTIONS -isystem /usr/include/mpich-x86_64  -fexceptions -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -std=c++11 -o CMakeFiles/cmTC_0b2c6.dir/test_mpi.cpp.o -c /home/espressopp/espressopp/build/CMakeFiles/FindMPI/test_mpi.cpp
clang-10: error: unknown argument: '-fstack-clash-protection'
gmake[1]: *** [CMakeFiles/cmTC_0b2c6.dir/build.make:86: CMakeFiles/cmTC_0b2c6.dir/test_mpi.cpp.o] Error 1
gmake[1]: Leaving directory '/home/espressopp/espressopp/build/CMakeFiles/CMakeTmp'
gmake: *** [Makefile:141: cmTC_0b2c6/fast] Error 2

@junghans
Copy link
Member

junghans commented Sep 1, 2020

I see, the mpich wrapper contains -fstack-clash-protection, which clang doesn't support. I fixed it on Fedora recently (see https://src.fedoraproject.org/rpms/mpich/c/c40fc556e10d5fe17e44963138543891f2e56354?branch=master), so we need to report this Ubuntu as well. I would exclude the build for now.

@jkrajniak
Copy link
Member Author

Ubuntu seems to be fine. The issue is with Fedora MPICH

@junghans
Copy link
Member

junghans commented Sep 1, 2020

Oh, the fedora docker build was disabled, see espressopp/buildenv#27

@govarguz
Copy link
Member

govarguz commented Sep 1, 2020

@jkrajniak Py3 is indeed required! I will make some tests in the weekend

@govarguz did you have a chance to do some tests?

Hey there, I was trying to compyle the branch with intel compilers and python 3.7 and i got one strange boost error I could not get rid of
Could not find a package configuration file provided by "boost_python"
(requested version 1.73.0) with any of the following names:

boost_pythonConfig.cmake
boost_python-config.cmake

Add the installation prefix of "boost_python" to CMAKE_PREFIX_PATH or set
"boost_python_DIR" to a directory containing one of the above files. If
"boost_python" provides a separate development package or SDK, be sure it
has been installed.

any clues?

@jkrajniak
Copy link
Member Author

@jkrajniak Py3 is indeed required! I will make some tests in the weekend

@govarguz did you have a chance to do some tests?

Hey there, I was trying to compyle the branch with intel compilers and python 3.7 and i got one strange boost error I could not get rid of
Could not find a package configuration file provided by "boost_python"
(requested version 1.73.0) with any of the following names:

boost_pythonConfig.cmake
boost_python-config.cmake

Add the installation prefix of "boost_python" to CMAKE_PREFIX_PATH or set
"boost_python_DIR" to a directory containing one of the above files. If
"boost_python" provides a separate development package or SDK, be sure it
has been installed.

any clues?

I think this is because I hardcoded in required boost components python38; I have made a small fix to CMakeList.txt to select properly boost component based on the founded version of Python (hopefully this works).

@jkrajniak
Copy link
Member Author

Oh, the fedora docker build was disabled, see espressopp/buildenv#27

It's still failing, even with rebuilt fedora image. I have temporally switched off build of fedora_mpich with clang

@jkrajniak
Copy link
Member Author

With codecov step the issue is with the log size on travis. The log contains enormouse number of

File '/usr/include/boost/smart_ptr/detail/sp_counted_impl.hpp'
Lines executed:0.00% of 13
No branches
No calls
Creating '#usr#include#boost#smart_ptr#detail#sp_counted_impl.hpp.gcov'

lines. This results in the error:
The job exceeded the maximum log length, and has been terminated.

I do not get why codecov analysis, e.g. /usr/include/boost/function/function_template.hpp

@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@84c2543). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #311   +/-   ##
========================================
  Coverage          ?    0.0%           
========================================
  Files             ?     424           
  Lines             ?   27724           
  Branches          ?    3187           
========================================
  Hits              ?       0           
  Misses            ?   27724           
  Partials          ?       0           
Impacted Files Coverage Δ
.../interaction/SingleParticleInteractionTemplate.hpp 0.0% <0.0%> (ø)
src/integrator/DPDThermostat.cpp 0.0% <0.0%> (ø)
src/interaction/LennardJonesGromacs.hpp 0.0% <0.0%> (ø)
src/esutil/Array2D.hpp 0.0% <0.0%> (ø)
src/analysis/NeighborFluctuation.hpp 0.0% <0.0%> (ø)
src/analysis/VelocityAutocorrelation.cpp 0.0% <0.0%> (ø)
src/integrator/CapForce.cpp 0.0% <0.0%> (ø)
src/interaction/LennardJonesEnergyCapped.cpp 0.0% <0.0%> (ø)
src/integrator/EmptyExtension.hpp 0.0% <0.0%> (ø)
src/iterator/CellListIterator.hpp 0.0% <0.0%> (ø)
... and 414 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84c2543...e098097. Read the comment docs.

@jkrajniak
Copy link
Member Author

Any progress with this PR?

@junghans
Copy link
Member

From my side, go ahead!

Copy link
Member

@jnvance jnvance left a comment

Choose a reason for hiding this comment

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

I'm testing on Intel Compiler 19.0.5.281 and Boost 1.74, and everything seems to be fine except for the following test with -O2 and -O3 optimization. This could be just the result of more aggressive optimization from the Intel compiler as it doesn't show up in -O1 and w/o any optimization flag. If we don't need to address this now, then the PR is already fine from my side.

21/66 Test #21: MTSAdResS ..........................***Failed    1.27 sec
..F.
======================================================================
FAIL: test_AT_CG_templates_FAdResS (__main__.TestMTSAdResS)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vance/espressopp/py3/espressopp/testsuite/AdResS/MTSAdResS/test_MTSAdResS.py", line 242, in test_AT_CG_templates_FAdResS
    self.assertAlmostEqual(after[0], 9.364194, places=5)
AssertionError: 9.36420869641006 != 9.364194 within 5 places (1.4696410060466292e-05 difference)

----------------------------------------------------------------------
Ran 4 tests in 0.056s

FAILED (failures=1)
Cubicity check passed -> powered by HeSpaDDA
HeSpaDDA message: Size Lenghts are eq. while ordering axis with preference on X, Y and Z!
Cubicity check passed -> powered by HeSpaDDA
HeSpaDDA message: Size Lenghts are eq. while ordering axis with preference on X, Y and Z!
Cubicity check passed -> powered by HeSpaDDA
HeSpaDDA message: Size Lenghts are eq. while ordering axis with preference on X, Y and Z!
Cubicity check passed -> powered by HeSpaDDA
HeSpaDDA message: Size Lenghts are eq. while ordering axis with preference on X, Y and Z!

Copy link
Member

@govarguz govarguz left a comment

Choose a reason for hiding this comment

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

I have tested with 2 versions of python3 and it seems to work fine. The only complementary PR, I see that might be required is a bit more of documenting that we reached Py3. As an idea I would suggest to add some compiling/installing requirements for a couple of Dist. perhaps the ones we have in the CI. However, this can be a PR coming up in the next days. @jkrajniak go ahead!

@jkrajniak jkrajniak merged commit 86326a6 into espressopp:master Sep 21, 2020
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.

Move to Python3
4 participants