BP5: RemoveAttribute() leaves traces of the removed attributes, redefinition with changed type impossible

[franzpoeschel]

Describe the bug
Our workflow is to define default attributes that are automatically set, but can also be specified manually by users. This might lead to a situation where an attribute has already been defined, but must later in the same step be changed again to adhere to user input.
We then remove the old attribute, and define another attribute with the same name within the same IO step.
With BP5, this is fundamentally possible, but leads to errors when the attribute is redefined with another type.

To Reproduce

#include <adios2.h>

int main(int argsc, char **argsv)
{
    adios2::ADIOS adios;
    auto IO = adios.DeclareIO("asdf");
    IO.SetEngine("bp5");
    auto engine = IO.Open("data.bp", adios2::Mode::Write);

    engine.BeginStep();
    IO.DefineAttribute<float>("changing_datatype", 5.7);
    engine.PerformPuts();
    IO.RemoveAttribute("changing_datatype");
    IO.DefineAttribute<double>("changing_datatype", 5.8);
    engine.EndStep();
    engine.Close();
}

This creates a file data.bp which cannot be opened:

$ bpls data.bp
Failed to open with BPFile engine: [Tue Dec 20 13:25:28 2022] [ADIOS2 EXCEPTION] <Core> <IO> <DefineAttribute> : modifiable attribute changing_datatype has been defined with type float. Type cannot be changed to double

Failed to open with HDF5 engine: [Tue Dec 20 13:25:28 2022] [ADIOS2 EXCEPTION] <Engine> <HDF5ReaderP> <HDF5ReaderP> : Invalid HDF5 file found


Error: Could not open this file with any ADIOS2 file reading engines

data.bp.zip

Expected behavior
Expected behavior can be seen when setting IO.SetEngine("bp4") in the above code:

$ bpls -al data.bp/
  double   changing_datatype  attr   = 5.8

Desktop (please complete the following information):

• OS/Platform: Ubuntu 22.04
• Build: G++ 11.3.0, ADIOS2 master tag 9c9e4ca

Additional context

• It might be better if we specified our defaults at a later point so that we avoid having to delete/override attributes as much as possible
• First observed here Radiation plugin: Workaround BP5 bug ComputationalRadiationPhysics/picongpu#4399

Following up
Was the issue fixed? Please report back.

[eisenhauer]

This probably requires higher-level conceptual discussion. ADIOS doesn't allow type changes in variables from step to step and the API essentially relies upon this property (particularly for the random access mode where you can inquire a type-specific variable and then select what step you want to read). It doesn't (yet) have a similar interface for attributes, simply because attributes were previously unchangeable. So, how attributes behave in random access mode vs. streaming mode is ill-defined. It's also the case that most of BP5's performance improvements over BP4 are due to exploiting things that don't change from timestep to timestep. The more dynamism we allow, the more potential to compromise those performance gains. IMHO, I'd probably come down on the side of not supporting type dynamism in attributes (and throwing an appropriate error), but I'm interested in hearing other opinions.

[franzpoeschel]

RemoveAttribute() is only supported inside a step anyway if I remember correctly. Inside our implementation (even for BP4), we explicitly check that an attribute has only been defined in the current step before we even go on to overwrite an attribute.
The use case that I'm asking about in this issue is no actual type-dynamism in the dataset itself, but the option to "change one's mind" while the step is still going on. This should not affect the BP5 file format at all (because I'm not asking for a feature that it does not yet cover), but it might of course affect the implementation in ADIOS2, so I don't really know if this feature is feasible.

And, as I've written in the "Additional context" part, the cleanest solution would be to avoid overwriting attributes, which is on us to fix.

[eisenhauer]

Yikes! I didn't realize RemoveAttribute() was even in the external API. "Supported" might be a strong word in this context. I put it more in the "I can think of situations where this might work as one might expect and not blow things up". Adding an attribute and removing it again before anything happens that might make ADIOS look at it is one of those. It looks like that's what is tested, as well as define-remove-define, but this seems like a case where the particular API's potential for abuse outweighs its actual utility.

[franzpoeschel]

So, you are saying that this is no bug?
This situation is luckily obscure enough that we will be able to work around it in the seldom case that it arises, until we find a better workflow on our side.

[eisenhauer]

It's more that the semantics behind attributes are a lot fuzzier than I (at least) would like... This particularly applies to changing attributes because there are likely things that depend upon them not changing (or at least behaves in a way that might be unexpected or ill-defined if they do. So, yeah, IMHO this falls into a corner case and it might be best to avoid it if you can.

[franzpoeschel]

Alright, then we'll rather figure out a solution on our side.