Fix append mode double attributes by franzpoeschel · Pull Request #1302 · openPMD/openPMD-api

franzpoeschel · 2022-07-14T15:41:08Z

When appending to an existing group/variable-based Series, don't define top-level attributes again (except basePath and openPMD, these are needed internally and ADIOS2 doesn't care as long as they are duplicate but equivalent)
This fixes an Append mode bug that is already present on dev and has a low chance of making CI runs fail.

Merge ADIOS2: more fine-grained control for file endings #1218 first
Add CHECK_FILE task for other backends too (maybe not for ADIOS1)
~~maybe merge CHECK_FILE and CREATE_FILE tasks~~, no this would require executing CREATE_FILE very early
cleanup
Support file creation via Append mode in group-based iteration encoding

~~This seems to trigger an ADIOS2 bug?:~~ I get this on dev already, will need to see what's going on

append_mode                                                                                                                                                                                                     
-------------------------------------------------------------------------------                                                                                                                                 
/home/franzpoeschel/git-repos/openPMD-api/test/SerialIOTest.cpp:6486                                                                                                                                            
...............................................................................                                                                                                                                 
                                                                                                                                                                                                                
/home/franzpoeschel/git-repos/openPMD-api/test/SerialIOTest.cpp:6486: FAILED:                                                                                                                                   
  {Unknown expression after the reported line}                                                                                                                                                                  
due to unexpected exception with message:                                                                                                                                                                       
  [Tue Sep 20 14:18:56 2022] [ADIOS2 EXCEPTION] <Engine>                                                                                                                                                        
  <BP5Reader> <ParseMetadataIndex> : Record 5 (id = 115) has invalid length 48.                                                                                                                                 
  We parsed 2195488 bytes for this record

This reverts commit a39b618. The test is added in PR openPMD#1302 which fixes the bug

include/openPMD/IO/AbstractIOHandlerImpl.hpp

franzpoeschel · 2022-08-16T14:42:41Z

src/Series.cpp

        int padding;
        uint64_t iterationIndex;
        std::set<int> paddings;
-        for (auto const &entry : auxiliary::list_directory(directory))


This fixes another subtle bug: Creating a Series via Append mode should be possible. But so far, this routine failed if the directory was not present.

include/openPMD/auxiliary/Mpi.hpp

even if the specified directory does not exist

Needed later for checking file presence in parallel situations

Also use parallel logical or for file existence check

src/IO/ADIOS/ADIOS2IOHandler.cpp

include/openPMD/auxiliary/Mpi.hpp

include/openPMD/Series.hpp

ax3l · 2022-09-19T15:26:39Z

include/openPMD/auxiliary/Mpi.hpp

+namespace
+{
+    template <typename>
+    struct MPI_Types;


Future refactoring: since this is an openPMD internal type trait, we should consider renaming this so it does not collide with the MPI_.... prefix used as convention in the MPI C libraries for all its functions.

Suggestions: MPI_Types -> openPMD_MPI_Type

Looking at this, I'm asking myself if this code is even correct. If an MPI application runs on a heterogeneous system with differing CPU architectures, there will be undefined behavior, no?
Maybe, we should just use the largest numeric type for those cases? Or is there a better way?

This depends a bit and would require more work, but I think not in the location here.

heterogeneous system with differing CPU architectures

The questions here is not really the CPU (micro) architecture but the data model: https://en.cppreference.com/w/cpp/language/types Essentially all modern systems you would put on an HPC system are LP64.

If you would like to run with MPI on a system with heterogeneous hosts, you would have to compile your binaries for each of them and spawn them with MPI. That's the easy part, the hard part would be if someone ever implemented an MPI that would allow to communicate between LP64 and, let's say ILP32, or LP64 with big and little endian.

Either way, it would not change the local code in this location.

I think we can actually simplify this by using static constexpr MPI_Datatype const value = ...;

We can probably even make this a simpler constexpr function now, which might also benefit compile time.

constexpr will probably not work fully since the MPI datatypes are not constexpr in every implementation, but we can still replace the specialized class template with if constexpr, yeah.

If you would like to run with MPI on a system with heterogeneous hosts, you would have to compile your binaries for each of them and spawn them with MPI. That's the easy part, the hard part would be if someone ever implemented an MPI that would allow to communicate between LP64 and, let's say ILP32, or LP64 with big and little endian.

Either way, it would not change the local code in this location.

Fair enough, thanks for clarifying!

for more information, see https://pre-commit.ci

I think that this won't work with every MPI implementation

ornladios/ADIOS2#3358

ax3l

Splendid, thanks a lot! :)

