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

JSON update #1043

Merged
merged 23 commits into from
Dec 18, 2021
Merged

JSON update #1043

merged 23 commits into from
Dec 18, 2021

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Jul 9, 2021

Fix #1037, see there for a description.
This will probably not address all of the points noted therein, currently we have:

  • capitalization. I favor capitalization-insensitivity, keeping consistent with how ADIOS2 does it.
  • More lenient datatypes in ADIOS2: To users, it seems weird that they should specify adios2.engine.usesteps = true (as a boolean) but adios2.engine.parameters.OpenTimeoutSecs = "30" (as a string).
  • Use TracingJSON everywhere: partially implemented, warnings not yet shown for global options
  • Merge Two suggestions to fix CommonADIOS1IOHandlerImpl #1101 first
  • This PR removes some keys from Dataset: chunks, compression and transform. So far, only transform was even implemented (I've replaced that implementation with a JSON-based one now), and only in ADIOS1 which is legacy anyway. For keeping compatibility with user code, we should maybe keep the fields for now, but deprecate them and remove all implementation surrounding them. Discussed: remove them fully in the API if we change the API contract (implementation) to break only once.
  • Warn for global options: Dataset: unknown options should warn #1095
  • Warning if file ending collides with JSON option
  • Are these options in ADIOS2 capitalization insensitive? If not, I'll need to add some handling for that. Eh, let's not rely on this in ADIOS2. I've added exceptions for the lower case transformation now.

@franzpoeschel
Copy link
Contributor Author

Warning for unused global options should work now, needs a bit more testing though. I configured an ADIOS2 dataset with the following string:

{
  "resizable": true,
  "asdf": "asdf",
  "adios2": {
    "unused": "dataset parameter",
    "dataset": {
      "unused": "too",
      "operators": [
        {
          "type": "blosc",
          "parameters": {
            "clevel": 3,
            "doshuffle": "BLOSC_BITSHUFFLE"
          }
        }
      ]
    },
    "hdf5": {
      "this": "should not warn"
    }
  }
}

Resulting warning:

Warning: parts of the JSON configuration for ADIOS2 dataset '/data/0/meshes/E/y' remain unused:
{"adios2":{"dataset":{"unused":"too"},"unused":"dataset parameter"},"asdf":"asdf"}

There is no warning for the HDF5 dataset as the HDF5 part is not parsed.

@franzpoeschel franzpoeschel force-pushed the topic-json branch 7 times, most recently from b783973 to 7f12065 Compare September 13, 2021 14:31
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Sep 13, 2021

Only thing really still missing for finishing this PR is to introduce JSON configuration for stuff in ADIOS1 that has so far been configured via environment variables. Since this PR is already large enough, I'll suggest to postpone it to a follow-up.
Otherwise, I've split this thing into logical commits now, so reviewing by commit should be doable. I'll also go through things once again myself for a little cleanup, otherwise this is quite ready for review @ax3l :)

PS: What's also missing is to add JSON config to the MPI benchmarks. This PR removes the compression from them and doesn't replace them for now. Added that now

@franzpoeschel
Copy link
Contributor Author

I've done some cleanup now and the CI is now also running fine

@ax3l
Copy link
Member

ax3l commented Sep 15, 2021

I merged #1101 now :)

These are the backends that are already configurable via JSON.
New additions:
1. Use of error::BackendConfigSchema
2. Lower case string value reading where applicable
3. Warning when global Dataset options are unused
4. Lax datatypes for string values
Also, warn on unused global keys
The CI doesn't like having too many escaped characters in a normal
string
JSON params may be specified in arbitrary capitalization. The backend
should recognize this and not read from environment variables if the
parameter has already been set via JSON.
Currently those objects that contain keys forwarded to ADIOS2
Can be extended as needed
include/openPMD/IO/IOTask.hpp Outdated Show resolved Hide resolved
test/CoreTest.cpp Outdated Show resolved Hide resolved
test/CoreTest.cpp Outdated Show resolved Hide resolved
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.

Thanks a lot 🎉 Great work! 🚀 ✨

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.

JSON Update
2 participants