From 84f71c9451d21701d7d0df2ac54da6b5a6f12ef5 Mon Sep 17 00:00:00 2001 From: Norbert Podhorszki Date: Tue, 17 Aug 2021 10:44:02 -0400 Subject: [PATCH 1/4] Allow redefining attributes. BP4 stream reading will provide the latest definition at a given step. BP4 file reading provides the last definition even if BeginStep/EndStep is attempted. BP3 does not support changing attributes, it always presents the first definition of the attribute (although, in the encoded data blocks newer attributes appear but the metadata does not change). Caveat: Modifying an attribute on non-zero rank process will have no effect as the change is not registered on the metadata aggregator. $ ctest -R Engine.BP.BPWriteReadAttributes.WriteReadStreamMutable.BP4.Serial Test project /home/adios/ADIOS2/build.debug Start 258: Engine.BP.BPWriteReadAttributes.WriteReadStreamMutable.BP4.Serial 1/1 Test #258: Engine.BP.BPWriteReadAttributes.WriteReadStreamMutable.BP4.Serial ... Passed 0.01 sec Stream reading: $ ./bin/bpls -lta testing/adios2/engine/bp/bp4/foo/AttributesWriteReadMutable.bp/ Step 0: int32_t var1 scalar double var1\dArray attr = {0.1, 0.2, 0.3} uint32_t var1\i32Value attr = 0 int32_t var2 {8} double var2\dArray attr = {0.1, 0.2, 0.3} uint32_t var2\i32Value attr = 0 Step 1: int32_t var1 scalar double var1\dArray attr = {1.1, 1.2, 1.3} -- modified value of attribute uint32_t var1\i32Value attr = 1 -- modified value of attribute double var2\dArray attr = {0.1, 0.2, 0.3} -- attribute was not modified so last value appears uint32_t var2\i32Value attr = 0 -- attribute was not modified so last value appears Step 2: int32_t var1 scalar double var1\dArray attr = {2.1, 2.2, 2.3} uint32_t var1\i32Value attr = 2 int32_t var2 {8} double var2\dArray attr = {2.1, 2.2, 2.3} uint32_t var2\i32Value attr = 2 File reading: $ ./bin/bpls -lA testing/adios2/engine/bp/bp4/foo/AttributesWriteReadMutable.bp/ double var1\dArray attr = {2.1, 2.2, 2.3} -- last definition appears uint32_t var1\i32Value attr = 2 double var2\dArray attr = {2.1, 2.2, 2.3} uint32_t var2\i32Value attr = 2 --- source/adios2/core/Engine.cpp | 2 + source/adios2/core/Engine.h | 5 + source/adios2/core/IO.tcc | 64 ++++--- source/adios2/engine/bp4/BP4Writer.cpp | 5 + source/adios2/engine/bp4/BP4Writer.h | 2 + .../engine/bp/TestBPWriteReadAttributes.cpp | 176 +++++++++++++++++- 6 files changed, 228 insertions(+), 26 deletions(-) diff --git a/source/adios2/core/Engine.cpp b/source/adios2/core/Engine.cpp index ca025fa9b6..d7da3d8f2e 100644 --- a/source/adios2/core/Engine.cpp +++ b/source/adios2/core/Engine.cpp @@ -100,6 +100,8 @@ void Engine::Init() {} void Engine::InitParameters() {} void Engine::InitTransports() {} +void Engine::NotifyEngineAttribute(std::string name, DataType type) noexcept {} + // DoPut* #define declare_type(T) \ void Engine::DoPut(Variable &, typename Variable::Span &, \ diff --git a/source/adios2/core/Engine.h b/source/adios2/core/Engine.h index 1e5654ae3e..9ae5ba06dd 100644 --- a/source/adios2/core/Engine.h +++ b/source/adios2/core/Engine.h @@ -495,6 +495,11 @@ class Engine return nullptr; } + /** Notify the engine when a new attribute is defined. Called from IO.tcc + */ + virtual void NotifyEngineAttribute(std::string name, + DataType type) noexcept; + protected: /** from ADIOS class passed to Engine created with Open * if no communicator is passed */ diff --git a/source/adios2/core/IO.tcc b/source/adios2/core/IO.tcc index 6d8adf62c8..3404f61e50 100644 --- a/source/adios2/core/IO.tcc +++ b/source/adios2/core/IO.tcc @@ -23,6 +23,7 @@ #include "adios2/helper/adiosFunctions.h" #include "adios2/helper/adiosType.h" #include +#include namespace adios2 { @@ -117,25 +118,31 @@ Attribute &IO::DefineAttribute(const std::string &name, const T &value, auto itExistingAttribute = m_Attributes.find(globalName); if (itExistingAttribute != m_Attributes.end()) { - if (helper::ValueToString(value) == + if (helper::ValueToString(value) != itExistingAttribute->second->GetInfo()["Value"]) { - return static_cast &>(*itExistingAttribute->second); + itExistingAttribute->second = std::unique_ptr( + new Attribute(globalName, value)); + for (auto &e : m_Engines) + { + e.second->NotifyEngineAttribute( + globalName, itExistingAttribute->second->m_Type); + } } - else + return static_cast &>(*itExistingAttribute->second); + } + else + { + auto itAttributePair = m_Attributes.emplace( + globalName, std::unique_ptr( + new Attribute(globalName, value))); + for (auto &e : m_Engines) { - throw std::invalid_argument( - "ERROR: attribute " + globalName + - " has been defined and its value cannot be changed, in call to " - "DefineAttribute\n"); + e.second->NotifyEngineAttribute( + globalName, itAttributePair.first->second->m_Type); } + return static_cast &>(*itAttributePair.first->second); } - - auto itAttributePair = m_Attributes.emplace( - globalName, - std::unique_ptr(new Attribute(globalName, value))); - - return static_cast &>(*itAttributePair.first->second); } template @@ -165,23 +172,30 @@ Attribute &IO::DefineAttribute(const std::string &name, const T *array, helper::VectorToCSV(std::vector(array, array + elements)) + " }"); - if (itExistingAttribute->second->GetInfo()["Value"] == arrayValues) + if (itExistingAttribute->second->GetInfo()["Value"] != arrayValues) { - return static_cast &>(*itExistingAttribute->second); + itExistingAttribute->second = std::unique_ptr( + new Attribute(globalName, array, elements)); + for (auto &e : m_Engines) + { + e.second->NotifyEngineAttribute( + globalName, itExistingAttribute->second->m_Type); + } } - else + return static_cast &>(*itExistingAttribute->second); + } + else + { + auto itAttributePair = m_Attributes.emplace( + globalName, std::unique_ptr( + new Attribute(globalName, array, elements))); + for (auto &e : m_Engines) { - throw std::invalid_argument( - "ERROR: attribute " + globalName + - " has been defined and its value cannot be changed, in call to " - "DefineAttribute\n"); + e.second->NotifyEngineAttribute( + globalName, itAttributePair.first->second->m_Type); } + return static_cast &>(*itAttributePair.first->second); } - - auto itAttributePair = m_Attributes.emplace( - globalName, std::unique_ptr( - new Attribute(globalName, array, elements))); - return static_cast &>(*itAttributePair.first->second); } template diff --git a/source/adios2/engine/bp4/BP4Writer.cpp b/source/adios2/engine/bp4/BP4Writer.cpp index 4cf763e570..169a0d78af 100644 --- a/source/adios2/engine/bp4/BP4Writer.cpp +++ b/source/adios2/engine/bp4/BP4Writer.cpp @@ -823,6 +823,11 @@ size_t BP4Writer::DebugGetDataBufferSize() const return m_BP4Serializer.DebugGetDataBufferSize(); } +void BP4Writer::NotifyEngineAttribute(std::string name, DataType type) noexcept +{ + m_BP4Serializer.m_SerializedAttributes.erase(name); +} + } // end namespace engine } // end namespace core } // end namespace adios2 diff --git a/source/adios2/engine/bp4/BP4Writer.h b/source/adios2/engine/bp4/BP4Writer.h index 4990709e87..74002ae2dd 100644 --- a/source/adios2/engine/bp4/BP4Writer.h +++ b/source/adios2/engine/bp4/BP4Writer.h @@ -166,6 +166,8 @@ class BP4Writer : public core::Engine template void PerformPutCommon(Variable &variable); + + void NotifyEngineAttribute(std::string name, DataType type) noexcept; }; } // end namespace engine diff --git a/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp b/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp index 59db39eb47..373a3719f7 100644 --- a/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp +++ b/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp @@ -930,6 +930,15 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamVar) adios2::IO io = adios.DeclareIO("TestIO"); + if (!engineName.empty()) + { + io.SetEngine(engineName); + } + else + { + io.SetEngine("FileStream"); + } + auto var1 = io.DefineVariable("var1"); auto var2 = io.DefineVariable("var2", shape, start, count); @@ -1021,6 +1030,14 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamVar) }; adios2::IO io = adios.DeclareIO("ReaderIO"); + if (!engineName.empty()) + { + io.SetEngine(engineName); + } + else + { + io.SetEngine("FileStream"); + } adios2::Engine bpReader = io.Open(fName, adios2::Mode::Read); while (bpReader.BeginStep() == adios2::StepStatus::OK) @@ -1035,7 +1052,7 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamVar) auto var2 = io.InquireVariable("var2"); if (var2) { - lf_VerifyAttributes("var1", separator, io, false); + lf_VerifyAttributes("var2", separator, io, false); lf_VerifyAttributes("var2", separator, io, true); } @@ -1043,6 +1060,163 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamVar) } } } + +TEST_F(BPWriteReadAttributes, WriteReadStreamMutable) +{ + const std::string fName = "foo" + std::string(&adios2::PathSeparator, 1) + + "AttributesWriteReadMutable.bp"; + + const std::string separator = "\\"; + + int mpiRank = 0, mpiSize = 1; + // Number of rows + const size_t Nx = 8; + + // Number of steps + const size_t NSteps = 3; + +#if ADIOS2_USE_MPI + MPI_Comm_rank(MPI_COMM_WORLD, &mpiRank); + MPI_Comm_size(MPI_COMM_WORLD, &mpiSize); +#endif + + const double d3[3] = {-1.1, -1.2, -1.3}; + SmallTestData currentTestData = + generateNewSmallTestData(m_TestData, 0, 0, 0); + +// Write test data using BP +#if ADIOS2_USE_MPI + adios2::ADIOS adios(MPI_COMM_WORLD); +#else + adios2::ADIOS adios; +#endif + { + const adios2::Dims shape{static_cast(Nx * mpiSize)}; + const adios2::Dims start{static_cast(Nx * mpiRank)}; + const adios2::Dims count{Nx}; + + adios2::IO io = adios.DeclareIO("TestIO"); + if (!engineName.empty()) + { + io.SetEngine(engineName); + } + else + { + io.SetEngine("FileStream"); + } + + auto var1 = io.DefineVariable("var1"); + auto var2 = io.DefineVariable("var2", shape, start, count); + + io.DefineAttribute("dArray", d3, 3, var1.Name(), separator); + io.DefineAttribute("dArray", d3, 3, var2.Name(), separator); + + io.DefineAttribute("i32Value", -1, var1.Name(), separator); + io.DefineAttribute("i32Value", -1, var2.Name(), separator); + + adios2::Engine bpWriter = io.Open(fName, adios2::Mode::Write); + + for (size_t step = 0; step < NSteps; ++step) + { + // Generate test data for each process uniquely + SmallTestData currentTestData = generateNewSmallTestData( + m_TestData, static_cast(step), mpiRank, mpiSize); + + const int32_t step32 = static_cast(step); + const double stepD = static_cast(step); + double d[3] = {stepD + 0.1, stepD + 0.2, stepD + 0.3}; + + bpWriter.BeginStep(); + + io.DefineAttribute("dArray", d, 3, var1.Name(), separator); + io.DefineAttribute("i32Value", step32, var1.Name(), + separator); + bpWriter.Put(var1, step32); + + if (step % 2 == 0) + { + bpWriter.Put(var2, currentTestData.I32.data()); + io.DefineAttribute("dArray", d, 3, var2.Name(), + separator); + io.DefineAttribute("i32Value", step32, var2.Name(), + separator); + } + + bpWriter.EndStep(); + } + bpWriter.Close(); + } + + // reader + { + auto lf_VerifyAttributes = [](const int32_t step, + const std::string &variableName, + const std::string separator, + adios2::IO &io) { + const std::map attributesInfo = + io.AvailableAttributes(variableName, separator, false); + + const double stepD = static_cast(step); + const double d[3] = {stepD + 0.1, stepD + 0.2, stepD + 0.3}; + + auto itDArray = attributesInfo.find("dArray"); + EXPECT_NE(itDArray, attributesInfo.end()); + EXPECT_EQ(itDArray->second.at("Type"), "double"); + EXPECT_EQ(itDArray->second.at("Elements"), "3"); + + auto a = + io.InquireAttribute("dArray", variableName, separator); + auto adata = a.Data(); + for (int i = 0; i < 3; ++i) + { + EXPECT_EQ(adata[i], d[i]); + } + + const std::string stepS = std::to_string(step); + auto itU32Value = attributesInfo.find("i32Value"); + EXPECT_NE(itU32Value, attributesInfo.end()); + EXPECT_EQ(itU32Value->second.at("Type"), "uint32_t"); + EXPECT_EQ(itU32Value->second.at("Elements"), "1"); + EXPECT_EQ(itU32Value->second.at("Value"), stepS); + }; + + adios2::IO io = adios.DeclareIO("ReaderIO"); + if (!engineName.empty()) + { + io.SetEngine(engineName); + io.SetParameter("StreamReader", "ON"); + } + else + { + io.SetEngine("FileStream"); + } + adios2::Engine bpReader = io.Open(fName, adios2::Mode::Read); + + while (bpReader.BeginStep() == adios2::StepStatus::OK) + { + int32_t step = static_cast(bpReader.CurrentStep()); + if (engineName == "BP3") + { + // BP3 does not support changing attributes + step = 0; + } + auto var1 = io.InquireVariable("var1"); + if (var1) + { + lf_VerifyAttributes(step, "var1", separator, io); + } + + auto var2 = io.InquireVariable("var2"); + if (var2) + { + lf_VerifyAttributes(step, "var2", separator, io); + } + + bpReader.EndStep(); + } + } +} + //****************************************************************************** // main //****************************************************************************** From 2a815a23a81784d5ecd1dba767a1805dea76b10e Mon Sep 17 00:00:00 2001 From: Norbert Podhorszki Date: Tue, 17 Aug 2021 14:17:11 -0400 Subject: [PATCH 2/4] Rework API for attributes to have explicit flag for modifiable attributes. By default, they are non-modifiable. DefineAttribute at the second time will only pass if the attribute is modifiable and the type does not change. --- bindings/CXX11/adios2/cxx11/IO.cpp | 4 +- bindings/CXX11/adios2/cxx11/IO.h | 12 ++- bindings/CXX11/adios2/cxx11/IO.tcc | 19 +++-- source/adios2/core/Attribute.h | 18 +++- source/adios2/core/Attribute.tcc | 83 +++++++++++++++++-- source/adios2/core/AttributeBase.cpp | 13 ++- source/adios2/core/AttributeBase.h | 10 ++- source/adios2/core/IO.cpp | 4 +- source/adios2/core/IO.h | 14 +++- source/adios2/core/IO.tcc | 29 ++++--- .../toolkit/format/bp/bp4/BP4Deserializer.tcc | 6 +- .../engine/bp/TestBPWriteReadAttributes.cpp | 39 +++++---- .../interface/TestADIOSDefineAttribute.cpp | 15 ++++ 13 files changed, 194 insertions(+), 72 deletions(-) diff --git a/bindings/CXX11/adios2/cxx11/IO.cpp b/bindings/CXX11/adios2/cxx11/IO.cpp index 64fcf8204c..ba5584be71 100644 --- a/bindings/CXX11/adios2/cxx11/IO.cpp +++ b/bindings/CXX11/adios2/cxx11/IO.cpp @@ -182,11 +182,11 @@ ADIOS2_FOREACH_TYPE_1ARG(declare_template_instantiation) #define declare_template_instantiation(T) \ template Attribute IO::DefineAttribute( \ const std::string &, const T *, const size_t, const std::string &, \ - const std::string); \ + const std::string, const bool); \ \ template Attribute IO::DefineAttribute(const std::string &, const T &, \ const std::string &, \ - const std::string); \ + const std::string, const bool); \ \ template Attribute IO::InquireAttribute( \ const std::string &, const std::string &, const std::string); diff --git a/bindings/CXX11/adios2/cxx11/IO.h b/bindings/CXX11/adios2/cxx11/IO.h index 49067e7512..6e6013c049 100644 --- a/bindings/CXX11/adios2/cxx11/IO.h +++ b/bindings/CXX11/adios2/cxx11/IO.h @@ -172,6 +172,7 @@ class IO * @param separator default is "/", hierarchy between variable name and * attribute, e.g. variableName/attribute1, variableName::attribute1. Not * used if variableName is empty. + * @param allowModification true allows redefining existing attribute * @return object reference to internal Attribute in IO * @exception std::invalid_argument if Attribute with unique name (in IO or * Variable) is already defined @@ -180,7 +181,8 @@ class IO Attribute DefineAttribute(const std::string &name, const T *data, const size_t size, const std::string &variableName = "", - const std::string separator = "/"); + const std::string separator = "/", + const bool allowModification = false); /** * @brief Define single value attribute @@ -192,6 +194,7 @@ class IO * @param separator default is "/", hierarchy between variable name and * attribute, e.g. variableName/attribute1, variableName::attribute1. Not * used if variableName is empty. + * @param allowModification true allows redefining existing attribute * @return object reference to internal Attribute in IO * @exception std::invalid_argument if Attribute with unique name (in IO or * Variable) is already defined @@ -199,7 +202,8 @@ class IO template Attribute DefineAttribute(const std::string &name, const T &value, const std::string &variableName = "", - const std::string separator = "/"); + const std::string separator = "/", + const bool allowModification = false); /** * @brief Retrieve an existing attribute @@ -386,11 +390,11 @@ ADIOS2_FOREACH_TYPE_1ARG(declare_template_instantiation) #define declare_template_instantiation(T) \ extern template Attribute IO::DefineAttribute( \ const std::string &, const T *, const size_t, const std::string &, \ - const std::string); \ + const std::string, const bool); \ \ extern template Attribute IO::DefineAttribute( \ const std::string &, const T &, const std::string &, \ - const std::string); \ + const std::string, const bool); \ \ extern template Attribute IO::InquireAttribute( \ const std::string &, const std::string &, const std::string); diff --git a/bindings/CXX11/adios2/cxx11/IO.tcc b/bindings/CXX11/adios2/cxx11/IO.tcc index c83213b09f..2b10b0e4a3 100644 --- a/bindings/CXX11/adios2/cxx11/IO.tcc +++ b/bindings/CXX11/adios2/cxx11/IO.tcc @@ -38,31 +38,32 @@ Variable IO::InquireVariable(const std::string &name) } template -Attribute IO::DefineAttribute(const std::string &name, const T *data, - const size_t size, - const std::string &variableName, - const std::string separator) +Attribute +IO::DefineAttribute(const std::string &name, const T *data, const size_t size, + const std::string &variableName, + const std::string separator, const bool allowModification) { using IOType = typename TypeInfo::IOType; helper::CheckForNullptr(m_IO, "for attribute name " + name + " and variable name " + variableName + ", in call to IO::DefineAttribute"); - return Attribute( - &m_IO->DefineAttribute(name, reinterpret_cast(data), - size, variableName, separator)); + return Attribute(&m_IO->DefineAttribute( + name, reinterpret_cast(data), size, variableName, + separator, allowModification)); } template Attribute IO::DefineAttribute(const std::string &name, const T &value, const std::string &variableName, - const std::string separator) + const std::string separator, + const bool allowModification) { using IOType = typename TypeInfo::IOType; helper::CheckForNullptr(m_IO, "for attribute name " + name + ", in call to IO::DefineAttribute"); return Attribute( &m_IO->DefineAttribute(name, reinterpret_cast(value), - variableName, separator)); + variableName, separator, allowModification)); } template diff --git a/source/adios2/core/Attribute.h b/source/adios2/core/Attribute.h index 83fb5fcf1b..84be2c6451 100644 --- a/source/adios2/core/Attribute.h +++ b/source/adios2/core/Attribute.h @@ -38,19 +38,33 @@ class Attribute : public AttributeBase * @param name * @param data * @param elements + * @param allowModifications */ - Attribute(const std::string &name, const T *data, const size_t elements); + Attribute(const std::string &name, const T *data, const size_t elements, + const bool allowModification); /** * Single value constructor * @param name * @param data * @param elements + * @param allowModifications */ - Attribute(const std::string &name, const T &data); + Attribute(const std::string &name, const T &data, + const bool allowModification); ~Attribute() = default; + /** + * Modification of an existing attribute (array) + */ + void Modify(const T *data, const size_t elements); + + /** + * Modification of an existing attribute (single value) + */ + void Modify(const T &data); + private: std::string DoGetInfoValue() const noexcept override; }; diff --git a/source/adios2/core/Attribute.tcc b/source/adios2/core/Attribute.tcc index 7516d6644e..265bf11daa 100644 --- a/source/adios2/core/Attribute.tcc +++ b/source/adios2/core/Attribute.tcc @@ -60,27 +60,94 @@ template Attribute::Attribute(const Attribute &other) : AttributeBase(other), m_DataArray(other.m_DataArray) { - Pad::Zero(m_DataSingleValue); - m_DataSingleValue = other.m_DataSingleValue; + if (other.m_IsSingleValue) + { + m_DataArray.clear(); + m_DataSingleValue = other.m_DataSingleValue; + } + else + { + m_DataArray = other.m_DataArray; + Pad::Zero(m_DataSingleValue); + } } template Attribute::Attribute(const std::string &name, const T *array, - const size_t elements) -: AttributeBase(name, helper::GetDataType(), elements) + const size_t elements, const bool allowModification) +: AttributeBase(name, helper::GetDataType(), elements, allowModification) { - Pad::Zero(m_DataSingleValue); m_DataArray = std::vector(array, array + elements); + Pad::Zero(m_DataSingleValue); } template -Attribute::Attribute(const std::string &name, const T &value) -: AttributeBase(name, helper::GetDataType()) +Attribute::Attribute(const std::string &name, const T &value, + const bool allowModification) +: AttributeBase(name, helper::GetDataType(), allowModification) { - Pad::Zero(m_DataSingleValue); + m_DataArray.clear(); m_DataSingleValue = value; } +template +void Attribute::Modify(const T *data, const size_t elements) +{ + if (m_AllowModification) + { + if (this->m_Type == helper::GetDataType()) + { + m_DataArray = std::vector(data, data + elements); + Pad::Zero(m_DataSingleValue); + this->m_IsSingleValue = false; + this->m_Elements = elements; + } + else + { + throw std::invalid_argument( + "ERROR: modifiable attribute " + this->m_Name + + " has been defined with type " + ToString(this->m_Type) + + ". Type cannot be changed to " + + ToString(helper::GetDataType()) + "\n"); + } + } + else + { + throw std::invalid_argument( + "ERROR: Trying to modify attribute " + this->m_Name + + " which has been defined as non-modifiable\n"); + } +} + +template +void Attribute::Modify(const T &data) +{ + if (m_AllowModification) + { + if (this->m_Type == helper::GetDataType()) + { + m_DataArray.clear(); + m_DataSingleValue = data; + this->m_IsSingleValue = true; + this->m_Elements = 1; + } + else + { + throw std::invalid_argument( + "ERROR: modifiable attribute " + this->m_Name + + " has been defined with type " + ToString(this->m_Type) + + ". Type cannot be changed to " + + ToString(helper::GetDataType()) + "\n"); + } + } + else + { + throw std::invalid_argument( + "ERROR: Trying to modify attribute " + this->m_Name + + " which has been defined as non-modifiable\n"); + } +} + template std::string Attribute::DoGetInfoValue() const noexcept { diff --git a/source/adios2/core/AttributeBase.cpp b/source/adios2/core/AttributeBase.cpp index f77ccdb4c5..dcc03d60f3 100644 --- a/source/adios2/core/AttributeBase.cpp +++ b/source/adios2/core/AttributeBase.cpp @@ -15,14 +15,18 @@ namespace adios2 namespace core { -AttributeBase::AttributeBase(const std::string &name, const DataType type) -: m_Name(name), m_Type(type), m_Elements(1), m_IsSingleValue(true) +AttributeBase::AttributeBase(const std::string &name, const DataType type, + const bool allowModification) +: m_Name(name), m_Type(type), m_Elements(1), m_IsSingleValue(true), + m_AllowModification(allowModification) { } AttributeBase::AttributeBase(const std::string &name, const DataType type, - const size_t elements) -: m_Name(name), m_Type(type), m_Elements(elements), m_IsSingleValue(false) + const size_t elements, + const bool allowModification) +: m_Name(name), m_Type(type), m_Elements(elements), m_IsSingleValue(false), + m_AllowModification(allowModification) { } @@ -32,6 +36,7 @@ Params AttributeBase::GetInfo() const noexcept info["Type"] = ToString(m_Type); info["Elements"] = std::to_string(m_Elements); info["Value"] = this->DoGetInfoValue(); + info["Modifiable"] = std::to_string(m_AllowModification); return info; } diff --git a/source/adios2/core/AttributeBase.h b/source/adios2/core/AttributeBase.h index 94d2916f5c..d2da95e3e1 100644 --- a/source/adios2/core/AttributeBase.h +++ b/source/adios2/core/AttributeBase.h @@ -29,15 +29,17 @@ class AttributeBase public: const std::string m_Name; const DataType m_Type; - const size_t m_Elements; - const bool m_IsSingleValue; + size_t m_Elements; + bool m_IsSingleValue; + const bool m_AllowModification; /** * Single value constructor used by Attribute derived class * @param name * @param type */ - AttributeBase(const std::string &name, const DataType type); + AttributeBase(const std::string &name, const DataType type, + const bool allowModification); /** * Array constructor used by Attribute derived class @@ -46,7 +48,7 @@ class AttributeBase * @param elements */ AttributeBase(const std::string &name, const DataType type, - const size_t elements); + const size_t elements, const bool allowModification); virtual ~AttributeBase() = default; diff --git a/source/adios2/core/IO.cpp b/source/adios2/core/IO.cpp index 51b40067f9..b0c87b4a5e 100644 --- a/source/adios2/core/IO.cpp +++ b/source/adios2/core/IO.cpp @@ -842,10 +842,10 @@ ADIOS2_FOREACH_STDTYPE_1ARG(define_template_instantiation) #define declare_template_instantiation(T) \ template Attribute &IO::DefineAttribute( \ const std::string &, const T *, const size_t, const std::string &, \ - const std::string); \ + const std::string, const bool); \ template Attribute &IO::DefineAttribute( \ const std::string &, const T &, const std::string &, \ - const std::string); \ + const std::string, const bool); \ template Attribute *IO::InquireAttribute( \ const std::string &, const std::string &, const std::string) noexcept; diff --git a/source/adios2/core/IO.h b/source/adios2/core/IO.h index 1034094b73..7170b11466 100644 --- a/source/adios2/core/IO.h +++ b/source/adios2/core/IO.h @@ -197,6 +197,8 @@ class IO * @param array pointer to user data * @param elements number of data elements * @param variableName optionally associates the attribute to a Variable + * @param allowModification true allows redefining/modifying existing + * attribute * @return reference to internal Attribute * @exception std::invalid_argument if Attribute with unique name is already * defined @@ -205,12 +207,15 @@ class IO Attribute &DefineAttribute(const std::string &name, const T *array, const size_t elements, const std::string &variableName = "", - const std::string separator = "/"); + const std::string separator = "/", + const bool allowModification = false); /** * @brief Define single value attribute * @param name must be unique for the IO object * @param value single data value + * @param allowModification true allows redefining/modifying existing + * attribute * @return reference to internal Attribute * @exception std::invalid_argument if Attribute with unique name is already * defined @@ -218,7 +223,8 @@ class IO template Attribute &DefineAttribute(const std::string &name, const T &value, const std::string &variableName = "", - const std::string separator = "/"); + const std::string separator = "/", + const bool allowModification = false); /** * @brief Removes an existing Variable in current IO object. @@ -534,10 +540,10 @@ ADIOS2_FOREACH_STDTYPE_1ARG(declare_template_instantiation) #define declare_template_instantiation(T) \ extern template Attribute &IO::DefineAttribute( \ const std::string &, const T *, const size_t, const std::string &, \ - const std::string); \ + const std::string, const bool); \ extern template Attribute &IO::DefineAttribute( \ const std::string &, const T &, const std::string &, \ - const std::string); \ + const std::string, const bool); \ extern template Attribute *IO::InquireAttribute( \ const std::string &, const std::string &, const std::string) noexcept; diff --git a/source/adios2/core/IO.tcc b/source/adios2/core/IO.tcc index 3404f61e50..445ddbbf43 100644 --- a/source/adios2/core/IO.tcc +++ b/source/adios2/core/IO.tcc @@ -100,7 +100,8 @@ Variable *IO::InquireVariable(const std::string &name) noexcept template Attribute &IO::DefineAttribute(const std::string &name, const T &value, const std::string &variableName, - const std::string separator) + const std::string separator, + const bool allowModification) { PERFSTUBS_SCOPED_TIMER("IO::DefineAttribute"); if (!variableName.empty() && @@ -121,8 +122,9 @@ Attribute &IO::DefineAttribute(const std::string &name, const T &value, if (helper::ValueToString(value) != itExistingAttribute->second->GetInfo()["Value"]) { - itExistingAttribute->second = std::unique_ptr( - new Attribute(globalName, value)); + Attribute &a = + static_cast &>(*itExistingAttribute->second); + a.Modify(value); for (auto &e : m_Engines) { e.second->NotifyEngineAttribute( @@ -134,8 +136,8 @@ Attribute &IO::DefineAttribute(const std::string &name, const T &value, else { auto itAttributePair = m_Attributes.emplace( - globalName, std::unique_ptr( - new Attribute(globalName, value))); + globalName, std::unique_ptr(new Attribute( + globalName, value, allowModification))); for (auto &e : m_Engines) { e.second->NotifyEngineAttribute( @@ -146,10 +148,10 @@ Attribute &IO::DefineAttribute(const std::string &name, const T &value, } template -Attribute &IO::DefineAttribute(const std::string &name, const T *array, - const size_t elements, - const std::string &variableName, - const std::string separator) +Attribute & +IO::DefineAttribute(const std::string &name, const T *array, + const size_t elements, const std::string &variableName, + const std::string separator, const bool allowModification) { PERFSTUBS_SCOPED_TIMER("IO::DefineAttribute"); if (!variableName.empty() && @@ -174,8 +176,9 @@ Attribute &IO::DefineAttribute(const std::string &name, const T *array, if (itExistingAttribute->second->GetInfo()["Value"] != arrayValues) { - itExistingAttribute->second = std::unique_ptr( - new Attribute(globalName, array, elements)); + Attribute &a = + static_cast &>(*itExistingAttribute->second); + a.Modify(array, elements); for (auto &e : m_Engines) { e.second->NotifyEngineAttribute( @@ -187,8 +190,8 @@ Attribute &IO::DefineAttribute(const std::string &name, const T *array, else { auto itAttributePair = m_Attributes.emplace( - globalName, std::unique_ptr( - new Attribute(globalName, array, elements))); + globalName, std::unique_ptr(new Attribute( + globalName, array, elements, allowModification))); for (auto &e : m_Engines) { e.second->NotifyEngineAttribute( diff --git a/source/adios2/toolkit/format/bp/bp4/BP4Deserializer.tcc b/source/adios2/toolkit/format/bp/bp4/BP4Deserializer.tcc index 7ffa95d3d4..8092f98f40 100644 --- a/source/adios2/toolkit/format/bp/bp4/BP4Deserializer.tcc +++ b/source/adios2/toolkit/format/bp/bp4/BP4Deserializer.tcc @@ -1039,14 +1039,14 @@ void BP4Deserializer::DefineAttributeInEngineIO( if (characteristics.Statistics.IsValue) { - engine.m_IO.DefineAttribute(attributeName, - characteristics.Statistics.Value); + engine.m_IO.DefineAttribute( + attributeName, characteristics.Statistics.Value, "", "", true); } else { engine.m_IO.DefineAttribute( attributeName, characteristics.Statistics.Values.data(), - characteristics.Statistics.Values.size()); + characteristics.Statistics.Values.size(), "", "", true); } } diff --git a/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp b/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp index 373a3719f7..10b23630c8 100644 --- a/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp +++ b/testing/adios2/engine/bp/TestBPWriteReadAttributes.cpp @@ -1061,10 +1061,10 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamVar) } } -TEST_F(BPWriteReadAttributes, WriteReadStreamMutable) +TEST_F(BPWriteReadAttributes, WriteReadStreamModifiable) { const std::string fName = "foo" + std::string(&adios2::PathSeparator, 1) + - "AttributesWriteReadMutable.bp"; + "AttributesWriteReadModifiable.bp"; const std::string separator = "\\"; @@ -1108,11 +1108,15 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamMutable) auto var1 = io.DefineVariable("var1"); auto var2 = io.DefineVariable("var2", shape, start, count); - io.DefineAttribute("dArray", d3, 3, var1.Name(), separator); - io.DefineAttribute("dArray", d3, 3, var2.Name(), separator); + io.DefineAttribute("dArray", d3, 3, var1.Name(), separator, + true); + io.DefineAttribute("dArray", d3, 3, var2.Name(), separator, + true); - io.DefineAttribute("i32Value", -1, var1.Name(), separator); - io.DefineAttribute("i32Value", -1, var2.Name(), separator); + io.DefineAttribute("i32Value", -1, var1.Name(), separator, + true); + io.DefineAttribute("i32Value", -1, var2.Name(), separator, + true); adios2::Engine bpWriter = io.Open(fName, adios2::Mode::Write); @@ -1128,18 +1132,19 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamMutable) bpWriter.BeginStep(); - io.DefineAttribute("dArray", d, 3, var1.Name(), separator); - io.DefineAttribute("i32Value", step32, var1.Name(), - separator); + io.DefineAttribute("dArray", d, 3, var1.Name(), separator, + true); + io.DefineAttribute("i32Value", step32, var1.Name(), + separator, true); bpWriter.Put(var1, step32); if (step % 2 == 0) { bpWriter.Put(var2, currentTestData.I32.data()); io.DefineAttribute("dArray", d, 3, var2.Name(), - separator); - io.DefineAttribute("i32Value", step32, var2.Name(), - separator); + separator, true); + io.DefineAttribute("i32Value", step32, var2.Name(), + separator, true); } bpWriter.EndStep(); @@ -1173,11 +1178,11 @@ TEST_F(BPWriteReadAttributes, WriteReadStreamMutable) } const std::string stepS = std::to_string(step); - auto itU32Value = attributesInfo.find("i32Value"); - EXPECT_NE(itU32Value, attributesInfo.end()); - EXPECT_EQ(itU32Value->second.at("Type"), "uint32_t"); - EXPECT_EQ(itU32Value->second.at("Elements"), "1"); - EXPECT_EQ(itU32Value->second.at("Value"), stepS); + auto iti32Value = attributesInfo.find("i32Value"); + EXPECT_NE(iti32Value, attributesInfo.end()); + EXPECT_EQ(iti32Value->second.at("Type"), "int32_t"); + EXPECT_EQ(iti32Value->second.at("Elements"), "1"); + EXPECT_EQ(iti32Value->second.at("Value"), stepS); }; adios2::IO io = adios.DeclareIO("ReaderIO"); diff --git a/testing/adios2/interface/TestADIOSDefineAttribute.cpp b/testing/adios2/interface/TestADIOSDefineAttribute.cpp index eeb4f30f5f..3eb7aa1816 100644 --- a/testing/adios2/interface/TestADIOSDefineAttribute.cpp +++ b/testing/adios2/interface/TestADIOSDefineAttribute.cpp @@ -45,6 +45,7 @@ TEST_F(ADIOSDefineAttributeTest, DefineAttributeNameException) auto availableAttributes = io.AvailableAttributes(); EXPECT_EQ(availableAttributes.size(), 1); + // Redefinition is not allowed (non-modifiable attribute) EXPECT_THROW(io.DefineAttribute(name, "0"), std::invalid_argument); @@ -54,6 +55,20 @@ TEST_F(ADIOSDefineAttributeTest, DefineAttributeNameException) auto attributeString2 = io.InquireAttribute(name); EXPECT_TRUE(attributeString2); + + /* Modifiable attribute can change its value(s) ... */ + io.DefineAttribute("modifiable", "initial", "", "", true); + io.DefineAttribute("modifiable", "modified", "", "", true); + + auto attributeString3 = io.InquireAttribute("modifiable"); + EXPECT_TRUE(attributeString3); + auto attributeString3Value = attributeString3.Data(); + ASSERT_EQ(attributeString3Value.size() == 1, true); + EXPECT_EQ(attributeString3Value[0], "modified"); + + /* ... but not its type */ + EXPECT_THROW(io.DefineAttribute("modifiable", 1.0, "", "", true), + std::invalid_argument); } TEST_F(ADIOSDefineAttributeTest, DefineAttributeTypeByValue) From 057cfda124fb071e53d58186df4601b451c685eb Mon Sep 17 00:00:00 2001 From: Norbert Podhorszki Date: Tue, 17 Aug 2021 16:18:08 -0400 Subject: [PATCH 3/4] add back padding magic that I removed ignorantly to fix msan tests --- source/adios2/core/Attribute.tcc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/adios2/core/Attribute.tcc b/source/adios2/core/Attribute.tcc index 265bf11daa..7061643a42 100644 --- a/source/adios2/core/Attribute.tcc +++ b/source/adios2/core/Attribute.tcc @@ -63,6 +63,7 @@ Attribute::Attribute(const Attribute &other) if (other.m_IsSingleValue) { m_DataArray.clear(); + Pad::Zero(m_DataSingleValue); m_DataSingleValue = other.m_DataSingleValue; } else @@ -87,6 +88,7 @@ Attribute::Attribute(const std::string &name, const T &value, : AttributeBase(name, helper::GetDataType(), allowModification) { m_DataArray.clear(); + Pad::Zero(m_DataSingleValue); m_DataSingleValue = value; } @@ -127,6 +129,7 @@ void Attribute::Modify(const T &data) if (this->m_Type == helper::GetDataType()) { m_DataArray.clear(); + Pad::Zero(m_DataSingleValue); m_DataSingleValue = data; this->m_IsSingleValue = true; this->m_Elements = 1; From 4205973266222d56977e8ae80d851d509809c340 Mon Sep 17 00:00:00 2001 From: Norbert Podhorszki Date: Wed, 18 Aug 2021 16:14:07 -0400 Subject: [PATCH 4/4] Check type of attribute before attempting modification (before casting the pointer). --- source/adios2/core/Attribute.tcc | 40 ++++++-------------------- source/adios2/core/IO.tcc | 48 ++++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 43 deletions(-) diff --git a/source/adios2/core/Attribute.tcc b/source/adios2/core/Attribute.tcc index 7061643a42..8232e8c6f9 100644 --- a/source/adios2/core/Attribute.tcc +++ b/source/adios2/core/Attribute.tcc @@ -97,21 +97,10 @@ void Attribute::Modify(const T *data, const size_t elements) { if (m_AllowModification) { - if (this->m_Type == helper::GetDataType()) - { - m_DataArray = std::vector(data, data + elements); - Pad::Zero(m_DataSingleValue); - this->m_IsSingleValue = false; - this->m_Elements = elements; - } - else - { - throw std::invalid_argument( - "ERROR: modifiable attribute " + this->m_Name + - " has been defined with type " + ToString(this->m_Type) + - ". Type cannot be changed to " + - ToString(helper::GetDataType()) + "\n"); - } + m_DataArray = std::vector(data, data + elements); + Pad::Zero(m_DataSingleValue); + this->m_IsSingleValue = false; + this->m_Elements = elements; } else { @@ -126,22 +115,11 @@ void Attribute::Modify(const T &data) { if (m_AllowModification) { - if (this->m_Type == helper::GetDataType()) - { - m_DataArray.clear(); - Pad::Zero(m_DataSingleValue); - m_DataSingleValue = data; - this->m_IsSingleValue = true; - this->m_Elements = 1; - } - else - { - throw std::invalid_argument( - "ERROR: modifiable attribute " + this->m_Name + - " has been defined with type " + ToString(this->m_Type) + - ". Type cannot be changed to " + - ToString(helper::GetDataType()) + "\n"); - } + m_DataArray.clear(); + Pad::Zero(m_DataSingleValue); + m_DataSingleValue = data; + this->m_IsSingleValue = true; + this->m_Elements = 1; } else { diff --git a/source/adios2/core/IO.tcc b/source/adios2/core/IO.tcc index 445ddbbf43..63c98dfd32 100644 --- a/source/adios2/core/IO.tcc +++ b/source/adios2/core/IO.tcc @@ -122,13 +122,25 @@ Attribute &IO::DefineAttribute(const std::string &name, const T &value, if (helper::ValueToString(value) != itExistingAttribute->second->GetInfo()["Value"]) { - Attribute &a = - static_cast &>(*itExistingAttribute->second); - a.Modify(value); - for (auto &e : m_Engines) + if (itExistingAttribute->second->m_Type == helper::GetDataType()) { - e.second->NotifyEngineAttribute( - globalName, itExistingAttribute->second->m_Type); + Attribute &a = + static_cast &>(*itExistingAttribute->second); + a.Modify(value); + for (auto &e : m_Engines) + { + e.second->NotifyEngineAttribute( + globalName, itExistingAttribute->second->m_Type); + } + } + else + { + throw std::invalid_argument( + "ERROR: modifiable attribute " + globalName + + " has been defined with type " + + ToString(itExistingAttribute->second->m_Type) + + ". Type cannot be changed to " + + ToString(helper::GetDataType()) + "\n"); } } return static_cast &>(*itExistingAttribute->second); @@ -176,13 +188,25 @@ IO::DefineAttribute(const std::string &name, const T *array, if (itExistingAttribute->second->GetInfo()["Value"] != arrayValues) { - Attribute &a = - static_cast &>(*itExistingAttribute->second); - a.Modify(array, elements); - for (auto &e : m_Engines) + if (itExistingAttribute->second->m_Type == helper::GetDataType()) + { + Attribute &a = + static_cast &>(*itExistingAttribute->second); + a.Modify(array, elements); + for (auto &e : m_Engines) + { + e.second->NotifyEngineAttribute( + globalName, itExistingAttribute->second->m_Type); + } + } + else { - e.second->NotifyEngineAttribute( - globalName, itExistingAttribute->second->m_Type); + throw std::invalid_argument( + "ERROR: modifiable attribute " + globalName + + " has been defined with type " + + ToString(itExistingAttribute->second->m_Type) + + ". Type cannot be changed to " + + ToString(helper::GetDataType()) + "\n"); } } return static_cast &>(*itExistingAttribute->second);