* dev: (70 commits) Docs: Recommend Static Build for Superbuilds (openPMD#1325) Python 3.11 (openPMD#1323) pybind11: v2.10.1+ (openPMD#1322) Add Attribute::getOptional<T>() and use to add some more dynamic datatype conversions at read time (openPMD#1278) Mapping between ADIOS steps and openPMD iterations (openPMD#949) Deprecate shareRaw (openPMD#1229) Fix append mode double attributes (openPMD#1302) Constant scalars: Don't flush double (openPMD#1315) Remove caching cmake vars (openPMD#1313) [pre-commit.ci] pre-commit autoupdate (openPMD#1311) storeChunk: Add an overload for shared_ptr<T[]> (openPMD#1296) Fix `operationAsString` Export (openPMD#1309) ADIOS2: more fine-grained control for file endings (openPMD#1218) [pre-commit.ci] pre-commit autoupdate (openPMD#1307) Fix file existence check in parallel tests (openPMD#1303) ADIOS2: Flush to disk within a step (openPMD#1207) [pre-commit.ci] pre-commit autoupdate (openPMD#1304) [pre-commit.ci] pre-commit autoupdate (openPMD#1295) Update catch2 to v2.13.9 (openPMD#1299) [pre-commit.ci] pre-commit autoupdate (openPMD#1292) ... # Conflicts: # .github/workflows/linux.yml

franzpoeschel added bug backend: ADIOS2 labels Jul 14, 2022

franzpoeschel mentioned this pull request Jul 15, 2022

Fix append mode double attributes franzpoeschel/openPMD-api#3

Closed

franzpoeschel force-pushed the fix-append-mode-double-attributes branch from 719e8db to 2f56ac0 Compare July 15, 2022 13:53

franzpoeschel changed the title ~~[WIP] Fix append mode double attributes~~ Fix append mode double attributes Jul 18, 2022

franzpoeschel force-pushed the fix-append-mode-double-attributes branch 2 times, most recently from 8315402 to f5e511f Compare July 19, 2022 09:40

franzpoeschel added a commit to franzpoeschel/openPMD-api that referenced this pull request Jul 19, 2022

Revert "Test for double attribute issue in ADIOS2 v2.7.1"

611d834

This reverts commit a39b618. The test is added in PR openPMD#1302 which fixes the bug

franzpoeschel force-pushed the fix-append-mode-double-attributes branch 2 times, most recently from f329c8c to 1b5b053 Compare July 20, 2022 13:19

franzpoeschel force-pushed the fix-append-mode-double-attributes branch from a6c67b4 to 22be635 Compare July 29, 2022 09:05

franzpoeschel force-pushed the fix-append-mode-double-attributes branch from 22be635 to 0f22a17 Compare August 15, 2022 09:07

franzpoeschel mentioned this pull request Aug 15, 2022

Fix operationAsString Export #1309

Merged

franzpoeschel commented Aug 16, 2022

View reviewed changes

include/openPMD/IO/AbstractIOHandlerImpl.hpp Outdated Show resolved Hide resolved

franzpoeschel commented Aug 16, 2022

View reviewed changes

franzpoeschel force-pushed the fix-append-mode-double-attributes branch from 0f22a17 to 77215b9 Compare August 16, 2022 14:51

ax3l self-requested a review August 16, 2022 17:17

ax3l self-assigned this Aug 16, 2022

ax3l reviewed Aug 16, 2022

View reviewed changes

include/openPMD/auxiliary/Mpi.hpp Outdated Show resolved Hide resolved

ax3l mentioned this pull request Aug 16, 2022

Mapping between ADIOS steps and openPMD iterations #949

Merged

11 tasks

franzpoeschel added 9 commits August 17, 2022 11:22

Add failing test

39ed7be

Allow creating file-based Series with Append mode

98e2024

even if the specified directory does not exist

Add Mpi.hpp with MPI helpers

291d7f8

Needed later for checking file presence in parallel situations

Add CHECK_FILE IO task and implement in the backends

7a2adec

Only write top-level attributes when really needed

e878cfc

Enable Series creation in Append mode

8b258b4

Add Windows CI workaround

d5454df

Windows Workaround: Fallback to Create mode

4b56d1b

Avoid MPI mocking, use ifdefs

2bc1ac4

Also use parallel logical or for file existence check

franzpoeschel force-pushed the fix-append-mode-double-attributes branch from ac5504f to 2bc1ac4 Compare August 17, 2022 09:25

Append mode: test in parallel

90194ad

franzpoeschel force-pushed the fix-append-mode-double-attributes branch from 8972ff0 to 90194ad Compare August 17, 2022 14:30

Don't alias sendbfr and recvbfr

013946e

ax3l reviewed Sep 19, 2022

View reviewed changes

src/IO/ADIOS/ADIOS2IOHandler.cpp Outdated Show resolved Hide resolved

ax3l reviewed Sep 19, 2022

View reviewed changes

include/openPMD/auxiliary/Mpi.hpp Outdated Show resolved Hide resolved

ax3l reviewed Sep 19, 2022

View reviewed changes

include/openPMD/Series.hpp Outdated Show resolved Hide resolved

ax3l reviewed Sep 19, 2022

View reviewed changes

franzpoeschel and others added 6 commits September 20, 2022 16:21

Undo unused changes

03efb78

Remove unused Mock_MPI_Comm

d63c7e7

[pre-commit.ci] auto fixes from pre-commit.com hooks

1f890d2

for more information, see https://pre-commit.ci

Replace MPI specializations by if constexpr

ea0c3bb

Try making this constexpr

e55df66

I think that this won't work with every MPI implementation

Link ADIOS2 issue

32f9fbb

ornladios/ADIOS2#3358

franzpoeschel requested a review from ax3l October 4, 2022 09:00

ax3l approved these changes Oct 7, 2022

View reviewed changes

ax3l merged commit 230afdd into openPMD:dev Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix append mode double attributes#1302

Fix append mode double attributes#1302
ax3l merged 17 commits intoopenPMD:devfrom
franzpoeschel:fix-append-mode-double-attributes

franzpoeschel commented Jul 14, 2022 •

edited

Loading

Uh oh!

Uh oh!

franzpoeschel Aug 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ax3l Sep 19, 2022 •

edited

Loading

Uh oh!

franzpoeschel Sep 22, 2022

Uh oh!

ax3l Sep 23, 2022

Uh oh!

ax3l Sep 23, 2022

Uh oh!

ax3l Sep 23, 2022

Uh oh!

franzpoeschel Sep 23, 2022 •

edited

Loading

Uh oh!

ax3l left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

franzpoeschel commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

franzpoeschel Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ax3l Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franzpoeschel Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

franzpoeschel Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

franzpoeschel commented Jul 14, 2022 •

edited

Loading

ax3l Sep 19, 2022 •

edited

Loading

franzpoeschel Sep 23, 2022 •

edited

Loading