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

USER-ADIOS package with ADIOS 2.x #1317

Merged
merged 16 commits into from Feb 27, 2019

Conversation

@pnorbert
Copy link
Collaborator

commented Feb 6, 2019

Purpose

USER-ADIOS package for dump atom/adios and dump custom/adios commands

Author(s)

Norbert Podhorszki, pnorbert@ornl.gov

Backward Compatibility

no

Implementation Notes

This is for outputting data from LAMMPS using ADIOS. It has no effect on LAMMPS computation.
USER-ADIOS currently builds only with cmake.

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

https://github.com/ornladios/ADIOS2

@pnorbert pnorbert requested review from junghans and rbberger as code owners Feb 6, 2019

@pnorbert

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 6, 2019

Steve Plimpton suggested to assign this PR for review to @akohlmey

@akohlmey
Copy link
Member

left a comment

Thanks for your contribution. As indicated, there is work do to. I've added some requests for changes, to align the code better with current LAMMPS programming conventions (which is mostly 'C with Classes'). It also seems to be lacking support for the conventional build system, which is how the majority of LAMMPS users builds LAMMPS. Also an essential requirement for inclusion into LAMMPS is adding documentation to the new commands, and instructions for building LAMMPS with the new package enabled into the manual in doc/src

There are possibly some more details that would be desirable to have them changed/improved to integrate this code as smoothly and as portably as possible into LAMMPS, but this is the most obvious that stood out when i glanced across the pull request.

Contributing author: Norbert Podhorszki (ORNL)
------------------------------------------------------------------------- */

#include <string.h>

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please use c++ style includes, i.e. #include <cstring> instead of #include <string.h>

char *ptr = strchr(filestar,'*');
*ptr = '\0';
if (padflag == 0)
sprintf(filecurrent,"%s" BIGINT_FORMAT "%s",

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please use snprintf() instead of sprintf() to avoid crashes due to memory overflows

}
free(filestar);
delete [] filecurrent;
}

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please use } else { and if () { and do not put the curly brace on the next line for consistency in programming style with the core of LAMMPS.

This comment has been minimized.

Copy link
@pnorbert

pnorbert Feb 7, 2019

Author Collaborator

Is there a specific formatting style that I can use in Eclipse to format LAMMPS source? Currently I format my codes with clang 3.8 with specifications for the ADIOS2 project (e.g. curly brace in new lines)

#define LMP_DUMP_ATOM_ADIOS_H

#include "dump_atom.h"
#include <stdlib.h>

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please do not include headers except the ones required for parent classes into "style" headers. also, please use the c++ versions of c-library headers.


protected:

const std::string ioName="atom"; // name of adios group, referrable in adios2_config.xml

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please do not use std::string() here but const char * and initialize in constructor.

protected:

const std::string ioName="atom"; // name of adios group, referrable in adios2_config.xml
adios2::ADIOS *ad = nullptr; // adios object

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

for "style" class headers, please hide all implementation specific data elements into a wrapper struct or class, and only store a pointer to that kind of struct/class, so that we don't need to include any of those headers here and can instead use a forward declaration and pass this as an opaque pointer, if needed. All "style" headers need to be included at the same time, and thus we need to reduce any potential name space collisions or inconsistencies due to the order in which non-LAMMPS headers are included (it has happened in the past repeatedly).

adios2::Variable<double> varAtoms; // one ADIOS output variable we need to change
uint64_t groupSize; // pre-calculate # of bytes written per processor in a step before writing anything
uint64_t groupTotalSize; // ADIOS buffer size returned by adios_group_size(), valid only if size is > default 16MB ADIOS buffer
std::string filecurrent; // name of file for this round (with % and * replaced)

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please use "char *" in headers, not std::string

Contributing author: Paul Coffman (IBM)
------------------------------------------------------------------------- */

#include <math.h>

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please use c++ style includes: <cmath>, <cstdlib>, and <cstring>


DumpCustomADIOS::~DumpCustomADIOS()
{
columnNames.clear();

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

shouldn't there be a delete ad here?

adios2::Engine fh; // adios file/stream handle object
adios2::Variable<double> varAtoms; // one ADIOS output variable we need to change
uint64_t groupsize; // pre-calculate # of bytes written per processor in a step before writing anything
std::vector<std::string>columnNames; // list of column names for the atom table (individual list of 'columns' string)

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 7, 2019

Member

please don't use STL container classes in "style" headers / style classes. See my previous comments about reducing issues from including all "style" headers at the same time in some files.

@akohlmey akohlmey added this to the Stable Release Spring 2019 milestone Feb 7, 2019

@akohlmey akohlmey self-assigned this Feb 7, 2019

@akohlmey akohlmey changed the title WIP: USER-ADIOS package with ADIOS 2.x USER-ADIOS package with ADIOS 2.x Feb 7, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

if(PKG_USER-ADIOS)
message(STATUS "Force newer standards because of using ADIOS")
cmake_minimum_required(VERSION 3.6)
# Force C++11 and C99

This comment has been minimized.

Copy link
@junghans

junghans Feb 9, 2019

Member

C99, seems not to be set!

@@ -2,7 +2,15 @@
# CMake build system
# This file is part of LAMMPS
# Created by Christoph Junghans and Richard Berger
cmake_minimum_required(VERSION 2.8.12)
if(PKG_USER-ADIOS)
message(STATUS "Force newer standards because of using ADIOS")

This comment has been minimized.

Copy link
@junghans

junghans Feb 9, 2019

Member

Let's drop this message the other packages, which need a higher cmake version, e.g.PKG_GPU don't do that either.

cmake_minimum_required(VERSION 2.8.12)
if(PKG_USER-ADIOS)
message(STATUS "Force newer standards because of using ADIOS")
cmake_minimum_required(VERSION 3.6)

This comment has been minimized.

Copy link
@junghans

junghans Feb 9, 2019

Member

Just move a

if (CMAKE_VERSION VERSION_LESS "3.6")
  message(FATAL_ERROR "For the USER-ADIOS package you need at least cmake-3.6")
endif()

in the if(USER-ADIOS) block below.

Also why 3.6 anything specific with that version?

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@pnorbert please drop us a note here, when you feel that your contribution is ready for a second look. thanks.

@pnorbert pnorbert force-pushed the ornladios:adios2-master branch from 0ed0cc2 to 75fe967 Feb 20, 2019

@@ -58,7 +58,7 @@ PACKAGE = asphere body class2 colloid compress coreshell dipole gpu \
molecule mpiio mscg opt peri poems \
python qeq replica rigid shock snap spin srd voronoi

PACKUSER = user-atc user-awpmd user-bocs user-cgdna user-cgsdk user-colvars \
PACKUSER = user-adios user-atc user-awpmd user-bocs user-cgdna user-cgsdk user-colvars \

This comment has been minimized.

Copy link
@akohlmey

akohlmey Feb 20, 2019

Member

FYI: user-adios also needs to be added to the variables PACKLIB and PACKEXT

@pnorbert

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 20, 2019

@akohlmey Ready for a review again. Can you please explain what is wrong in the new-build-docs-pr build? Thanks.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@akohlmey Ready for a review again. Can you please explain what is wrong in the new-build-docs-pr build? Thanks.

looks like false positives from the spellchecker. i can fix it.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@pnorbert i have one question about installation/configuration: do you want to include support for downloading and building a local version of ADIOS into LAMMPS (like, e.g. for Plumed, or LATTE or KIM), or do you prefer that ADIOS is installed first independently?

@pnorbert

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 20, 2019

I don't want to include it myself but I am not against that idea. ADIOS should build easily without external dependencies on linux/mac/windows PCs. However, for supercomputers, I would prefer relying on an external build (hopefully a module provided by the admins or the ECP (Exascale Computing Program))

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

I don't want to include it myself but I am not against that idea. ADIOS should build easily without external dependencies on linux/mac/windows PCs. However, for supercomputers, I would prefer relying on an external build (hopefully a module provided by the admins or the ECP (Exascale Computing Program))

it is not a case of either / or. both are possible. but for now let us stick with the external version only.

akohlmey added some commits Feb 27, 2019

@akohlmey akohlmey requested review from junghans and akohlmey Feb 27, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@pnorbert FYI, to make installation work with CMake, I not only had to set ADIOS2_DIR, but also extend $PATH to contain $ADIOS2_DIR/bin. Otherwise cmake would fail to configure

@pnorbert

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 27, 2019

Which version of cmake needs that? My setup works without having adios in the PATH. I am using cmake 3.12.0 on my machine

export HDF5_ROOT=/opt/hdf5-parallel
cmake -D CMAKE_INSTALL_PREFIX=/opt/lammps \
      -D CMAKE_BUILD_TYPE=Debug \
      -D BUILD_MPI=yes \
      -D LAMMPS_MACHINE=adiosvm \
      -D BUILD_EXE=yes \
      -D BUILD_LIB=no \
      -D BUILD_SHARED_LIBS=no \
      -D BUILD_DOC=yes \
      -D LAMMPS_SIZES=smallbig \
      -D PKG_USER-H5MD=yes \
      -D PKG_USER-ADIOS=yes -DADIOS2_DIR=/opt/adios2 \
      ../cmake

Can you share the log of the cmake failure?

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Which version of cmake needs that? My setup works without having adios in the PATH. I am using cmake 3.12.0 on my machine

export HDF5_ROOT=/opt/hdf5-parallel
cmake -D CMAKE_INSTALL_PREFIX=/opt/lammps \
      -D CMAKE_BUILD_TYPE=Debug \
      -D BUILD_MPI=yes \
      -D LAMMPS_MACHINE=adiosvm \
      -D BUILD_EXE=yes \
      -D BUILD_LIB=no \
      -D BUILD_SHARED_LIBS=no \
      -D BUILD_DOC=yes \
      -D LAMMPS_SIZES=smallbig \
      -D PKG_USER-H5MD=yes \
      -D PKG_USER-ADIOS=yes -DADIOS2_DIR=/opt/adios2 \
      ../cmake

Can you share the log of the cmake failure?

I first downloaded and compiled/installed ADIOS2 as follows:

tar -xjvvf adios2-2.3.1.tar.bz2 
cd adios2-2.3.1/
mkdir build
cd build/
cmake .. -DCMAKE_INSTALL_PREFIX=/home/akohlmey/compile/lammps/lib/adios
make -j4
make install

and then switched to the LAMMPS folder and did:

mkdir build-adios
cd build-adios
cmake ../cmake -DPKG_USER-ADIOS=yes -DADIOS2_DIR=$HOME/compile/lammps/lib/adios
-- The CXX compiler identification is GNU 8.2.1
-- Check for working CXX compiler: /usr/lib64/ccache/c++
-- Check for working CXX compiler: /usr/lib64/ccache/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Running check for auto-generated files from make-based build system
-- The C compiler identification is GNU 8.2.1
-- Check for working C compiler: /usr/lib64/ccache/cc
-- Check for working C compiler: /usr/lib64/ccache/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
CMake Error at CMakeLists.txt:210 (find_package):
  By not providing "FindADIOS2.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "ADIOS2", but
  CMake did not find one.

  Could not find a package configuration file provided by "ADIOS2" with any
  of the following names:

    ADIOS2Config.cmake
    adios2-config.cmake

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


-- Configuring incomplete, errors occurred!
See also "/home/akohlmey/compile/lammps/build-adios/CMakeFiles/CMakeOutput.log".

It works with prefixing PATH=/home/akohlmey/compile/lammps/lib/adios/bin:$PATH and it also works, if I follow the instructions given in the error and use:

cmake ../cmake -DPKG_USER-ADIOS=yes -DADIOS2_DIR=$HOME/compile/lammps/lib/adios/lib64/cmake/adios2
@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

So I think it should not be an issue, if ADIOS is properly installed with an environment module or equivalent, but simply for the case of a person building a local version and then wanting to use/link that. So probably some minor adjustment to the docs will set things straight. I only reported it, because I tried to follow instructions in the LAMMPS manual.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Conventional build is working fine, but despite embedding -rpath, there is an unresolved shared library:

$ ldd lmp_mpi
	linux-vdso.so.1 (0x00007fff3cdec000)
	libadios2.so.2 => /home/akohlmey/compile/lammps/lib/adios/lib64/libadios2.so.2 (0x00007f66f4255000)
	libmpicxx.so.12 => /usr/lib64/mpich/lib/libmpicxx.so.12 (0x00007f66f4231000)
	libmpi.so.12 => /usr/lib64/mpich/lib/libmpi.so.12 (0x00007f66f3f94000)
	libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f66f3dd9000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f66f3c55000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f66f3c3a000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f66f3c16000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f66f3a50000)
	libzmq.so.5 => /lib64/libzmq.so.5 (0x00007f66f39d5000)
	libadios2_sst.so.2 => not found
	libbz2.so.1 => /lib64/libbz2.so.1 (0x00007f66f39c2000)
	libhdf5.so.10 => /usr/lib64/mpich/lib/libhdf5.so.10 (0x00007f66f36ea000)
	libsz.so.2 => /lib64/libsz.so.2 (0x00007f66f36e3000)
	libz.so.1 => /lib64/libz.so.1 (0x00007f66f36c9000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f66f36c3000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f66f4760000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f66f36b9000)
	libpgm-5.2.so.0 => /lib64/libpgm-5.2.so.0 (0x00007f66f3669000)
	libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007f66f3615000)
	libaec.so.0 => /lib64/libaec.so.0 (0x00007f66f360a000)
	libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007f66f3519000)
	libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007f66f34fb000)
	libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f66f34f4000)
	libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007f66f34e0000)
	libcrypto.so.1.1 => /lib64/libcrypto.so.1.1 (0x00007f66f3206000)
	libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007f66f3200000)
	libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f66f31e6000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f66f31b9000)
	libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f66f3133000)

