Skip to content

Series Flush Type #484

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Series Flush Type #484

wants to merge 8 commits into from

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Feb 19, 2019

Add a new potential data flush mode to Series.

Although we implement a more general model of data passing and accumulating, allowing a "directly synchronous" mode will allow us to target the typical h5py Python user who just quickly wants to explore small data sets without frustration ("why is my data all zeros?").

We definitely have a continued need for our generalized, deferred, asynchronous chunk load/store mechanisms for our applications in parallel I/O and upcoming staging (think: streaming) modes. Just let us enable them explicitly as an "advanced" feature (therefore switch the default to the simple DIRECT mode). This is purely a user-interface change in order to target a wider audience.

The new DIRECT mode should also help somewhat in debugging.

Stylistic note: I think AccessType and FlushType might better be called ...Mode?

@franzpoeschel @C0nsultant what do you think? I hope that description is not too brief, otherwise let's chat :-)
PRs to this branch that implement that mode, also in parts, very welcome.

To Do

@ax3l ax3l added api: breaking breaking API changes api: new additions to the API labels Feb 19, 2019
* Series::flush is called.
* @todo implement: if the shared pointers' `use_count` drops to one
* before Series::flush is called, the load will be skipped during
* Series::flush. (check if this works with our `shareRaw()`
Copy link
Member Author

Choose a reason for hiding this comment

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

this will become possible when #470 is implemented

* Series::flush is called.
* @todo implement: if the shared pointers' `use_count` drops to one
* before Series::flush is called, the store will be skipped during
* Series::flush. (check if this works with our `shareRaw()`
Copy link
Member Author

Choose a reason for hiding this comment

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

this will become possible when #470 is implemented

@ax3l
Copy link
Member Author

ax3l commented Feb 22, 2019

@C0nsultant do you think the right location to store the flush mode would be with AbstractIOHandler?

@ax3l ax3l force-pushed the draft-flushType branch 4 times, most recently from adaf209 to b10925c Compare February 22, 2019 13:58
@@ -86,6 +86,7 @@ TEST_CASE( "hdf5_write_test", "[parallel][hdf5]" )
uint64_t mpi_size = static_cast<uint64_t>(mpi_s);
uint64_t mpi_rank = static_cast<uint64_t>(mpi_r);
Series o = Series("../samples/parallel_write.h5", AccessType::CREATE, MPI_COMM_WORLD);
o.setFlush(FlushType::DEFER); // @todo crashes in DIRECT flush mode
Copy link
Member Author

Choose a reason for hiding this comment

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

see bug #490

@@ -120,6 +123,7 @@ TEST_CASE( "hdf5_write_test_zero_extent", "[parallel][hdf5]" )
uint64_t size = static_cast<uint64_t>(mpi_s);
uint64_t rank = static_cast<uint64_t>(mpi_r);
Series o = Series("../samples/parallel_write_zero_extent.h5", AccessType::CREATE, MPI_COMM_WORLD);
o.setFlush(FlushType::DEFER); // @todo crashes in DIRECT flush mode
Copy link
Member Author

Choose a reason for hiding this comment

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

see bug #490

@anokfireball
Copy link
Member

anokfireball commented Feb 22, 2019

@C0nsultant do you think the right location to store the flush mode would be with AbstractIOHandler?

There (currently) is a 1:1 mapping of Series to IOHandlers, but in theory this could become n:1 in the future (i.e. multiple versions of a Series in the same directory with the same access properties). One could think of scenarios where the different versions have different flush strategies, but I would not worry much about that case as there's one too many if s involved for serious dedication.

Strictly speaking separation of concern, the IO backend (and in this case, the handler) is a perfectly suitable place for it. The flush() calls in the front- and backend have slightly different intentions. The one you care about (actually swapping bytes between RAM and NVstorage, without traversing and syncing the whole frontend tree) is the one the backend is concerned with.

@ax3l
Copy link
Member Author

ax3l commented Feb 22, 2019

Thanks, sounds good.

I actually pushed a first working draft in this PR, for in-code comments/review :)

@ax3l ax3l changed the title [Draft] Data Flush Mode [WIP] Data Flush Mode Feb 22, 2019
@ax3l ax3l removed the discussion label Feb 22, 2019
@ax3l ax3l force-pushed the draft-flushType branch 4 times, most recently from af7c8b2 to 0ba666a Compare February 23, 2019 12:17
@ax3l ax3l changed the title [WIP] Data Flush Mode Data Flush Mode Feb 23, 2019
@ax3l ax3l force-pushed the draft-flushType branch 2 times, most recently from 6bf4c46 to c63f74b Compare February 23, 2019 12:21
@ax3l ax3l changed the title Data Flush Mode Data Flush Type Feb 23, 2019
@ax3l
Copy link
Member Author

ax3l commented Feb 23, 2019

@C0nsultant ready for review :)
Any idea why direct writing in 3_write_serial might be segfaulting? Do we need to flush some path creations? Seems the Record::flush_impl path creation is a bit late as well as the parent/writable registration for new record components if they are not scalar.

@ax3l ax3l changed the title Data Flush Type Series Flush Type Feb 23, 2019
@ax3l ax3l mentioned this pull request Mar 9, 2019
@ax3l ax3l force-pushed the draft-flushType branch from a0f5e7b to 396c879 Compare March 9, 2019 15:12
@ax3l ax3l force-pushed the draft-flushType branch from 396c879 to fc99125 Compare March 9, 2019 22:20
@ax3l
Copy link
Member Author

ax3l commented May 13, 2019

@C0nsultant if you have the time, the logic in flushing here and in #490 needs to be refactored by us to be more flexible with flushes triggered from a record component (instead of top-down from a series-iteration-record-...) :-)

@ax3l ax3l force-pushed the draft-flushType branch from fc99125 to 6983e8d Compare July 1, 2019 14:01
@ax3l ax3l force-pushed the draft-flushType branch from 6983e8d to 50f808b Compare August 28, 2019 17:38
@ax3l ax3l force-pushed the draft-flushType branch 2 times, most recently from 650b9c9 to d316e66 Compare November 5, 2019 00:52
@ax3l ax3l mentioned this pull request Nov 19, 2019
ax3l added 8 commits January 27, 2020 13:23
Add a new data flush mode to Series.

Although we implement a more general model of data passing
and accumulating, allowing a "directly synchronous" mode will
allow us to target the typical h5py Python user who just quickly
wants to explore small data sets without frustration.

We definitely have a continued need for our generalized, deferred,
asynchronous chunk load/store mechanisms for our applications in
parallel I/O and upcoming staging (think: streaming) modes. Just
let us enable them explicitly as an "advanced" feature.
Try to use deferred mode for most tests, but add some
direct ones as well.

Especially, with fixed defaults for particle position/positionOffset
creation, we should add direct mode tests for CREATE of those as
well.
Document the new Series flush type and how to upgrade from
previous releases, as this is a breaking change.
dynamic casts can return nullptrs ;-)

  openPMD#471
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: breaking breaking API changes api: new additions to the API help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants