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

Read backend configuration from .toml files #1146

Merged
merged 11 commits into from Jan 11, 2022
Merged

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Nov 11, 2021

Approach for implementation: Read from .toml file, but convert to nlohmann::json internally

Config files are much more readable in TOML:

unused = "global parameter"

[hdf5]
unused = "hdf5 parameter please dont warn"

[adios2]
unused = "parameter"

[adios2.engine]
type = "bp3"
unused = "as well"

[adios2.engine.parameters]
BufferGrowthFactor = 2
Profile = "On"

# double brackets, because the operators are a list
[[adios2.dataset.operators]]
type = "blosc"

# parameters for the current list entry
[adios2.dataset.operators.parameters]
parameters.clevel = "1"
parameters.doshuffle = "BLOSC_BITSHUFFLE"

TODO:

@franzpoeschel franzpoeschel added the api: new additions to the API label Nov 11, 2021
@franzpoeschel franzpoeschel force-pushed the topic-toml branch 3 times, most recently from f2348aa to 462d83d Compare November 11, 2021 13:42
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Nov 12, 2021

When I leave out the filehandle.good() check on Windows, I get this error:

C:\Users\Franz\git-repos\openPMD-api\test\SerialIOTest.cpp(3352): FAILED:
due to unexpected exception with message:
string too long 

I'll have to try using a standalone toml11 example to see if there is a bug on Windows.

@ax3l
Copy link
Member

ax3l commented Dec 1, 2021

Rebased after #1148 was merged :)

@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Dec 9, 2021

Rebased after #1148 was merged :)

We'll have to watch out when rebasing as soon as #1043 is merged, since this rebase duplicated the commits of that PR

@franzpoeschel
Copy link
Contributor Author

Rebased and up for review :) @ax3l

@ax3l
Copy link
Member

ax3l commented Jan 6, 2022

Thank you, the code looks great to me! ✨

In order to finalize the PR for merge, can you please:

  • add in the manual according sections (the same way as we document user-facing JSON options right now)
  • add a user-facing example case

? :)

@franzpoeschel
Copy link
Contributor Author

Thank you, the code looks great to me! sparkles

In order to finalize the PR for merge, can you please:

* add in the manual according sections (the same way as we document user-facing JSON options right now)

* add a user-facing example case

? :)

Still in jetlag? :D

Yep, those are good suggestions. I'll try to work the TOML configuration into the docs (since both are equivalent, I think I'll not duplicate everything, but have some examples for TOML here and there), also I think for user-facing examples you mean something under the /examples folder? If yes, yep I can add something there.

@franzpoeschel
Copy link
Contributor Author

I've now added documentation and a new example based on the streaming examples.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jan 7, 2022

This pull request introduces 1 alert when merging 94364d9 into 05eff02 - view on LGTM.com

new alerts:

  • 1 for Unused import

@franzpoeschel franzpoeschel force-pushed the topic-toml branch 3 times, most recently from 55f7845 to 1609dd7 Compare January 10, 2022 14:06
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.

Looks great, minor final updates inline :)

Also a small collision from #1162 in src/IO/ADIOS/CommonADIOS1IOHandler.cpp now.

docs/source/details/backendconfig.rst Outdated Show resolved Hide resolved
examples/13_write_dynamic_configuration.cpp Outdated Show resolved Hide resolved
examples/13_write_dynamic_configuration.cpp Outdated Show resolved Hide resolved
examples/13_write_dynamic_configuration.py Outdated Show resolved Hide resolved
examples/13_write_dynamic_configuration.py Outdated Show resolved Hide resolved
franzpoeschel and others added 5 commits January 11, 2022 17:21
This way we don't confuse users when they use TOML but get a JSON-based
error message.
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
@franzpoeschel
Copy link
Contributor Author

Also a small collision from #1162 in src/IO/ADIOS/CommonADIOS1IOHandler.cpp now.

rebased

@ax3l ax3l merged commit d13c9c8 into openPMD:dev Jan 11, 2022
@ax3l ax3l mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: new additions to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants