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

new MESSAGE package for client/server/coupling #1013

Merged
merged 33 commits into from Sep 4, 2018

Conversation

Projects
None yet
4 participants
@sjplimp
Copy link
Contributor

sjplimp commented Jul 23, 2018

Purpose

New MESSAGE package to allow LAMMPS to be used as either a client or server in client/server mode and be coupled to another code. Examples for doing MD and MC (Monte Carlo) in this fashion,
as well as wrapping VASP.

Author(s)

Steve

Backward Compatibility

Should be N/A.

Implementation Notes

Provide any relevant details about how the changes are implemented, how correctness was verified, how other features - if any - in LAMMPS are affected

Post Submission Checklist

Please check the fields below as they are completed

  • The feature or features in this pull request is complete
  • Suitable new documentation files and/or updates to the existing docs are included
  • One or more example input decks are included
  • The source code follows the LAMMPS formatting guidelines

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Jul 23, 2018

@rbberger here is the PR I mentioned - needs some CMake work on the lib/message/cslib build and link

@@ -678,8 +681,6 @@ USER-INTEL, k = KOKKOS, o = USER-OMP, t = OPT.
"vector"_fix_vector.html,
"viscosity"_fix_viscosity.html,
"viscous"_fix_viscous.html,
"wall/body/polygon"_fix_wall_body_polygon.html,

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

@sjplimp this is undoing changes from the body package merge

@@ -932,9 +933,7 @@ KOKKOS, o = USER-OMP, t = OPT.
"airebo (oi)"_pair_airebo.html,
"airebo/morse (oi)"_pair_airebo.html,
"beck (go)"_pair_beck.html,
"body/nparticle"_pair_body_nparticle.html,

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

@sjplimp more reverted changes and a resulting broken link.

@@ -283,8 +288,6 @@ fix_vector.html
fix_viscosity.html
fix_viscous.html
fix_wall.html
fix_wall_body_polygon.html

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

@sjplimp some more reverted changes

@@ -426,9 +429,8 @@ pair_agni.html
pair_airebo.html
pair_awpmd.html
pair_beck.html
pair_body_nparticle.html

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

@sjplimp and another few reverted lines

// mode = file, zmq
// modearg = filename for file, localhost:5555 for zmq

#include <math.h>

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

@sjplimp for consistency, these include statements should use c++ style headers like <cmath> and alike

@@ -0,0 +1,234 @@
#!/usr/bin/env python

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

@sjplimp this python file needs to be adapted to become python3 compatible

@@ -140,7 +144,8 @@ fix_bond_break.html
fix_bond_create.html
fix_bond_react.html
fix_bond_swap.html
fix_box_relax.html
fix_box_relax.htmlf

This comment has been minimized.

@akohlmey

akohlmey Jul 24, 2018

Member

typo alert. fix_box_relax.html not fix_box_relax.htmlf

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 24, 2018

@sjplimp giving up on flagging each instance where consistency changes (include statements, python3 compatibility) need to be made.

@akohlmey akohlmey added this to the Stable Release Summer 2018 milestone Jul 24, 2018

@sjplimp sjplimp requested review from junghans and rbberger as code owners Jul 24, 2018

@rbberger
Copy link
Member

rbberger left a comment

@sjplimp I've added a preliminary cmake support but I didn't have much time to test it yet

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Jul 24, 2018

@sjplimp next time please just do a git pull to update your local branch when someone else has made modifications on it. I don't know what you did, but for some reason, you recreated my change. btw, I suggest that this last minute feature shouldn't be part of the "stable" release.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 24, 2018

@sjplimp i agree with @rbberger this touches too many core files and there are a bunch of questions and remarks i have about the implementation and how the link to LAMMPS is facilitated. This is better done after the stable release. I've thus changed the milestone.

Steven J. Plimpton added some commits Jul 24, 2018

@@ -426,6 +426,32 @@ if(PKG_KIM)
include_directories(${KIM_INCLUDE_DIRS})
endif()