Again, this is for the unusual case of building ADIOS only locally for building LAMMPS without having a global installation that sets LD_LIBRARY_PATH as needed. I suspect this is caused by make install of the ADIOS library, where CMake is stripping the embedded rpath form the shared libraries, that are linked to main ADIOS library, but not explicitly linked to the LAMMPS executable.

Changes to CMakeLists.txt are now very minimal and consistent with other Packages

@akohlmey akohlmey requested review from sjplimp and stanmoore1 Feb 27, 2019

need an approving review from a different LAMMPS developer for merging in accordance with our workflow policy.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@pnorbert I am ok to merge this after updating the docs to require a "proper" installation of ADIOS2. It is the reasonable thing to do. The way i compiled LAMMPS with the package enabled is more like people doing packaging would do it, and then using the temporary workarounds I mentioned are acceptable. Please let me know, if you prefer to make the documentation changes yourself, or want me to do them.

@pnorbert

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 27, 2019

Let me ask @chuckatkins at Kitware since this is supposed to work with recent versions of cmake. I required cmake 3.6 or newer in the CMakeLists.txt when someone wanted to turn-on USER-ADIOS but I was asked to remove it by the code review.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

src/USER-ADIOS/dump_custom_adios.cpp Outdated Show resolved Hide resolved

akohlmey added some commits Feb 27, 2019

@akohlmey akohlmey merged commit 326a976 into lammps:master Feb 27, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@pnorbert i made the doc tweaks myself and merged to branch as is. if you have additional updates, please create a new pull request from the updated master branch. we wanted to include USER-ADIOS in the next patch and small improvements can wait until the next patch.

@pnorbert

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 27, 2019

@akohlmey Thank you for taking care of this PR.
Next, we want to create a test script that we could use in the ADIOS CI infrastructure to build LAMMPS with ADIOS, then execute an example and test the result and put the success/failure on the ADIOS dashboard. This would ensure that we test LAMMPS with ADIOS every night and alert ourselves if we break ADIOS for LAMMPS. Can I store such a script in the src/USER-ADIOS/ directory, or somewhere else?
We don't want to nightly build the master branch of LAMMPS, but use a stable branch. Of course, I can create a branch in my fork for this purpose but I am wondering if we could pull from the official repository.
This is an ECP milestone for us due March 30...

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@akohlmey Thank you for taking care of this PR.
Next, we want to create a test script that we could use in the ADIOS CI infrastructure to build LAMMPS with ADIOS, then execute an example and test the result and put the success/failure on the ADIOS dashboard. This would ensure that we test LAMMPS with ADIOS every night and alert ourselves if we break ADIOS for LAMMPS. Can I store such a script in the src/USER-ADIOS/ directory, or somewhere else?

the preferred location for such a script (and any additional files) would be a folder tools/adios possibly with subfolders, if needed.

We don't want to nightly build the master branch of LAMMPS, but use a stable branch. Of course, I can create a branch in my fork for this purpose but I am wondering if we could pull from the official repository.
This is an ECP milestone for us due March 30...

i would recommended to track the unstable branch. that is updated with every patch release. you could also track stable, but that will be updated only every 3-4 months (last was 12 December 2018 and we are currently planning for the next stable release in late April 2019).

@akohlmey

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

oh, and i forgot to mention, we hope to have a new patch release tomorrow, if everything works out.

@pnorbert pnorbert deleted the ornladios:adios2-master branch Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.