-
Notifications
You must be signed in to change notification settings - Fork 51
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
TOML Backend #1436
TOML Backend #1436
Conversation
We will not be able to activate TOML for our broad test suite as TOML serialization and deserialization are both quite slow. The following example is the With TOML backend activated, most time is spent parsing ~1000 TOML files: |
e3b9179
to
ad4183c
Compare
a001ac8
to
bc141aa
Compare
8ef584e
to
620cf53
Compare
620cf53
to
25a57de
Compare
922f674
to
3d0c786
Compare
3d0c786
to
09012fd
Compare
09012fd
to
fe4e835
Compare
TOML is not shown as available on NVIDIA compilers
fe4e835
to
48af319
Compare
} | ||
#if defined(__INTEL_COMPILER) | ||
/* | ||
* ICPC has trouble with if constexpr, thinking that return statements are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is likely with EDG frontents in general and might also affect nvcc. I remember we saw this there as well and reported it at some point (should be fixed in newer >CUDA 12 versions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we should change the macro to something more precise?
If yes, do you know what would be the right check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was mumbling here and kept this for future reference, in case we see a warning from it. Nothing to do now.
break; | ||
} | ||
// TOML does not support nulls, so initialize with zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting discussion: toml-lang/toml#30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the central bit of that discussion is "TOML is intended for configuration", which is exactly the use case for which we are adding the TOML backen.
If users insist on writing entire datasets with TOML, that's fine too, but it will be initialized with 0.
Otherwise, TOML is mostly intended for usage in conjunction with the follow-up #1493 which only writes the dataset metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that makes total sense. Should we be a bit more explicit in our guidance in docs/source/backends/json.rst
to avoid that users mistake it as a full-fledged, high-performance data backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add something. Also, in #1493 we could think about enabling the abbreviated modes by default in TOML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea about the default with #1493, yes! :)
Co-authored-by: Axel Huebl <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestions on motivation.
Generalizing a bit and clarifying.
Co-authored-by: Axel Huebl <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you! 🎉
* dev: Fix CMake: HDF5 Libs are PUBLIC (openPMD#1520) Fix `chmod` in `download_samples.sh` (openPMD#1518) CI: Old CTest (openPMD#1519) Python: Fix ODR Violation (openPMD#1521) replace extent in weighting and displacement (openPMD#1510) CMake: Warn and Continue on Empty HDF5_VERSION (openPMD#1512) Replace openPMD_Datatypes global with function (openPMD#1509) Streaming examples: Set WAN as default transport (openPMD#1511) TOML Backend (openPMD#1436) make it possible to manually set chunks when loading dask arrays (openPMD#1477) [pre-commit.ci] pre-commit autoupdate (openPMD#1504) Optional debugging output for AbstractIOHandlerImpl::flush() (openPMD#1495) Python: 3.8+ (openPMD#1502) # Conflicts: # .github/workflows/linux.yml # src/binding/python/Series.cpp
Extracted from #1277, since that PR contains thematically related, but clearly distinct items.
TODO: