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

ADIOS 2 I/O #2766

Merged
merged 42 commits into from
Dec 15, 2023
Merged

ADIOS 2 I/O #2766

merged 42 commits into from
Dec 15, 2023

Conversation

bendudson
Copy link
Contributor

@bendudson bendudson commented Oct 12, 2023

Work in progress to enable BOUT++ to write and read data using ADIOS-2 (https://adios2.readthedocs.io/en/v2.9.1/).

Rebased #2739 on the next branch.

BOUT++ can now be configured with -DBOUT_DOWNLOAD_ADIOS=ON to download, configure and build ADIOS.

Once configured, the output and restart file data format can be set with output:type and restart_files:type settings e.g.

./conduction output:type=adios

outputs a BOUT.dmp.bp ADIOS2 dataset.

pnorbert and others added 20 commits October 12, 2023 07:31
In general file I/O maps a logically rectangular subset of
the local (nx, ny, nz) array onto a part of a global array.
That global array may be sparse i.e. there might be holes in it.

The mapping does not cover the entire local array because we
insert communication guard cells around the outside.

To perform this mapping we use ADIOS Put() function to get
a span of a buffer, and then iterate over fields to copy data
into those buffers.
…). Add helper functions to avoid redefining variables in an adios2::IO object.
…for restart files when using ADIOS. Replaced the use of Span and manual copy for memory selections to write data without ghost cells.
The Options tree passed to this function contains the data to be
written to file, not necessarily the input settings.

The output file name should be set from the Options::root() input
options, not the input options tree.
Using autoconf this should be defined, but is always "no"
Haven't yet regenerated configure script
Generated new configure script, updated autoconf_build_defines.hxx.in
Needs to be added to the makefile to be built.
Add --has-adios option to bout-config, and require in
test-options-adios integrated test.
@bendudson bendudson changed the base branch from master to next October 12, 2023 14:41
@bendudson bendudson mentioned this pull request Oct 12, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 62. Check the log or trigger a new build to see more.

/// (MapGlobalX, MapGlobalY, MapGlobalZ) in the global index space maps to (MapLocalX, MapLocalY, MapLocalZ) locally.
/// Note that boundary cells are included in the global index space, but communication
/// guard cells are not.
int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'MapGlobalX' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
      ^

/// (MapGlobalX, MapGlobalY, MapGlobalZ) in the global index space maps to (MapLocalX, MapLocalY, MapLocalZ) locally.
/// Note that boundary cells are included in the global index space, but communication
/// guard cells are not.
int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'MapGlobalY' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
                  ^

/// (MapGlobalX, MapGlobalY, MapGlobalZ) in the global index space maps to (MapLocalX, MapLocalY, MapLocalZ) locally.
/// Note that boundary cells are included in the global index space, but communication
/// guard cells are not.
int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'MapGlobalZ' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
                              ^

/// Note that boundary cells are included in the global index space, but communication
/// guard cells are not.
int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
int MapLocalX, MapLocalY, MapLocalZ; ///< Start local indices
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'MapLocalX' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int MapLocalX, MapLocalY, MapLocalZ;    ///< Start local indices
      ^

/// Note that boundary cells are included in the global index space, but communication
/// guard cells are not.
int MapGlobalX, MapGlobalY, MapGlobalZ; ///< Start global indices
int MapLocalX, MapLocalY, MapLocalZ; ///< Start local indices
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'MapLocalY' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int MapLocalX, MapLocalY, MapLocalZ;    ///< Start local indices
                 ^

include/bout/options_io.hxx Outdated Show resolved Hide resolved
include/bout/options_io.hxx Show resolved Hide resolved
include/bout/options_io.hxx Outdated Show resolved Hide resolved
include/bout/options_io.hxx Outdated Show resolved Hide resolved
include/bout/options_io.hxx Outdated Show resolved Hide resolved
@ZedThree
Copy link
Member

I think the clang-tidy comments are mostly sensible, except to use [[maybe_unused]] instead of commenting out the parameter name for unused arguments.

Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

I think my only two major comments are:

  1. if OptionsIO is the new interface, then we could/should make options_adios/netcdf.hxx private
  2. it would be good to switch to using the generic Factory to handle the creation of the specific type

include/bout/options_io.hxx Outdated Show resolved Hide resolved
src/sys/options/options_adios.cxx Outdated Show resolved Hide resolved
template <class T>
bool readVariable(adios2::Engine& reader, adios2::IO& io, const std::string& name,
const std::string& type, Options& result) {
std::vector<T> data;
Copy link
Member

Choose a reason for hiding this comment

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

This looks to be unused?

}

if (variable.ShapeID() == adios2::ShapeID::LocalArray) {
throw std::invalid_argument(
Copy link
Member

Choose a reason for hiding this comment

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

Worth switching to BoutException so we can use fmt to format the message?

pnorbert and others added 4 commits December 5, 2023 11:33
* next:
  Set oversubscribe flags for openmpi 5
  CI: Replace pip script with requirements.txt
  Add BOUT_HOST_DEVICE to accessor operators for GPU
  hypre3d: Replace finite() with std::isfinite()
  hypre3d: Remove reference to datafile
  Use latest python in CI
  Bump externalpackages/boutdata from `ab59ef9` to `908a4c2`
  Bump externalpackages/googletest from `2dd1c13` to `829c199`
  Bump externalpackages/fmt from `f0903ad` to `2ac6c5c`
  Bump externalpackages/mpark.variant from `3c7fc82` to `23cb94f`
  CI: run only once per week
  Increase shm size in container
  CI: Also build slepc from devel branch
  CI: Run on PETSc developement branch
  Fix test-laplace-petsc3d
  CI: user master branch for OCI workflow
  [RTD] specify tool
  [RTD] add os too read the docs config
  Bump ZedThree/clang-tidy-review from 0.13.1 to 0.14.0
  Bump stefanzweifel/git-auto-commit-action from 4 to 5
  Bump actions/setup-python from 1 to 4
  Increase MG levels for Petsc3DAMG test
  Don't include headers for petsc interface if we're not building it
  Apply clang-format changes
  Fix a whole bunch of clang-tidy issues in petsc interface/AMG solver
  Apply clang-format
  Add defaulted move ctor/assignment for `PetscMatrix::Element`
  Fix getting correct parallel slice for `getWeightsForYApproximation`
  Stop clang-tidy complaining about some short names
  Change some variable names to keep clang-tidy happy
  Apply clang-format changes
  Delete unneeded PetscVectorElement test
  Refactor setting boundary conditions in LaplacePetsc3dAmg
  Set LaplacePetsc3damg options in initialiser list
  Print error in laplace-petsc3d test
  Add a utility function for checking flags in bitsets
  Make sure guard cells in PetscVector buffer are -1
  Fix some clang-tidy issues, mostly multiple declarations on one line
  Replace direct calls to `VecSetValue` with buffered calls
  Make `Field2D::data` private
  Collect all test failures for petsc3d
  Add `size()` method to Fields to get total number of points
  Simplify getting parallel slice interpolation weights
  Use `BoutException` formatted message instead of `std::cout`
  Use default constructor for `Vec/Mat` deleters
  WIP convert petscvector
  Fix lots of clang-tidy issues in PETSc interface
  Docs: Note removal of `./configure` build system
  Remove all autoconf files
  Docs: Remove remaining references to `./configure` from docs
  Fix sphinx configuration
  Bump docker/build-push-action
  Ensure also lower case is accepted
  Bump externalpackages/googletest from `6092810` to `2dd1c13`
  Apply clang-format changes
  Cast reason to int for formatting
  Do not try to print KSPConvergedReason
  CI: Use more system packages
  CI: Reenable petsc
  Try disabeling PETSc
  Bump actions/cache from 2 to 3
  Bump docker/metadata-action
  Bump actions/checkout from 2 to 4
  Bump codecov/codecov-action from 1 to 3
  Bump externalpackages/boututils from `c2e97a2` to `433995f`
  Also give a backtrace if we are hitting a bus error
  Switch back to openmpi
  CI: switch to mpich to workaround openmpi issue
  Switch to fedora packes
  Default to current working directory
  Ensure submodules are present for local testing
  Change default to rawhide
  Prefer dnf5
  Bump required C++ standard to 17
Mainly unused parameters, moving definitions to the header,
adding override keywords.

Note that there is a GCC compiler bug with [[maybe_unused]] on
first argument of a constructor: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81429
bendudson and others added 17 commits December 5, 2023 16:46
Automatic formatting
Sorry for the large commit; this touches quite a few places.

The OptionsNetCDF and OptionsADIOS classes are now private,
so that OptionsIO is now the interface to both.

The generic Factory is used to implement OptionsIOFactory,
and the "netcdf" and "adios" types are registered in their
separate header files.

The OptionsIOFactory handles the choice of default section
("restart_files" for restarts, "output" for dump/output files),
and sets the default path and prefix. This simplifies both
the PhysicsModel and the separate implementations by putting
that logic in one place.
Now tested with ADIOS enabled.
If configured with `-DBOUT_DOWNLOAD_ADIOS=ON`, CMake will
download ADIOS2 from github, configure and build it with BOUT++.
Some documentation in the comments, and in the manual.
The unit tests require GMock, so set BUILD_GMOCK=ON by default.
Now always need to pass the data to be written,
rather than implicitly writing Options::root().
Changing from OptionsNetCDF to OptionsIO interface.
Output file needs MZ and other mesh variables for collect()
to work.
Needed in post-processing when reading data
collect needs to know BOUT_VERSION or it assumes pre-0.2 format.
ADIOS uses '/' as a separator, but BOUT++ options uses ':'.
When reading variables, we now split on '/' separator and navigate
to the correct subsection. Attributes are also named using the final
part of the name after the last '/'.

solver and beuler integrated tests weren't calling BoutInitialise,
so failed when ADIOSFinalize is called.
BoutFinalise throws an exception when ADIOS is enabled but
BoutInitialise is not called.

BoutInitialise throws an exception if the "data" directory
doesn't exist.

For this test neither BoutInitialise nor BoutFinalise are needed,
so removed.
Not required, and BoutInitialise throws an exception because
the "data" directory doesn't exist.
Using $HOME/local rather than $HOME/local/adios2. Hopefully cmake
can find it.
@bendudson
Copy link
Contributor Author

After an extended game of whack-a-mole, I think this is now ready for merging. I don't know what's going on with clang-tidy-review, but I don't think it's related to the changes.
More documentation and tests would be nice, but for trying out in next I'm calling this good.

@bendudson bendudson merged commit 94493f9 into next Dec 15, 2023
24 of 25 checks passed
@bendudson bendudson deleted the adios-io-next-rebase branch December 15, 2023 22: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.

3 participants