Skip to content
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

[WIP] [QC-1176] Display a warning for developers if a registered object is not mergeable #2400

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions Framework/include/QualityControl/ObjectsManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@
#include "QualityControl/Activity.h"
#include "QualityControl/MonitorObject.h"
#include "QualityControl/MonitorObjectCollection.h"
#include <Mergers/Mergeable.h>
// stl
#include <concepts>
#include <string>
#include <memory>
#include <type_traits>

class TObject;
class TObjArray;

namespace o2::quality_control::core
{
Expand Down Expand Up @@ -76,17 +78,31 @@ class ObjectsManager
/**
* Start publishing the object obj, i.e. it will be pushed forward in the workflow at regular intervals.
* The ownership remains to the caller.
* @param IgnoreMergeable if you want to ignore static_assert check for Mergeable
* @param T type of object that we want to publish.
* @param obj The object to publish.
* @throws DuplicateObjectError
*/
void startPublishing(TObject* obj, PublicationPolicy = PublicationPolicy::Forever);
template <bool IgnoreMergeable = false, typename T>
void startPublishing(T obj, PublicationPolicy policy = PublicationPolicy::Forever)
{
// We don't want to do this compile time check in PostProcessing
#ifndef QUALITYCONTROL_POSTPROCESSINTERFACE_H
static_assert(std::same_as<std::remove_pointer_t<T>, TObject> ||
IgnoreMergeable || mergers::Mergeable<T>,
"you are trying to startPublishing object that is not mergeable."
" If you know what you are doing use startPublishing<true>(...)");
#endif
startPublishingImpl(obj, policy, IgnoreMergeable);
}

/**
* Stop publishing this object
* @param obj
* @throw ObjectNotFoundError if object is not found.
*/
void stopPublishing(TObject* obj);
void
stopPublishing(TObject* obj);

/**
* Stop publishing this object
Expand Down Expand Up @@ -223,6 +239,8 @@ class ObjectsManager
bool mUpdateServiceDiscovery;
Activity mActivity;
std::vector<std::string> mMovingWindowsList;

void startPublishingImpl(TObject* obj, PublicationPolicy, bool ignoreMergeableWarning);
};

} // namespace o2::quality_control::core
Expand Down
8 changes: 7 additions & 1 deletion Framework/src/ObjectsManager.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,22 @@ ObjectsManager::~ObjectsManager()
ILOG(Debug, Devel) << "ObjectsManager destructor" << ENDM;
}

void ObjectsManager::startPublishing(TObject* object, PublicationPolicy publicationPolicy)
void ObjectsManager::startPublishingImpl(TObject* object, PublicationPolicy publicationPolicy, bool ignoreMergeableWarning)
{
if (!object) {
ILOG(Warning, Support) << "A nullptr provided to ObjectManager::startPublishing" << ENDM;
return;
}

if (mMonitorObjects->FindObject(object->GetName()) != nullptr) {
ILOG(Warning, Support) << "Object is already being published (" << object->GetName() << "), will remove it and add the new one" << ENDM;
stopPublishing(object->GetName());
}

if (!ignoreMergeableWarning && !mergers::isMergeable(object)) {
ILOG(Warning, Support) << "Object '" + std::string(object->GetName()) + "' with type '" + std::string(object->ClassName()) + "' is not one of the mergeable types, it will not be correctly merged in distributed setups, such as P2 and Grid" << ENDM;
}

auto* newObject = new MonitorObject(object, mTaskName, mTaskClass, mDetectorName);
newObject->setIsOwner(false);
newObject->setActivity(mActivity);
Expand Down
4 changes: 2 additions & 2 deletions Framework/src/SliceTrendingTask.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ void SliceTrendingTask::generatePlots()
}

mPlots[plot.name] = c;
getObjectsManager()->startPublishing(c, PublicationPolicy::Once);
getObjectsManager()->startPublishing<true>(c, PublicationPolicy::Once);
}
} // void SliceTrendingTask::generatePlots()

Expand Down Expand Up @@ -666,4 +666,4 @@ std::string SliceTrendingTask::beautifyTitle(const std::string_view rawtitle, co
}

return beautified;
}
}
2 changes: 1 addition & 1 deletion Framework/src/TrendingTask.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ void TrendingTask::generatePlots()
}
auto c = drawPlot(plotConfig);
mPlots[plotConfig.name].reset(c);
getObjectsManager()->startPublishing(c, PublicationPolicy::Once);
getObjectsManager()->startPublishing<true>(c, PublicationPolicy::Once);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should not need <true> in TrendingTask and SliceTrendingTask anymore, after you disabled the check for postprocessing tasks.

}
}

Expand Down
36 changes: 18 additions & 18 deletions Framework/test/testObjectsManager.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ BOOST_AUTO_TEST_CASE(duplicate_object_test)
config.consulUrl = "";
ObjectsManager objectsManager(config.taskName, config.taskClass, config.detectorName, config.consulUrl, 0, true);
TObjString s("content");
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
BOOST_CHECK_NO_THROW(objectsManager.startPublishing(&s, PublicationPolicy::Forever));
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
BOOST_CHECK_NO_THROW(objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever));
BOOST_REQUIRE(objectsManager.getMonitorObject("content") != nullptr);

TObjString s2("content");
BOOST_CHECK_NO_THROW(objectsManager.startPublishing(&s2, PublicationPolicy::Forever));
BOOST_CHECK_NO_THROW(objectsManager.startPublishing<true>(&s2, PublicationPolicy::Forever));
auto mo2 = objectsManager.getMonitorObject("content");
BOOST_REQUIRE(mo2 != nullptr);
BOOST_REQUIRE(mo2->getObject() != &s);
Expand All @@ -73,8 +73,8 @@ BOOST_AUTO_TEST_CASE(is_being_published_test)
ObjectsManager objectsManager(config.taskName, config.taskClass, config.detectorName, config.consulUrl, 0, true);
TObjString s("content");
BOOST_CHECK(!objectsManager.isBeingPublished("content"));
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
BOOST_CHECK_NO_THROW(objectsManager.startPublishing(&s, PublicationPolicy::Forever));
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
BOOST_CHECK_NO_THROW(objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever));
BOOST_CHECK(objectsManager.isBeingPublished("content"));
}

Expand All @@ -84,46 +84,46 @@ BOOST_AUTO_TEST_CASE(unpublish_test)
config.taskName = "test";
ObjectsManager objectsManager(config.taskName, config.taskClass, config.detectorName, config.consulUrl, 0, true);
TObjString s("content");
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 1);
objectsManager.stopPublishing(&s);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 0);
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 1);
objectsManager.stopPublishing("content");
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 0);
BOOST_CHECK_THROW(objectsManager.stopPublishing("content"), ObjectNotFoundError);
BOOST_CHECK_THROW(objectsManager.stopPublishing("asdf"), ObjectNotFoundError);

// unpublish all
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 1);
objectsManager.stopPublishingAll();
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 0);
BOOST_CHECK_NO_THROW(objectsManager.stopPublishingAll());

// unpublish after deletion
auto s2 = new TObjString("content");
objectsManager.startPublishing(s2, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(s2, PublicationPolicy::Forever);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 1);
delete s2;
objectsManager.stopPublishing(s2);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 0);

// unpublish for publication policy
auto s3 = new TObjString("content3");
objectsManager.startPublishing(s3, PublicationPolicy::Once);
objectsManager.startPublishing<true>(s3, PublicationPolicy::Once);
auto s4 = new TObjString("content4");
objectsManager.startPublishing(s4, PublicationPolicy::Once);
objectsManager.startPublishing<true>(s4, PublicationPolicy::Once);
auto s5 = new TObjString("content5");
objectsManager.startPublishing(s5, PublicationPolicy::ThroughStop);
objectsManager.startPublishing<true>(s5, PublicationPolicy::ThroughStop);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 3);
objectsManager.stopPublishing(PublicationPolicy::Once);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 1);
objectsManager.stopPublishing(PublicationPolicy::ThroughStop);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 0);

objectsManager.startPublishing(s3, PublicationPolicy::Once);
objectsManager.startPublishing<true>(s3, PublicationPolicy::Once);
objectsManager.stopPublishing(s3);
BOOST_CHECK_EQUAL(objectsManager.getNumberPublishedObjects(), 0);
BOOST_CHECK_NO_THROW(objectsManager.stopPublishing(PublicationPolicy::Once));
Expand All @@ -145,8 +145,8 @@ BOOST_AUTO_TEST_CASE(getters_test)
TObjString s("content");
TH1F h("histo", "h", 100, 0, 99);

objectsManager.startPublishing(&s, PublicationPolicy::Forever);
objectsManager.startPublishing(&h, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&h, PublicationPolicy::Forever);

// basic gets
BOOST_CHECK_NO_THROW(objectsManager.getMonitorObject("content"));
Expand Down Expand Up @@ -174,8 +174,8 @@ BOOST_AUTO_TEST_CASE(metadata_test)

TObjString s("content");
TH1F h("histo", "h", 100, 0, 99);
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
objectsManager.startPublishing(&h, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&h, PublicationPolicy::Forever);

objectsManager.addMetadata("content", "aaa", "bbb");
BOOST_CHECK_EQUAL(objectsManager.getMonitorObject("content")->getMetadataMap().at("aaa"), "bbb");
Expand Down Expand Up @@ -211,7 +211,7 @@ BOOST_AUTO_TEST_CASE(feed_with_nullptr)
config.consulUrl = "";
ObjectsManager objectsManager(config.taskName, config.taskClass, config.detectorName, config.consulUrl, 0, true);

BOOST_CHECK_NO_THROW(objectsManager.startPublishing(nullptr, PublicationPolicy::Forever));
BOOST_CHECK_NO_THROW(objectsManager.startPublishing<true>(nullptr, PublicationPolicy::Forever));
BOOST_CHECK_NO_THROW(objectsManager.setDefaultDrawOptions(nullptr, ""));
BOOST_CHECK_NO_THROW(objectsManager.setDisplayHint(nullptr, ""));
BOOST_CHECK_NO_THROW(objectsManager.stopPublishing(nullptr));
Expand Down
2 changes: 1 addition & 1 deletion Framework/test/testPublisher.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ BOOST_AUTO_TEST_CASE(publisher_test)
std::string consulUrl = "invalid";
ObjectsManager objectsManager(taskName, "taskClass", detectorName, consulUrl, 0, true);
TObjString s("content");
objectsManager.startPublishing(&s, PublicationPolicy::Forever);
objectsManager.startPublishing<true>(&s, PublicationPolicy::Forever);

TObjString* s2 = (TObjString*)(objectsManager.getMonitorObject("content")->getObject());
BOOST_CHECK_EQUAL(s.GetString(), s2->GetString());
Expand Down
8 changes: 4 additions & 4 deletions Modules/CTP/src/CountersQcTask.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ void CTPCountersTask::initialize(o2::framework::InitContext& /*ctx*/)
mHistInputRate[i]->Draw();
mHistInputRate[i]->SetBit(TObject::kCanDelete);
}
getObjectsManager()->startPublishing(mTCanvasInputs);
getObjectsManager()->startPublishing<true>(mTCanvasInputs);
}

