Small quality of life improvements to basic examples.

[NAThompson]

The commit gives a more informative error message when the .bp file has not been written by the corresponding writer, and uses the std::vector<Real> instead of the Real* call so that we avoid segfaults.

[williamfgc]

@NAThompson this is great, thanks! Just a couple of things to be more concise in the example

[williamfgc]

@NAThompson I'd follow @chuckatkins advice and make the changes accordingly. This is certainly helpful to illustrate the C++ API.

[NAThompson]

@chuckatkins , @williamfgc : Thanks so much for doing such a thorough code review on this commit; it has really helped me understand the philosophy and approach of this library.

[williamfgc]

Bound cerr and cout to rank 0

[NAThompson]

@germasch identified an issue that I think merits discussion: Some of these C++ exceptions are really unrecoverable errors. Would is make more sense to just call MPI_Abort directly in the MPI builds rather than throw C++ exceptions?

[germasch]

I don't think trying to open a non-existing file should be an unrecoverable error, I think it should become an error an application can recover from (maybe not your usual simulation program, MPI_Abort() is fine for that, but say I wouldn't want my entire visualization package to crash or hang because a file can't be opened).

Basically, what I think should be done is for BP3 to catch any exception before the broadcast, and if so, use the broadcast to broadcast the error status, so that then every MPI process throws from Open. Details need to be figure out, e.g., whether to communicate details about what went wrong -- easiest would be to just have a generic "an error occurred during Open" exception.

[williamfgc]

@germasch @NAThompson good points. The above safety issue is more complex than the initial Open and later MPI_Bcast. It gets pretty complex if any exception caught by any rank (e.g. faulty metadata or glitches) would have to be handled that way: Bcast + throw. My experience (not just with adios, but HPC parallel I/O, which should NOT be taken as a general solution/view) is that applications have the option to do an extra Open/Close using MPI_COMM_SELF with the additional cost of having a "safety" check per rank.

[NAThompson]

Looking at the build, it appears that this change is not the reason for breakage. To wit, the UBSAN build has the following errors:

411: /home/adios2/source/source/adios2/toolkit/interop/hdf5/HDF5Common.cpp:422:17: runtime error: variable length array bound evaluates to non-positive value 0

This appears to be somewhat benign to me.

/usr/include/c++/8/bits/stl_numeric.h:93:4: runtime error: store to misaligned address 0x000000ba53ec for type 'double', which requires 8 byte alignment
9: 0x000000ba53ec: note: pointer points here
9: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
9: ^

Really nothing can be done about the standard library UB.

So presumably this can be merged?

[germasch]

I don't see the errors that you quoted, but I do see two SST errors which I agree aren't related. The usual way of dealing with random failures in the CI is to amend your last commit (without any actual changes, just to updated the timestamp), force-push and keep your fingers crossed.

[NAThompson]

@germasch : A merge commit worked since master has evolved. I'll keep that suggestion in mind for other PRs; thanks!

[germasch]

I'm not sure how people here feel about merge commits in PRs (as opposed to rebasing). Anyway, that's for the actual ADIOS2 people (@williamfgc) to decide.

[williamfgc]

@NAThompson please follow @germasch suggestions on PRs and rebasing as the preferred way. Thank you both.

[chuckatkins]

Rather than a rebase you could also squash merge them into a single commit. That may be easier than dealing with the conflicts a rebase might need addressing. I'd like to see this in the release as well so I'm going to go ahead and change the base branch.

@williamfgc when you're happy with this and it's merged into release I'll pull the update back down to master.

[williamfgc]

@chuckatkins I am happy, our CI is not.

[chuckatkins]

Ugh. The failure is in clang analyzer taking too long on travis. I'll give it a kick and see if it's happier

[NAThompson]

@williamfgc : It looks like any more commits will hit the limits of common decency for requests against the build system; I'll rebase all further PRs rather than merge commit.