if(PKG_MESSAGE)
option(MESSAGE_ZMQ "Use ZeroMQ in MESSAGE package" OFF)
file(GLOB_RECURSE cslib_SOURCES ${LAMMPS_LIB_SOURCE_DIR}/message/cslib/*.F

This comment has been minimized.

@junghans

junghans Jul 25, 2018

Member

Could we move that glob part in the SIMPLELIB loop?

This comment has been minimized.

@rbberger

rbberger Jul 25, 2018

Member

@junghans have a look at the lib and the source folder, the names don't match. that's why I did it this way.

This comment has been minimized.

@junghans

junghans Jul 25, 2018

Member

Good point!

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Jul 30, 2018

@akohlmey I'd like to include this in the stable if possible - it doesn't really overlap with any existing LAMMPS functionality - I can't see how it would break anything. What changes to core files
are you referring to? Re: Python 2 vs 3, this is not adding any Python scripts to LAMMPS itself,
just to an example (in COUPLE). The Python wrapper in the CSlib itself is not needed by LAMMPS, think of the CSlib as just a provided 3rd party lib (by me).

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 31, 2018

@sjplimp what worries me the most is the modification of the command line parsing. This is very complex as it is, so any significant changes would make me uneasy even if this wasn't close to a release.

Apart from the general concern, I don't understand why this is needed. If you launch everything with a single mpirun command, i would expect, that you can obtain the proper communicators via MPI_COMM_SPLIT(), where each executable uses a different color. Doing this via the command line feels hack-ish and is very error prone. Perhaps I am missing something (haven't had time to look at the implementation of the communication itself). Perhaps, this is best discussed over the phone (note, that i am currently 8 hours ahead of you).

As for python3 support. If the library/python files are bundled with LAMMPS it is seen as part of LAMMPS (even if you would see it differently), and after we put in a lot of effort to make LAMMPS compatible with python3, i see it as an obligation to not deviate from that again and let python2-only code into the distribution.

From a superficial look over the changes, there are likely more items that i would like to comment on (mainly from the perspective of possible future maintenance issues), but without a more thorough look at it, i don't want to make any statements now, as i may be misreading things. My 6th sense of hacking is often right about things, but not always.

Steven J. Plimpton
@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 24, 2018

@rbberger Just pushed my recent code changes, I think it is also synced with current master,
I need to do some compile and run-time debugging on this, but "cmake ../cmake -DPKG_MESSAGE=yes" does not seem to build with the MESSAGE package? Possibly
the CMake additions for this have never been run until now ...

Steven J. Plimpton and others added some commits Aug 24, 2018

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Aug 28, 2018

@sjplimp added the missing cmake portions. it should work now

@rbberger rbberger removed their assignment Aug 28, 2018

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 28, 2018

@rbberger I think this is pretty close to ready now. I have a few minor things I still need to do.
But please look it over to see if you find other issues to address? Thanks

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 28, 2018

@rbberger One thing that may be confusing to users with CMake for this new CSlib in lib/message.
Which is that for the examples COUPLE/lammps_mc and lammps_vasp you need to have the CSlib
built in serial (for mc) and as a shared lib (for vasp). This is not for the LAMMPS build, but to build or run the 2nd provided app. So it's easy to go to lib/message/cslib/src and do that via make. However if the user built LAMMPS with CMake, I don't think either of those lib builds happen, nor do they even know where the libs are that CMake puts into their build dir. So not sure what is best to do or document?

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Aug 29, 2018

@sjplimp
(0) please pull the latest changes

(1) cslib is built with CMake, otherwise, there would be link errors. I've now renamed the lib so it conforms to what you were doing in your Makefile. After running cmake & make, if you look at the build directory the library that gets created is called libcsmpi.a or libcsnompi.a, depending on if it is build with -D BUILD_MPI=on (default is on). If you build with -D BUILD_LIB=on -D BUILD_SHARED_LIBS=on it will be libcsmpi.so or libcsnompi.so.

(2) -mpi might be confusing to users and implying to enable running with MPI support. It should be --mpi-color. Btw, lmp -h is missing the help output for that option.

(3) message is a really bad name for a command, that doesn't send a message

(4) Assuming LAMMPS is installed into CMAKE_INSTALL_PREFIX (or via DEB or RPM) and we add a few more files to that install, a code like your COUPLE MC example could then be built with CMake with something like this:

#CMakeLists.txt in examples/COUPLE/lammps_mc
option(ENABLE_ZMQ "Enable ZeroMQ support" off)

add_executable(mc)

if( ENABLE_ZMQ )
   find_package(ZMQ)
   include_directories(${ZMQ_INCLUDE_DIRS})
   target_link_libraries(mc ${ZMQ_LIBRARIES})
endif()

find_package(cslib)
include_directories(${cslib_INCLUDE_DIRS})
target_link_libraries(mc ${cslib_LIBRARIES})

target_sources(mc mc.cpp random_park.cpp)

and

mkdir build
cd build
cmake ..
make

This last point doesn't need to be solved now, I can add it later after the merge.

@rbberger rbberger removed their assignment Aug 30, 2018

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 31, 2018

@rbberger - comments on your points
(1) these CMake changes are fine for the LAMMPS build with the MESSAGE package. However the other complication is that to run with another app (as in the 2 COUPLE examples). the user also has
to build the CSlib (perhaps in a different way) to link with the other app. So in the 2 READMEs (mc and vasp), I just explained how to do that directly in the lib/message/cslib/src dir with make. I'm thinking that is orthogonal to CMake.
(2) I'll make this change (mpicolor)
(3) Well, there is no command or functionality to "send a message" in LAMMPS. It's a command to setup a mode of messaging (client/server in this case) with another code. Seems like "message" is a good short name for that?
(4) possibly related to what I said for (1). For the VASP case I don't think there is anything for CMake to do. But the user still has to build the CSlib as a shared lib to use with the python vasp_wrap.py, but they might have built the CSlib differently for the LAMMPS build.

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 31, 2018

@rbberger Previous pushes include my final changes. From my POV this is ready to go unless you identify other issues. thanks

@rbberger rbberger merged commit c9cf3fb into master Sep 4, 2018

5 checks passed

lammps/pull-requests/build-docs-pr merge run ended
Details
lammps/pull-requests/kokkos_omp merge run ended
Details
lammps/pull-requests/openmpi-pr merge run ended
Details
lammps/pull-requests/serial-pr merge run ended
Details
lammps/pull-requests/shlib-pr merge run ended
Details

@rbberger rbberger deleted the cslib branch Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment