Docs: endian_reverse unclear #1822
Comments
|
@ax3l what's the actual issue? Please, see my explanation in the referenced link. |
|
Basically, if you know you/your users need to deal with endianness, you
need to build adios with this flag everywhere. Then the conversion will
happen automatically and only if needed. It is not a forced flag on all
data.
I don't think this flag would make a big difference in read performance,
but never measured it myself, and it's not always on.
…On Mon, Oct 14, 2019 at 4:29 PM William F Godoy ***@***.***> wrote:
@ax3l <https://github.com/ax3l> what's the actual issue? Please, see my
explanation in the referenced link.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1822?email_source=notifications&email_token=AAYYYLPBSCZKJH65WPR3FDLQOTJDXA5CNFSM4JATIUBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBGMUPA#issuecomment-541903420>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYYYLOBRN75ZKSGF65OHKTQOTJDXANCNFSM4JATIUBA>
.
|
|
Also, there is a lack of testing due to limited access to such configuration in our CI. The feature itself is not fully tested (in CI or nightly), it was just done for one user who kindly provided some tests. |
|
The problem is that endian portability and conversion should be done automatically, not by a user and most certainly not by yet-an-other binary variant of the library. As explained in the description, that makes workflows super complicated. Imagine the following: machine A writes big endinanness to the FS, machine B is small endian and also writes to the FS. Yes, CI is an issue these days :-S |
|
@ax3l nothing is preventing turning this ON if that is appropriate for the system use case.
Not really, we only had one case that required this feature |
|
@ax3l I think this is more of a system admin packaging problem than an adios2 problem. Would you agree? |
|
Well in my opinion, I think the workflow I describe in detail in the PR description and comment is not supported yet and BP is not endian-portable due to it. I think a portable fileformat (BP) or adios engine of any kind should be self-aware of its endianness (just encode it) and then just flip during read properly when required, without interaction from user or sysadmin. I really do not think this should be a compile time option or for the better a runtime option, but an opaque feature of adios data reading/writing. This problem has been solved for a long time by just adding the write endianness to meta-data, e.g. in protobuf, Cap’n Proto, HDF5, et al., and then just runtime determining of one has to flip all reads depending on the read architecture. |
|
I was not aware that the BP file itself is not recording its endianness
properly. I thought only the reader library is affected by the flag. If the
library is compiled with the flag, it should automatically read BP files
from both little and big endian systems.
Isn't that so?
…On Tue, Oct 15, 2019, 5:24 PM Axel Huebl ***@***.***> wrote:
Well in my opinion, I think the workflow I describe in detail in the PR
description is not supported yet and BP is not portable due to it.
I think a portable fileformat (BP) or adios engine of any kind should be
self-aware of its endianness (just encode it) and then just flip during
read properly when required, without interaction from user or sysadmin. I
really do not think this should be a compile time option or even a runtime
option, but a feature of adios data reading/writing.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1822?email_source=notifications&email_token=AAYYYLMYWC3EH25IUYE7LR3QOYYH3A5CNFSM4JATIUBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBKITCQ#issuecomment-542411146>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYYYLOIZZLG72XTFKOBA63QOYYH3ANCNFSM4JATIUBA>
.
|
|
@ax3l OK, thanks for your input. I see the confusion, and I second @pnorbert response. Any BP file (regardless of compilation options) is aware of its endianness. The decision workflow should be: do we ever going to read big and little endian BP files on our target system? If yes, then turn it on, but knowing that we don't test on CI or nightly, if no (the majority of our users) then do nothing. |
|
Ohh, I see. 💡 This is just enabling potential runtime endian-transforms, yet only when they are necessary! I think there is a new doc string in 2.4.0+ as well that says: Well, all of that makes a lot of sense now. The short spack variant string Mea cupla and thanks for the clarification. Will update the spack variant description. |
|
Shall we generally update the string on that option to something like |
|
@ax3l no problem, improving the documentation is always welcome. To be fair, I used boost's endian_reverse terminology, but feel free to make the description more clear. |
|
Also, the functionality in the code is based on calls to std::reverse, but this function is more general than for just endianness. |
|
Hehe, sorry again for confusing this. I should have just immediately grep-ed in the source for what it is doing. |
Migrated from here so it does not get lost.
The CMake flag
ADIOS2_USE_ENDIAN_REVERSE=ONlooks scary to me for the following reason: if a file was written in one endinaness and read again in another, the BP file format should encode that and our BPEngine could automatically take care of it. (E.g. HDF5 does it that way).I was working/interning a few years ago in Juelich where serveral big and little endian machines were connected to the same file system and workflows going back and forth were common. Using various adios modules, depending on the origin of the data, would be very complicated.
If it's not possible to already express this in BP itself, which I think it should be for portability, then we should at least make this a runtime option for now.
The text was updated successfully, but these errors were encountered: