allowModification option of DefineAttribute: does not give the correct values at read time

[franzpoeschel]

Describe the bug I'm currently trying to use the allowModification option of DefineAttribute, recently introduced into ADIOS2 by @pnorbert if I recall correctly. bpls -alt correctly outputs the changing attribute across steps. Using InquireAttribute however will only ever yield the most recent value. Observed in BP4.

To Reproduce Use the attached dataset: variableBasedSeries.bp.zip

With bpls:

> bpls ../samples/variableBasedSeries.bp/ -lta -e '.*/data'
Step 0:
  uint64_t  __openPMD_groups/data                      attr   = 0
Step 1:
  uint64_t  __openPMD_groups/data                      attr   = 1
Step 2:
  uint64_t  __openPMD_groups/data                      attr   = 2
Step 3:
  uint64_t  __openPMD_groups/data                      attr   = 3
Step 4:
  uint64_t  __openPMD_groups/data                      attr   = 4
Step 5:
  uint64_t  __openPMD_groups/data                      attr   = 5
Step 6:
  uint64_t  __openPMD_groups/data                      attr   = 6
Step 7:
  uint64_t  __openPMD_groups/data                      attr   = 7
Step 8:
  uint64_t  __openPMD_groups/data                      attr   = 8
Step 9:
  uint64_t  __openPMD_groups/data                      attr   = 9

With the C++ API:

#include <adios2.h>
#include <iostream>
#include <string>
#include <vector>

int main(int argsc, char **argsv)
{
    auto file = argsv[1];

    adios2::ADIOS adios;
    adios2::IO IO = adios.DeclareIO("IO");
    adios2::Engine engine = IO.Open(file, adios2::Mode::Read);

    size_t step = 0;
    while (engine.BeginStep() == adios2::StepStatus::OK)
    {
        auto attribute =
            IO.InquireAttribute<unsigned long long>("__openPMD_groups/data");
        std::cout << "Value in step " << step << ": " << attribute.Data()[0]
                  << std::endl;
        engine.EndStep();
        ++step;
    }
    engine.Close();
}

> ./steps_read ../../samples/variableBasedSeries.bp/
Value in step 0: 9
Value in step 1: 9
Value in step 2: 9
Value in step 3: 9
Value in step 4: 9
Value in step 5: 9
Value in step 6: 9
Value in step 7: 9
Value in step 8: 9
Value in step 9: 9

Expected behavior In the C++ API, the attribute value should be associated to the corresponding step, like seen with bpls.

Desktop (please complete the following information):

• OS/Platform: nvidia/cuda:11.6.0-devel-ubuntu20.04 Singularity container on a Debian bullseye/sid machine
• Build [e.g. compiler version gcc 7.4.0, cmake version, build type: static ]: ADIOS 2.8.2, built with g++ 11.1.0 in Debug mode

Additional context It's ok for me if an eventual bugfix of this will stay on the master branch for a while, I don't need this in a release very soon.

Following up

[pnorbert]

confirmed

[franzpoeschel]

Note to self: Check this in BP5 with step-by-step mode also test with StreamReader option in BP4

[ax3l]

We will need this in WarpX to be able to read back-transformed diagnostics data when only part of the lab frame/station is written, e.g., if we abort a job or if it crashes.

[franzpoeschel]

Note to self: Check this in BP5 with step-by-step mode, also test with StreamReader option in BP4

Both options work. I think this resolves the issue for our use cases, since we can specify the correct options inside our ADIOS2 implementation as long as we need this feature.

[franzpoeschel]

Ok, I'll need to reopen this. This is now working in BP4 and BP5, but not in SST (I believe due to the BP3-based serialization?).

The error is thrown not by the writer, but the reader:

> ./stream_read 
Attribute: asdf
terminate called after throwing an instance of 'std::invalid_argument'
  what():  [Tue Aug 16 12:23:23 2022] [ADIOS2 EXCEPTION] <Core> <Attribute> <Modify> : Attribute asdf being modified is not modifiable

Aborted (core dumped)

Corresponding code:

Writer:

#include <adios2.h>
#include <numeric>
#include <vector>

int main(int argsc, char **argsv)
{
    std::string engine_type = "sst";

    adios2::ADIOS adios;
    adios2::IO IO = adios.DeclareIO("IO");
    IO.SetEngine(engine_type);
    adios2::Engine engine = IO.Open("stream", adios2::Mode::Write);
    auto var = IO.DefineVariable<int>("var");

    for (unsigned step = 0; step < 10; ++step)
    {
        engine.BeginStep();
        engine.Put(var, 17);
        IO.DefineAttribute("asdf", step, "", "/", true);
        engine.EndStep();
    }
    engine.Close();
}

Reader:

#include <adios2.h>
#include <iostream>
#include <string>
#include <vector>

int main(int argsc, char **argsv)
{
    std::string engine_type = "sst";

    adios2::ADIOS adios;
    adios2::IO IO = adios.DeclareIO("IO");
    IO.SetEngine(engine_type);
    adios2::Engine engine = IO.Open("stream", adios2::Mode::Read);

    while (engine.BeginStep() == adios2::StepStatus::OK)
    {
        for (auto const & attr : IO.AvailableAttributes())
        {
            std::cout << "Attribute: " << attr.first << std::endl;
        }
        engine.EndStep();
    }
    engine.Close();
}

Exception backtrace:

#0  0x00007fa2740b38b2 in __cxa_throw () from /lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x00007fa27444a92f in adios2::helper::Throw<std::invalid_argument> (component="Core", source="Attribute", activity="Modify", message="Attribute asdf being modified is not modifiable", commRank=-1) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/helper/adiosLog.h:83
#2  0x00007fa27354a671 in adios2::core::Attribute<unsigned int>::Modify (this=0x563e69c0c2d0, data=@0x7fffd4f73470: 1) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/core/Attribute.tcc:127
#3  0x00007fa2735e34d4 in adios2::core::IO::DefineAttribute<unsigned int> (this=0x563e69be3510, name="asdf", value=@0x7fffd4f73470: 1, variableName="", separator="/", allowModification=false) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/core/IO.tcc:137
#4  0x00007fa27387b90c in adios2::format::BP3Deserializer::DefineAttributeInEngineIO<unsigned int> (this=0x563e69c0ce60, header=..., engine=..., buffer=std::vector of length 283, capacity 283 = {...}, position=227) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/toolkit/format/bp/bp3/BP3Deserializer.tcc:975
#5  0x00007fa2738259ba in operator() (__closure=0x7fffd4f73688, engine=..., buffer=std::vector of length 283, capacity 283 = {...}, position=161) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/toolkit/format/bp/bp3/BP3Deserializer.cpp:307
#6  0x00007fa273825cdd in adios2::format::BP3Deserializer::ParseAttributesIndex (this=0x563e69c0ce60, bufferSTL=..., engine=...) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/toolkit/format/bp/bp3/BP3Deserializer.cpp:342
#7  0x00007fa273823e52 in adios2::format::BP3Deserializer::ParseMetadata (this=0x563e69c0ce60, bufferSTL=..., engine=...) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/toolkit/format/bp/bp3/BP3Deserializer.cpp:46
#8  0x00007fa273a5583b in adios2::core::engine::SstReader::BeginStep (this=0x563e69be36e0, Mode=adios2::StepMode::Read,timeout_sec=-1) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/engine/sst/SstReader.cpp:425
#9  0x00007fa2735736d4 in adios2::core::Engine::BeginStep (this=0x563e69be36e0) at /home/franzpoeschel/git-repos/ADIOS2/source/adios2/core/Engine.cpp:43
#10 0x00007fa274458981 in adios2::Engine::BeginStep (this=0x7fffd4f73948) at /home/franzpoeschel/git-repos/ADIOS2/bindings/CXX11/adios2/cxx11/Engine.cpp:52
#11 0x0000563e698518c1 in main ()

Based on the backtrace, it looks like this could be fixed by hardcoding allowModification=true in the DefineAttribute call above (given that this is only the read-end), but I don't know if that would have other unwanted implications elsewhere.

[franzpoeschel]

I tested this out now, and the workaround that I suggested in the previous comment seems to work for my purposes, i.e. inside source/adios2/toolkit/format/bp/bp3/BP3Deserializer.tcc:

template <class T>
void BP3Deserializer::DefineAttributeInEngineIO(
    const ElementIndexHeader &header, core::Engine &engine,
    const std::vector<char> &buffer, size_t position) const
{
    const Characteristics<T> characteristics =
        ReadElementIndexCharacteristics<T>(
            buffer, position, static_cast<DataTypes>(header.DataType), false,
            m_Minifooter.IsLittleEndian);

    std::string attributeName(header.Name);
    if (!header.Path.empty())
    {
        attributeName = header.Path + PathSeparator + header.Name;
    }

    if (characteristics.Statistics.IsValue)
    {
        engine.m_IO.DefineAttribute<T>(
            attributeName, characteristics.Statistics.Value, "", "/", true);   // changed this line
    }
    else
    {
        engine.m_IO.DefineAttribute<T>(
            attributeName, characteristics.Statistics.Values.data(),
            characteristics.Statistics.Values.size(), "", "/", true);          // changed this line
    }
}

[franzpoeschel]

This looks to be fixed with #3318? At least I don't see the error any longer on master.

[eisenhauer]

Fixed with SST and BP5 file engines. Not fixed with BP3 and BP4 files. Not sure if we've officially said that they don't support modifiable attributes or not...

[franzpoeschel]

With BP4 files, modifiable attributes are possible in my tests, but require the StreamReader option. For our purposes that's good enough since we can set that option inside our ADIOS2 implementation. BP3 we don't really care about, we support it for legacy datasets mostly.