{
Expand All @@ -79,7 +79,7 @@ void CTPCountersTask::initialize(o2::framework::InitContext& /*ctx*/)
mHistClassRate[i]->Draw();
mHistClassRate[i]->SetBit(TObject::kCanDelete);
}
getObjectsManager()->startPublishing(mTCanvasClasses);
getObjectsManager()->startPublishing<true>(mTCanvasClasses);
}

{
Expand All @@ -100,7 +100,7 @@ void CTPCountersTask::initialize(o2::framework::InitContext& /*ctx*/)
mHistClassRate[k]->Draw();
mHistClassRate[k]->SetBit(TObject::kCanDelete);
}*/
getObjectsManager()->startPublishing(mTCanvasClassRates[j]);
getObjectsManager()->startPublishing<true>(mTCanvasClassRates[j]);
}
}

Expand Down Expand Up @@ -136,7 +136,7 @@ void CTPCountersTask::initialize(o2::framework::InitContext& /*ctx*/)
mHistClassTotalCounts[i]->Draw();
mHistClassTotalCounts[i]->SetBit(TObject::kCanDelete);
}
getObjectsManager()->startPublishing(mTCanvasTotalCountsClasses);
getObjectsManager()->startPublishing<true>(mTCanvasTotalCountsClasses);
}
}

Expand Down
2 changes: 1 addition & 1 deletion Modules/Example/src/EveryObject.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void EveryObject::initialize(o2::framework::InitContext& /*ctx*/)
mTCanvasMembers[i]->Draw();
mTCanvasMembers[i]->SetBit(TObject::kCanDelete);
}
getObjectsManager()->startPublishing(mTCanvas, PublicationPolicy::Forever);
getObjectsManager()->startPublishing<true>(mTCanvas, PublicationPolicy::Forever);
}
}

Expand Down
4 changes: 2 additions & 2 deletions Modules/GLO/src/DataCompressionQcTask.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ void DataCompressionQcTask::initialize(o2::framework::InitContext&)
mEntropyCompressionCanvas->DivideSquare(mCompressionHists.size());
mCompressionCanvas->DivideSquare(mCompressionHists.size());

getObjectsManager()->startPublishing(mEntropyCompressionCanvas.get());
getObjectsManager()->startPublishing(mCompressionCanvas.get());
getObjectsManager()->startPublishing<true>(mEntropyCompressionCanvas.get());
getObjectsManager()->startPublishing<true>(mCompressionCanvas.get());
}
}

Expand Down
2 changes: 1 addition & 1 deletion Modules/HMPID/src/HmpidTask.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ void HmpidTask::initialize(o2::framework::InitContext& /*ctx*/)

