Skip to content

Conversation

@franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Oct 10, 2019

This PR adds backend-specific configuration options. Close #150. The configuration is passed as an optional string parameter in JSON format to the constructor of openPMD::Series.
The top-level JSON maps backends to the corresponding backend's configuration. This allows to configure several backends at once.

At the moment, only the ADIOS2 backend has been made aware of this. A possible configuration looks like this:

std::string options = R"(
{
  "adios2": {
    "engine": {
      "type": "sst",
      "parameters": ["BufferGrowthFactor": "2.0", "QueueLimit": "2"]
    }
  }
}
)";

TODO:

  • Safety measure: Warn upon reading an unexpected key.
  • Add Python bindings
  • Code cleanup
  • Add user documentation, e.g. in docs/source/backends/adios2.rst

Probably follow-up for a later PR:

  • Implement the suggestions in issue Backend- and dataset-specific configuration (e.g. compression) #688
  • Further configuration options in ADIOS2, e.g. switching on and off debug mode at runtime.
  • Implement option reading in other backends
  • What about case insensitivity? Should the JSON keys that are read by the openPMD API and not directly passed through to the backend (such as the ADIOS2 "parameters" list) be made case insensitive?
  • Override choice of backend from JSON. Choosing the backend by filename extension is simple, yet inflexible if one backend has several possible extensions (ADIOS2) or two backens share an extension (ADIOS in versions 1 and 2). Just overwrite setting introduced in ADIOS2: Infrastructure & Docs Update #568

@ax3l ax3l added the api: new additions to the API label Oct 10, 2019
@ax3l ax3l self-requested a review October 10, 2019 21:22
@ax3l ax3l mentioned this pull request Oct 15, 2019
3 tasks
@ax3l
Copy link
Member

ax3l commented Oct 15, 2019

I updated the CI with further ADIOS2 tests in #568.
Please rebase against the current dev :)

*
* @param name String of the pattern for file names. Must include iteration regex <CODE>\%T</CODE> for fileBased data.
* @param name String of the pattern for file names. Must include
* iteration regex <CODE>\%T</CODE> for fileBased data.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe align this to beginning of description?


OPENPMD_private:
struct ParsedInput;
OPENPMD_private : struct ParsedInput;
Copy link
Member

@ax3l ax3l Oct 15, 2019

Choose a reason for hiding this comment

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

this syntax confuses me, probably an auto-formatting issue?

@ax3l ax3l force-pushed the topic-advanced-backend-configuration branch from 5dab8da to 2765f82 Compare October 17, 2019 22:42
@ax3l
Copy link
Member

ax3l commented Oct 17, 2019

@franzpoeschel I rebased this for you.

@ax3l
Copy link
Member

ax3l commented Oct 17, 2019

We should decide if we want to make <nlohmann/json.hpp> a non-optional dependency, which looks potentially solid enough to me to make this call, or if we want to cleanly error out if a user tries to pass runtime options without having JSON available.

I will commit a patch for the latter for now, but it's not pretty.

@ax3l
Copy link
Member

ax3l commented Oct 18, 2019

I implemented the selection of the preferred BP backend via an environment variable here:

openPMD-api/src/Series.cpp

Lines 868 to 887 in 731a407

if( auxiliary::ends_with(filename, ".bp") )
{
auto const bp_backend = auxiliary::getEnvString(
"OPENPMD_BP_BACKEND",
#if openPMD_HAVE_ADIOS2
"ADIOS2"
#else
"ADIOS1"
#endif
);
if( bp_backend == "ADIOS2" )
return Format::ADIOS2;
else if( bp_backend == "ADIOS1" )
return Format::ADIOS1;
else
throw std::runtime_error(
"Environment variable OPENPMD_BP_BACKEND for .bp backend is neither ADIOS1 nor ADIOS2: " + bp_backend
);
}

We can also overwrite that by a runtime option, like the other options you added.

Adding control to HDF5 for collective/independent IO, via H5FD_MPIO_INDEPENDENT, to address #554 would be a good thing to implement right away as well.

@ax3l
Copy link
Member

ax3l commented Oct 27, 2019

ping @franzpoeschel any thoughts on this? :)
Would like to make this available for the next release, so I can address the HDF5 control for independent I/O via a runtime API option as well.

