Conversation
0cd47a2 to
10b1191
Compare
2df5f1b to
5de0dde
Compare
|
Does someone have suggestions on how to handle
|
|
Hm, besides current hiccups reading ADIOS1 files with ADIOS2 I think the first option is the cleanest and makes sense. If we have the choice to use the newer C++ ADIOS2 code base this is preferable over the ADIOS1 C code base. |
61165d5 to
16c30a5
Compare
|
This sounds related to the issues that I am encountering with opening BP files written by ADIOS1 with ADIOS2. For reference, I created a small sample file, once using the ADIOS1 and once the ADIOS2 backend of the openPMD API. Reading the ADIOS1 file with ADIOS1's bpls serial1.bp
unsigned long long /tmp/data/100/meshes/E/x_chunkSize0 scalar
unsigned long long /tmp/data/100/meshes/E/x_chunkOffset0 scalar
unsigned long long /tmp/data/100/meshes/E/x_chunkSize1 scalar
unsigned long long /tmp/data/100/meshes/E/x_chunkOffset1 scalar
double /data/100/meshes/E/x {3, 3}
byte /__adios__/timer_labels_1 {8, 16}
double /__adios__/timers_1 {1, 8}The output contains statistical data written by ADIOS1, suggesting that this is the same corner case discussed in the referenced thread. Trying to use ADIOS2's The file written by the ADIOS2 backend can be read by either version of Statistical data is not present here. Is it worth further pursuing the corner case (?) of reading ADIOS1 files by using the ADIOS2 backend? |
f76f376 to
7dadc81
Compare
Looks to me like the counter reading of ADIOS1 statistics was fixed in ADIOS2 in your linked thread. Can you just try if it works with the latest |
71fead4 to
785d733
Compare
785d733 to
4950953
Compare
4950953 to
2334007
Compare
|
@franzpoeschel can you please rebase your PR against the latest |
2334007 to
e44f6a4
Compare
ax3l
left a comment
There was a problem hiding this comment.
Little inline question.
Feel free to remove the [WIP] in the title and ping me when it's ready for merge :)
src/IO/ADIOS/ADIOS2IOHandler.cpp
Outdated
| { | ||
| } | ||
|
|
||
| #else |
There was a problem hiding this comment.
is this #else still correct? Shouldn't this be an #endif?
|
@franzpoeschel ping: any updates I should review? :) |
|
I pushed two small commits, one to fix compression and another one to fix the |
ed29f26 to
9a161be
Compare
|
Cool, thx. Please feel free to remove the [WIP] in the title when you consider the PR ready :)
|
|
I introduced a |
9a161be to
e056673
Compare
|
@franzpoeschel Please rebase this PR against the latest |
|
@franzpoeschel ping :) What's the status? |
|
I am currently looking through my branches that build upon this in order to see whether some commits may also be relevant for this. After that, I think you can review it. |
e056673 to
73ea62d
Compare
Undebugged and erroneous writing and reading version of ADIOS2 backend Add InvalidatableFile class Implement (parts of) ADIOS2IOHandlerImpl Create AbstractIOHandlerImplCommon Large parts of the concrete implementation is the same across backends, try to capture that. Frontend part of JSON backend implementation Implement JSON backend to dummy state Misses the actual implementation of AbstractIOHandlerImpl Declare IOHandlerImpl for JSON and integrate it with other places Misses the implementation. Undebugged minimum implementation for JSON writing First basically runnable version of JSON writing To address: No reading or deleting yet. Datatypes are currently ignored and the data is assumed to be int64_t. Attribute values are ignored and replaced with a dummy value. If a subgroup name can be parsed as a nonnegative string, the JSON API will create a JSON array rather than a JSON object (associative array) as intended. Correctly handle groups that can be parsed as int See last commit's description. Fix index calculation with offsets in WriteData Fix some mistakes in JSON writing Correctly handle overwriting files: -> overwritten files should not be possible to access any longer and be clearly distinguished from the newly-created file Make some verifications execute independent of compiler options. Full implementation of JSON writing Respects all datatypes now. Format code according to Clion Stylesheet https://github.com/ComputationalRadiationPhysics/contributing/blob/master/IDESettings/CLion/CRP_CLion2016_1.xml Add generic branching over an openPMD datatype First runnable version of JSON Reading Cleanup and implementation of dataset extension Undebugged version of JSON deletion Properly (de)serialize datatypes Instead of casting the Datatype enum to and from int (which is likely to break when altering the enum), serialize to and from String values. Fix a number of mistakes in JSON reading and writing Cleanup Add JSON tests and fix bugs found thusly Add further tests and fix a further bug The JSON library does not understand long double values (i.e. 128bit floats), represent them as a char array. Handle floating point special values JSON represents +/-Infinity and NaN values as null. The JSON library will correctly serialize those values *to* JSON, implement (semi)-correct handly for deserialization. As it is unclear which exact value a null represents, deserialize it to NaN. Take notice that large floating point values (128 bit) might be serialized to null as well. Use std::is_floating_point to distinguish them from other types Additionally write the byte width of the underlying type Not yet used in reading Mark the writable written after successfully extending a dataset Remove support for absolute paths from openPath Fix some rough edges from rebasing Add documentation for the JSON backend Integrate the JSON backend with the build system Further implement ADIOS2 backend First basically writing commit Further implement ADIOS2 backend Open an IO for each file to write to Move switchType structs to detail namespace Further implement ADIOS2 backend Mainly: handling non-trivial datatypes in writing operations Replace MPI::Init and MPI::Finalize with MPI_Init and MPI_Finalize Fix destruction order Implement readAttribute Refactor helper structs less boilerplate Implement readDataset WIP: refactor Engine opening and closing Execute buffered actions in original order Bugfixing First successful read Before cleanup Does still not write the correct values as datasets. Hoping to find the reason during cleanup. Cleanup, Clang compiler support Add support for non-MPI ADIOS2 Also enable building without ADIOS2. Overwrite existing Attributes Bugfixes Adapt to accessType -> accessTypeBackend refactoring Various datatype fixes Fix listPaths bug Fix openPath Bug Paths are opened relative to the Writable's parent. Passes all serial test By implementing a workaround for ADIOS2 currently not yet supporting Append mode. Use m_dirty to postpone Engine opening Delay attribute reading, allowing us to open Engines later Add validity checks for returned ADIOS2 objects Switch to clang-format Clion formatter does not understand all templates Activate test cases for ADIOS2 backend Cleanup: Imports and Formatting Update documentation Concerns documentation for building the openPMD API. Documentation on the usage of the ADIOS2 backend is not yet present. Fix parallel test Throw exception upon deletion instead of silently passing on Add configuration options via environment variables OPENPMD_ADIOS_HAVE_PROFILING, OPENPMD_ADIOS_HAVE_METADATA_FILE and OPENPMD_ADIOS_NUM_SUBSTREAMS Add documentation for ADIOS2 backend Add support for compression Zfp and Sz Remove debug output from Datatype.cpp Simplify the handling of attributes So far, the ADIOS2 backend distinguished attributes of datasets and of groups. Unify this. Add ADIOS2 to continuous integration - WIP experiencing compiler version woes with travis atm For each build that uses ADIOS1, add an equivalent build that replaces ADIOS1 with ADIOS2 Notice that a single build cannot support both versions of ADIOS, i.e. when building with both versions enabled, the runtime will always decide in favor of ADIOS2 Hence, do not add ADIOS2 to code coverage (for now) Deactivate ADIOS2 debug mode Revert .travis.yml to dev version Fix constructors of ADIOS2 classes Previous versions did not work correctly with MPI enabled. Also, not all constructors took compression into consideration. Fix pointer slicing Deactivate non-MPI versions of ADIOS2 backend when building with MPI Since ADIOS2 cannot be used without MPI if it has been built with support for MPI, we do it the same way (for now?). Deactivate tests accordingly. Add new ADIOS2 type names Activate serial ADIOS2 when building with MPI Due to restrictions in ADIOS2, this was hitherto impossible. Fix documentation and rename environment variables rename OPENPMD_ADIOS_* to OPENPMD_ADIOS2_* Fix picking of backends in tests Add '.bp' file ending at most once and enable serial ADIOS2 tests also when compiling with support for MPI. Remember whether an attribute is boolean Since we store boolean attributes as uchar, their type cannot be retrieved later properly. Explicitly store the type. Refactor compression to support any techniques that ADIOS2 supports Fix preprocessor macros in case ADIOS2 is not available
73ea62d to
909fa5e
Compare
|
@ax3l Ready for review from my side :) |
There was a problem hiding this comment.
Really nice work, thank you so much.
I added minor comments inline and then we should be able to merge this :)
Also, ADIOS2 v2.5.0 was just released. Any new features we want and love, to start with 2.5.0+ only?
Just for curiosity, when do you think a PR with the SST work is likely? Can you open the further features as [WIP] as well?
| { | ||
| namespace detail | ||
| { | ||
| // ADIOS2 does not natively support boolean values |
There was a problem hiding this comment.
urgh, right -.-
but for which attributes do we use bool in openPMD? At least in the standard and extensions, I tried to avoid them due to the lack of a C99 implementation, lack of standard way in HDF5/ADIOS1 and tricky way in std::vector<bool>.
There was a problem hiding this comment.
I think we don't use it yet. I have used it in my topic-streaming branch to indicate whether an iteration has been finalized, but I noticed that it is not so straightforward to do so. Since bools are coerced to unsigned char, you need to pay attention when reading whether an attribute was bool or whether it was genuinely an unsigned char. If I remember correctly, the other backends don't handle this case yet. My assumption would be that this hasn't come up yet since bools have not been used?
We might think about removing BOOLEAN alltogether from openPMD::Datatypes?
There was a problem hiding this comment.
It's probably not a bad idea, let's document this in an issue. At least for HDF5, there are some conventions to express bools. But many users are unaware of the implications for their data and it opens a lot of corner-cases for little gain. Needs probably more input.
src/IO/ADIOS/ADIOS2IOHandler.cpp
Outdated
| auto filePos = setAndGetFilePosition( writable, name ); | ||
| filePos->gd = ADIOS2FilePosition::GD::DATASET; | ||
| auto varName = filePositionToString( filePos ); | ||
| // we use a unique_ptr to circumvent the fact that std::optional |
There was a problem hiding this comment.
Btw, I you really want it for internal use, I can add optional-lite:
https://gist.githubusercontent.com/ax3l/b4ae0d439c8503cfc08b9c04d65f1327/raw/f750ada9a6460f4df05657827f70dea574165c9c/awesome_standalone.md
There was a problem hiding this comment.
I think this would be helpful to have. At least, I missed having something similar to this several times now, and it's arguably better than a type that also forces us to do heap allocations.
There was a problem hiding this comment.
I can provide you that, documented in #563.
We must just avoid to use it in user-facing header files.
BP4 engine does not write files, but directories which becomes relevant in tests for filebased layout.
Without formatting and licensing headers.
378e57b to
38e7c4f
Compare
ax3l
left a comment
There was a problem hiding this comment.
Thank you so much, this is really great work! ✨ 🚀
We have ADIOS2 support, yay!!
| environment variable default description | ||
| ===================================== ======= =================================================================== | ||
| ``OPENPMD_ADIOS2_HAVE_PROFILING`` ``1`` Turns on/off profiling information right after a run. | ||
| ``OPENPMD_ADIOS2_HAVE_METADATA_FILE`` ``1`` Online creation of the adios journal file (``1``: yes, ``0``: no). |
There was a problem hiding this comment.
@franzpoeschel are you sure that is still correct?
By default, this now creates a <name>.bp/ directory in ADIOS2 and bpls (among others) seams to be able to operate directly on that - without an extra metadata file.
| ---------------------------- | ||
|
|
||
| A good practice at scale is to disable the online creation of the metadata file. | ||
| After writing the data, run ``bpmeta`` on the (to-be-created) filename to generate the metadata file offline (repeat per iteration for file-based encoding). |
There was a problem hiding this comment.
see above, maybe we double-check the new workflow.
ADIOS2 backend. Close #148
To Do
.bp) – find and implement a better solution(ADIOS2 can currently not yet open all files written by ADIOS1)
diffin the PRREADME.md,docs/source/dev/[buildoptions|dependencies].rst