Skip to content
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

added async write flag for ADIOS2 #1460

Merged
merged 5 commits into from
Jun 21, 2023
Merged

Conversation

guj
Copy link
Contributor

@guj guj commented Jun 13, 2023

please add a PR description, including links to ADIOS2 docs about the added feature

This PR adds a new env variable OPENPMD_ADIOS2_ASYNC_WRITE. Default = 0. When set to 1, it sets the ADIOS parameter "AsyncWrite" to "on". In which case the BP5 engine will handle write calls asynchronously.

See https://adios2.readthedocs.io/en/latest/engines/engines.html#bp5
for details of BP5 feature "AsyncWrite".

@guj
Copy link
Contributor Author

guj commented Jun 13, 2023

Hi @franzpoeschel @ax3l

Any idea why the test fail with hdf5 on Conda_ompi_all fails on ?

Copy link
Contributor

@franzpoeschel franzpoeschel left a comment

Choose a reason for hiding this comment

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

Can you please document the additional parameter in adios2.rst?

Also, note that in BP5, openPMD will flush to disk by default which makes no sense in combination with async I/O. So, if you add an environment variable for this, it probably makes sense to flip that default in this case.

@ax3l ax3l self-requested a review June 17, 2023 20:52
@ax3l ax3l added this to the 0.16.0 milestone Jun 17, 2023
@guj
Copy link
Contributor Author

guj commented Jun 20, 2023

Can you please document the additional parameter in adios2.rst?

Will do

Also, note that in BP5, openPMD will flush to disk by default which makes no sense in combination with async I/O. So, if you add an environment variable for this, it probably makes sense to flip that default in this case.

Yes, I had to flip the PerformDataWrite. I wonder whether this should an option as well.

@franzpoeschel
Copy link
Contributor

Yes, I had to flip the PerformDataWrite. I wonder whether this should an option as well.

I think that we don't need an extra option for this. I added a commit that sets the default flush target to the Buffer when using this env var.

Copy link
Contributor

@franzpoeschel franzpoeschel left a comment

Choose a reason for hiding this comment

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

LGTM
The failing docs PR run concerns all PRs currently and has nothing to do with this PR.

@franzpoeschel franzpoeschel enabled auto-merge (squash) June 20, 2023 15:47
@guj
Copy link
Contributor Author

guj commented Jun 20, 2023

LGTM
The failing docs PR run concerns all PRs currently and has nothing to do with this PR.

Yes, I had to flip the PerformDataWrite. I wonder whether this should an option as well.

I think that we don't need an extra option for this. I added a commit that sets the default flush target to the Buffer when using this env var.

That will do. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants