availableChunks open logic (HDF5: Early Chunk Read) #1035

ax3l · 2021-07-08T06:06:31Z

This adds a test for early reading of chunks (via availableChunks) with HDF5 that appears quite often with post-processing routines based on chunks.

I have not yet found the reason for this fail, but it seems to be HDF5 specific (details documented in #961). Either the writable is a bit off or the file not yet open.

add test
fix Early Query: availableChunks & HDF5 #961
merge Fix h5 early chunk improved iteration open ax3l/openPMD-api#1
update MPI - Collective Behavior:
- document the "first call to loadChunk/storeChunk/availableChunks is collective" unless we first called ::open();
  expand that Iteration::open() is used only for (MPI || deferred iteration parsing), elaborate on open() relation to flushing logic
- add availableChunks to table (behavior independent unless it's the first call to a freshly opened series)

Add test.

ax3l · 2021-07-08T06:52:41Z

test/SerialIOTest.cpp

+            for( auto & r_c : r.second )
+            {
+                std::cout << r_c.first << "\n";
+                if( !r_c.second.constant() )


Memo to myself and unrelated to the issue: this if can probably be removed now that #942 is implemented :)

Also: let's do something with the chunks variable (e.g. print it).

franzpoeschel · 2021-07-08T08:59:39Z

Ref.: https://github.com/openPMD/openPMD-api/pull/862/files
tldr: No file handles are open after parsing in file-based iteration encoding. Fixed by using auto electrons = s.iterations[400].open().particles["electrons"];.

This test should fail for other backends, too. I claim "no bug".

ax3l · 2021-07-08T15:41:37Z

Thanks for the ideas!

Why I tend to think that this is a usability bug and we should auto-open iterations in availableChunks() calls is because we do the same for loadChunks().

I mean also from the API contract it's logical that if a serial

Series s = Series(...);

auto electrons = s.iterations[400].particles["electrons"];
auto chunks = electrons["position"]["x"].loadChunk();

works than a

Series s = Series(...);

auto electrons = s.iterations[400].particles["electrons"];
auto chunks = electrons["position"]["x"].availableChunks();

should work the same way. Especially since the latter would be called before the former now with #802-guided processing.

Ref.: https://github.com/openPMD/openPMD-api/pull/862/files

Note that #862 is a MPI-parallel context where we want to trigger a MPI-collective open call with a collective guarantee because we use loadChunks() and flush() in a non-collective way as the following first data calls (see test in that PR). The problem here is serial.

If course, if someone called in an MPI-parallel context availableChunks() as the first thing and only from one rank, then they would be again required to first perform a collective ::open() on the iteration, the same way as they need to for loadChunk().

This test should fail for other backends, too.

The problem became apparent to me in serial reads with src/binding/python/openpmd_api/DaskDataFrame.py, where we have no access to the iteration. In my tests, ADIOS2 reads work and HDF5 reads fail (both are serial, file-based encoding).

franzpoeschel · 2021-07-12T08:53:49Z

src/backend/BaseRecordComponent.cpp

+    this->dirty() = true;
+    this->seriesFlush();
+    this->dirty() = false;
+


I'm a bit doubtful about this..

It makes the availableChunks call collective, we should at least document that

availableChunks will now silently flush the whole Series. This will break user code that peruses our new flushing logic and its guarantees of no writes occurring until flushing. If we really want to do this, this function must not do anything more than merely opening those files.

franzpoeschel · 2021-07-12T09:04:21Z

Thanks for the ideas!

Why I tend to think that this is a usability bug and we should auto-open iterations in availableChunks() calls is because we do the same for loadChunks().

I mean also from the API contract it's logical that if a serial
Series s = Series(...);

auto electrons = s.iterations[400].particles["electrons"];
auto chunks = electrons["position"]["x"].loadChunk();

This code does not open the iteration, s.flush() does. Interacting with an iteration fundamentally requires opening it in openPMD, but Series::flush() as well as the Streaming API will do it automatically. The fundamental difference between availableChunks() and loadChunk() at this point is that the latter is a deferred operation and hence receives all the properties of a deferred operation, including that it is performed by flushing.

works than a
Series s = Series(...);

auto electrons = s.iterations[400].particles["electrons"];
auto chunks = electrons["position"]["x"].availableChunks();
should work the same way. Especially since the latter would be called before the former now with #802-guided processing.

Ref.: https://github.com/openPMD/openPMD-api/pull/862/files

This would make availableChunks() collective too, while loadChunk() can remain non-collective (collective business is dealt with in Series::flush()). Just as imbalanced as before, though even a bit trickier to debug.

Note that #862 is a MPI-parallel context where we want to trigger a MPI-collective open call with a collective guarantee because we use loadChunks() and flush() in a non-collective way as the following first data calls (see test in that PR). The problem here is serial.

We're making the problem parallel by implicit opening of files (and with the current implementation: implicit flushing).

If course, if someone called in an MPI-parallel context availableChunks() as the first thing and only from one rank, then they would be again required to first perform a collective ::open() on the iteration, the same way as they need to for loadChunk().

This test should fail for other backends, too.

The problem became apparent to me in serial reads with src/binding/python/openpmd_api/DaskDataFrame.py, where we have no access to the iteration. In my tests, ADIOS2 reads work and HDF5 reads fail (both are serial, file-based encoding).

It's weird that this would even work in ADIOS2, I'll need to have a look at what's going on there.

Do you want Iteration::open() fundamentally to be necessary in parallel contexts only? I'm a bit hesitant to add exceptions to using Iteration::open() here and there, but if we set that as a fundamental guideline, and have some clean general concepts, that's another story then.

franzpoeschel · 2021-07-12T11:25:15Z

After following the behavior of ADIOS2 in the debugger, this line implicitly opens an ADIOS2 engine if not yet happened. I would declare this a bug, such things should only happen if explicitly requested by the frontend.

ax3l · 2021-07-13T07:01:04Z

We will discuss this further on Wednesday.

I think the cleanest way would be to:

implicitly open the file if still closed; if this is the first call than this makes it collective (otherwise this is an independent call)
- serial reads are also significantly simpler that way, e.g. the demonstrator test in this PR
make sure Iteration::open() works for this context, currently it's not the case (tried it)

I would thus also discuss if we want to apply #1045 really or if we instead rather implement a similar solution here.

We currently don't need Iteration::open() besides for the described independent-first-access context above. If we want to change that we need to improve error reporting on non-opened backends I think and we probably need to advertise that Iteration::open() always needs to be called for consistency.

franzpoeschel · 2021-07-14T17:39:54Z

Memo to self: Try why Iteration::open() won't work in this context

test/SerialIOTest.cpp

ax3l · 2021-07-14T17:42:47Z

test/SerialIOTest.cpp

+    try
+    {
+        Series s = Series(
+            "../samples/git-sample/data%T.h5",


Discussed: add (file based) adios2 test, too due to #1045

franzpoeschel · 2021-07-16T13:11:33Z

elaborate on open() relation to flushing logic

The changes in ax3l#1 make open() independent from flushing logic.

This reverts commit e2d54ba.

Fix conflicts: - docs/source/details/mpi.rst

include/openPMD/backend/Attributable.hpp

src/backend/Attributable.cpp

ax3l · 2021-07-20T17:41:07Z

test/SerialIOTest.cpp

@@ -4288,6 +4326,48 @@ void deferred_parsing( std::string const & extension )
                std::numeric_limits< float >::epsilon() );
        }
    }
+    {
+        Series series(


Note: also fixes & tests read_write lazy parsing in this diff.

HDF5: Early Chunk Read

50ce515

Add test.

ax3l added bug backend: HDF5 labels Jul 8, 2021

ax3l requested a review from franzpoeschel July 8, 2021 06:06

ax3l assigned franzpoeschel Jul 8, 2021

ax3l commented Jul 8, 2021

View reviewed changes

Fix availableChunks: open Series

e2d54ba

ax3l mentioned this pull request Jul 8, 2021

Early Query: availableChunks & HDF5 #961

Closed

franzpoeschel reviewed Jul 12, 2021

View reviewed changes

franzpoeschel mentioned this pull request Jul 12, 2021

ADIOS2: Don't implicitly open files #1045

Merged

2 tasks

ax3l changed the title ~~HDF5: Early Chunk Read~~ availableChunks open logic (HDF5: Early Chunk Read) Jul 14, 2021

ax3l commented Jul 14, 2021

View reviewed changes

test/SerialIOTest.cpp Show resolved Hide resolved

ax3l commented Jul 14, 2021

View reviewed changes

franzpoeschel mentioned this pull request Jul 15, 2021

Fix h5 early chunk improved iteration open ax3l/openPMD-api#1

Merged

3 tasks

franzpoeschel added 9 commits July 19, 2021 23:37

Revert "Fix availableChunks: open Series"

13ad572

This reverts commit e2d54ba.

Refactor procedures for opening an iteration

ad1735a

Use last commit in Iteration::open()

b594c04

Use improved Iteration::open() in availableChunks

fe312ac

Remove redundant status writing

2d6bf77

Some inline commenting

5589a07

Make this whole thing work in READ_WRITE mode too

5b72c3c

Use specific commit for samples download

22dddf3

Fix bad static_cast

1bb9ca2

franzpoeschel and others added 2 commits July 19, 2021 23:37

More complete documentation on Iteration::open()

c315ff3

Merge remote-tracking branch 'mainline/dev' into fix-h5EarlyChunk

9e8a85b

Fix conflicts: - docs/source/details/mpi.rst

ax3l commented Jul 20, 2021

View reviewed changes

ax3l added 2 commits July 20, 2021 10:42

Add VC code review

782c78e

Merge remote-tracking branch 'mainline/dev' into fix-h5EarlyChunk

e02da0b

ax3l force-pushed the fix-h5EarlyChunk branch from 080ac6c to e02da0b Compare July 21, 2021 07:22

ax3l enabled auto-merge (squash) July 21, 2021 07:22

ax3l disabled auto-merge July 21, 2021 16:09

ax3l merged commit 020177a into openPMD:dev Jul 21, 2021

ax3l deleted the fix-h5EarlyChunk branch July 21, 2021 16:09

ax3l mentioned this pull request Mar 7, 2023

Close HFD5 handles in availableChunks task #1386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

availableChunks open logic (HDF5: Early Chunk Read) #1035

availableChunks open logic (HDF5: Early Chunk Read) #1035

ax3l commented Jul 8, 2021 •

edited

Loading

ax3l Jul 8, 2021 •

edited

Loading

franzpoeschel commented Jul 8, 2021 •

edited

Loading

ax3l commented Jul 8, 2021 •

edited

Loading

franzpoeschel Jul 12, 2021

franzpoeschel commented Jul 12, 2021 •

edited

Loading

franzpoeschel commented Jul 12, 2021

ax3l commented Jul 13, 2021 •

edited

Loading

franzpoeschel commented Jul 14, 2021

ax3l Jul 14, 2021

franzpoeschel commented Jul 16, 2021

ax3l Jul 20, 2021

availableChunks open logic (HDF5: Early Chunk Read) #1035

availableChunks open logic (HDF5: Early Chunk Read) #1035

Conversation

ax3l commented Jul 8, 2021 • edited Loading

ax3l Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

franzpoeschel commented Jul 8, 2021 • edited Loading

ax3l commented Jul 8, 2021 • edited Loading

franzpoeschel Jul 12, 2021

Choose a reason for hiding this comment

franzpoeschel commented Jul 12, 2021 • edited Loading

franzpoeschel commented Jul 12, 2021

ax3l commented Jul 13, 2021 • edited Loading

franzpoeschel commented Jul 14, 2021

ax3l Jul 14, 2021

Choose a reason for hiding this comment

franzpoeschel commented Jul 16, 2021

ax3l Jul 20, 2021

Choose a reason for hiding this comment

ax3l commented Jul 8, 2021 •

edited

Loading

ax3l Jul 8, 2021 •

edited

Loading

franzpoeschel commented Jul 8, 2021 •

edited

Loading

ax3l commented Jul 8, 2021 •

edited

Loading

franzpoeschel commented Jul 12, 2021 •

edited

Loading

ax3l commented Jul 13, 2021 •

edited

Loading