// Error messages
CheckerMessages = new TCanvas("CheckerMessages");
getObjectsManager()->startPublishing(CheckerMessages);
getObjectsManager()->startPublishing<true>(CheckerMessages);

// TH2 to check HV
hCheckHV = new TH2F("hCheckHV", "hCheckHV", 42, -0.5, 41.5, 4, 0, 4);
Expand Down
2 changes: 1 addition & 1 deletion Modules/MUON/MCH/src/PedestalsTask.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ void PedestalsTask::initialize(o2::framework::InitContext& /*ctx*/)
}

mCanvasCheckerMessages = std::make_unique<TCanvas>("CheckerMessages", "Checker Messages", 800, 600);
getObjectsManager()->startPublishing(mCanvasCheckerMessages.get());
getObjectsManager()->startPublishing<true>(mCanvasCheckerMessages.get());

mPrintLevel = 0;
}
Expand Down
2 changes: 1 addition & 1 deletion Modules/TPC/src/Clusters.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void Clusters::initialize(InitContext& /*ctx*/)
addAndPublish(getObjectsManager(), mTimeBinCanvasVec, { "c_Sides_Time_Bin", "c_ROCs_Time_Bin_1D", "c_ROCs_Time_Bin_2D" });

for (auto& wrapper : mWrapperVector) {
getObjectsManager()->startPublishing(&wrapper);
getObjectsManager()->startPublishing<true>(&wrapper);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Modules/TPC/src/JunkDetection.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void JunkDetection::initialize(o2::framework::InitContext&)
mJDHistos.emplace_back(new TH2F("h_removed_Strategy_B", "Removed Strategy (B)", 1, 0, 1, 1, 0, 1)); // dummy for the objectsManager

if (!mIsMergeable) {
getObjectsManager()->startPublishing(mJDCanv.get());
getObjectsManager()->startPublishing<true>(mJDCanv.get());
}
for (const auto& hist : mJDHistos) {
getObjectsManager()->startPublishing(hist);
Expand Down
2 changes: 1 addition & 1 deletion Modules/TPC/src/PID.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ void PID::initialize(o2::framework::InitContext& /*ctx*/)
}
for (auto const& pair : mQCPID.getMapOfCanvas()) {
for (auto& canv : pair.second) {
getObjectsManager()->startPublishing(canv.get());
getObjectsManager()->startPublishing<true>(canv.get());
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Modules/TPC/src/RawDigits.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void RawDigits::initialize(o2::framework::InitContext& /*ctx*/)
addAndPublish(getObjectsManager(), mTimeBinCanvasVec, { "c_Sides_Time_Bin", "c_ROCs_Time_Bin_1D", "c_ROCs_Time_Bin_2D" });

for (auto& wrapper : mWrapperVector) {
getObjectsManager()->startPublishing(&wrapper);
getObjectsManager()->startPublishing<true>(&wrapper);
}
}

Expand Down
2 changes: 1 addition & 1 deletion Modules/TPC/src/Utility.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void addAndPublish(std::shared_ptr<o2::quality_control::core::ObjectsManager> ob
for (const auto& canvName : canvNames) {
canVec.emplace_back(std::make_unique<TCanvas>(canvName.data()));
auto canvas = canVec.back().get();
objectsManager->startPublishing(canvas);
objectsManager->startPublishing<true>(canvas);
if (metaData.size() != 0) {
for (const auto& [key, value] : metaData) {
objectsManager->addMetadata(canvas->GetName(), key, value);
Expand Down