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

Fixes for variable-based encoding in backends without step support #1484

Merged
merged 12 commits into from
Aug 9, 2023

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Jul 25, 2023

Close #1481

While working on the fix, I noticed a few other related issues. This PR contains workflow fixes for using variable-based encoding in backends that don't support IO steps:

  • Throw an error when trying to create a second IO step in a non-supporting backend
  • Use READ_LINEAR access in variableBasedSingleIteration test (READ_ONLY also works when there are no steps involved, but prints a warning in variable-based encoding)
  • In READ_LINEAR mode, the constructor does not parse anything, parsing must be actively triggered. Series::readIterations() is a misleading name for this, so introduce Series::parseBase() as an alias.
  • I noticed that Series::readIterations() returns a new iterator instance for each call. Since this iterator modifies the state of the Series, it only makes sense to have a single instance of it. Store that instance internally in the Series.
  • file-based iteration encoding must not write snapshot attribute at Series::close() time, but at Iteration::close() instead
  • Do not write the snapshot attribute during initialization of an iteration
  • Ensure that each call to AbstractIOHandler::flush() sets m_lastFlushSuccessful (otherwise, a failed next iteration might leak into the output during the destructor run)

@franzpoeschel franzpoeschel force-pushed the throw-error-when-steps-not-supported branch from 4f06786 to 5f98403 Compare July 25, 2023 12:21
@franzpoeschel franzpoeschel changed the title Throw error when steps not supported Fixes for variable-based encoding in backends without step support Jul 27, 2023
@franzpoeschel franzpoeschel added bug api: new additions to the API labels Jul 27, 2023
@franzpoeschel franzpoeschel requested review from ax3l and guj July 27, 2023 15:31
Copy link
Contributor

@guj guj left a comment

Choose a reason for hiding this comment

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

Thanks Franz for the updates!

Just curious, what happens READ_LINEAR is applied to an h5/json file?

In terms of allowing any format to write one step and throw error on second steps when writing variable based. It is totally logical. From a client prospective, I would prefer openPMD-api to throw error when someone init series with h5/json + variable based, instead of wait until the second step.
The openPMD side of the code would be simpler too.

@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Aug 2, 2023

Just curious, what happens READ_LINEAR is applied to an h5/json file?

READ_LINEAR can be used with all backends, but it does not do any parsing in the Series constructor. The parsing has to be triggered by Series::readIterations(), or alternatively Series::parseBase() as introduced by this PR.
If the backend uses steps, only the first step will be parsed now (unlike READ_ONLY which will parse the entire file even when it uses steps), otherwise the entire file is parsed.

Especially, using READ_LINEAR and for(auto iteration : series.readIterations()) … is a workflow (the only one) that is supported by all backends.

In terms of allowing any format to write one step and throw error on second steps when writing variable based. It is totally logical. From a client prospective, I would prefer openPMD-api to throw error when someone init series with h5/json + variable based, instead of wait until the second step. The openPMD side of the code would be simpler too.

Writing a single step with variable-based encoding is a useful feature for data that has no iterations. We throw no error here because this is a workflow that we explicitly and intentionally support.

@guj
Copy link
Contributor

guj commented Aug 3, 2023

Just curious, what happens READ_LINEAR is applied to an h5/json file?

READ_LINEAR can be used with all backends, but it does not do any parsing in the Series constructor. The parsing has to be triggered by Series::readIterations(), or alternatively Series::parseBase() as introduced by this PR. If the backend uses steps, only the first step will be parsed now (unlike READ_ONLY which will parse the entire file even when it uses steps), otherwise the entire file is parsed.

Especially, using READ_LINEAR and for(auto iteration : series.readIterations()) … is a workflow (the only one) that is supported by all backends.

In terms of allowing any format to write one step and throw error on second steps when writing variable based. It is totally logical. From a client prospective, I would prefer openPMD-api to throw error when someone init series with h5/json + variable based, instead of wait until the second step. The openPMD side of the code would be simpler too.

Writing a single step with variable-based encoding is a useful feature for data that has no iterations. We throw no error here because this is a workflow that we explicitly and intentionally support.

Thanks for the explanation. Maybe this behavior can be in the docs for future reference.

@franzpoeschel
Copy link
Contributor Author

Maybe this behavior can be in the docs for future reference.

It is documented here

@franzpoeschel franzpoeschel force-pushed the throw-error-when-steps-not-supported branch from 41281a8 to 96f5477 Compare August 3, 2023 15:17
@franzpoeschel franzpoeschel added this to the 0.15.2 milestone Aug 8, 2023
@ax3l ax3l self-assigned this Aug 8, 2023
@franzpoeschel franzpoeschel force-pushed the throw-error-when-steps-not-supported branch from 96f5477 to d8c9b51 Compare August 9, 2023 15:52
@franzpoeschel franzpoeschel force-pushed the throw-error-when-steps-not-supported branch from d8c9b51 to b0e9e0d Compare August 9, 2023 15:57
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you for the report, updates and fixes!! 🚀 ✨

@ax3l ax3l merged commit c25b317 into openPMD:dev Aug 9, 2023
31 checks passed
@ax3l
Copy link
Member

ax3l commented Aug 16, 2023

Ah, we should have squashed these commits for easier backporting :) next time, hope I got it right :)

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

Successfully merging this pull request may close these issues.

variableBased Encoding should be limited to ADIOS
3 participants