@ax3l ax3l force-pushed the topic-advanced-backend-configuration branch from da5ab03 to a98a695 Compare November 5, 2019 00:55
@franzpoeschel
Copy link
Contributor Author

I'm fine with controlling the .bp backend with an environment variable. I think that we should add a parameter to the JSON string to explicitly choose backends that takes precedence over other options.
Making the other backends aware of this configuration style would be good (as you suggested for HDF5), I just don't have the time at the moment.
I think there is no issue with making <nlohmann/json.jpp> a non-optional dependency? It is currently not included in user-facing code, and this would also mean that the API will compile with at least one usable backend, simplifying accessibility.

@ax3l
Copy link
Member

ax3l commented Nov 7, 2019

I'm fine with controlling the .bp backend with an environment variable. I think that we should add a parameter to the JSON string to explicitly choose backends that takes precedence over other options.

Agreed, the env var was a quick hack of mine but I want users to be able to control it via an option, which is more friendly and which is why I try to merge this PR soon :)

Making the other backends aware of this configuration style would be good (as you suggested for HDF5), I just don't have the time at the moment.

Argh, too bad. Can you potentially finish the ADIOS impl. with it and I take a look at the other backends?

I think there is no issue with making <nlohmann/json.jpp> a non-optional dependency.

Excellent, agreed. Will do that in an independent PR then (#587).

Edit: branch updated and rebased so you can finish the ADIOS2 controls.

@ax3l ax3l mentioned this pull request Nov 7, 2019
@ax3l ax3l force-pushed the topic-advanced-backend-configuration branch from a98a695 to 9d600db Compare November 8, 2019 01:19
@ax3l ax3l changed the title Advanced backend configuration via JSON [WIP] Advanced backend configuration via JSON Nov 8, 2019
@ax3l ax3l force-pushed the topic-advanced-backend-configuration branch from 9d600db to c0aad60 Compare November 8, 2019 06:55
@ax3l
Copy link
Member

ax3l commented Jan 10, 2020

ping @franzpoeschel finishing this one would also be really nice, please :)

@franzpoeschel
Copy link
Contributor Author

My plan is to have topic-streaming go on top of this one, so this is part of the endeavours ;)

@franzpoeschel franzpoeschel force-pushed the topic-advanced-backend-configuration branch from c0aad60 to a98a695 Compare January 10, 2020 15:31
@ax3l
Copy link
Member

ax3l commented Jan 10, 2020

Sounds great, I already made the JSON backend required so you just need to rebase to continue.

@franzpoeschel franzpoeschel force-pushed the topic-advanced-backend-configuration branch from a98a695 to 8be39c7 Compare January 13, 2020 13:06
@ax3l
Copy link
Member

ax3l commented Jan 24, 2020

@franzpoeschel just since I need this more urgently for HDF5 option control and compression support: when do you think you can continue to work on this?

Remove traces of openPMD_HAVE_JSON
Improve JSON highlighting in documentation
Formatting improvements in documentation
Includes reordering
Documentation
@franzpoeschel franzpoeschel force-pushed the topic-advanced-backend-configuration branch from faf4a66 to 422143b Compare April 2, 2020 14:38
Configuration Structure per Backend
-----------------------------------

ADIOS2
Copy link
Member

Choose a reason for hiding this comment

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

Generally, and I think this can be a follow-up PR as well, I think these backend-specific options should go into the respective backend docs page (where we also document the current environment vars).

Copy link
Member

Choose a reason for hiding this comment

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

@franzpoeschel please try to split this up in a follow-up PR, we already have backend-specific docs where this should go :)

@franzpoeschel franzpoeschel force-pushed the topic-advanced-backend-configuration branch from e0f324f to c20eb42 Compare April 4, 2020 13:38
@franzpoeschel franzpoeschel force-pushed the topic-advanced-backend-configuration branch from c20eb42 to 00768b9 Compare April 4, 2020 14:05
@ax3l ax3l merged commit ad984b5 into openPMD:dev Apr 8, 2020
@franzpoeschel franzpoeschel deleted the topic-advanced-backend-configuration branch January 28, 2021 13:26
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.

Passing of advanced backend parameters

3 participants