-
Notifications
You must be signed in to change notification settings - Fork 147
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
base: master
Are you sure you want to change the base?
Conversation
For the record, we discussed in person, the agreed improvements are:
|
Also marked all unwanted occurances of mergeable with ignore flag
I think that I addressed everything that we talked about, but a bit different way. If you would prefer something else, I can do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just some details to sort out, but in general the idea is good.
Do I understand correctly that the inclusion order of ObjectManager.h
and PostProcessingInterface.h
should not matter because the template is put in place after both?
Framework/src/ObjectsManager.cxx
Outdated
@@ -70,6 +70,9 @@ void ObjectsManager::startPublishing(TObject* object, PublicationPolicy publicat | |||
ILOG(Warning, Support) << "Object is already being published (" << object->GetName() << "), will remove it and add the new one" << ENDM; | |||
stopPublishing(object->GetName()); | |||
} | |||
if (mergers::isMergeable(object)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be also disabled by the presence of QUALITYCONTROL_POSTPROCESSINTERFACE_H
and IgnoreMergable==true
, otherwise we will see a lot of spurious warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justonedev1 can you explain how your most recent changes make QUALITYCONTROL_POSTPROCESSINTERFACE_H
disable this warning? I'm not convinced they do.
Framework/src/ObjectsManager.cxx
Outdated
@@ -70,6 +70,9 @@ void ObjectsManager::startPublishing(TObject* object, PublicationPolicy publicat | |||
ILOG(Warning, Support) << "Object is already being published (" << object->GetName() << "), will remove it and add the new one" << ENDM; | |||
stopPublishing(object->GetName()); | |||
} | |||
if (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 might cause issues during publishing"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ILOG(Warning, Support) << "Object '" + std::string(object->GetName()) + "' with type '" + std::string(object->ClassName()) + "' is not one of the mergeable types, it might cause issues during publishing"; | |
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; |
missing ENDM and rephrasing
@@ -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); |
There was a problem hiding this comment.
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.
this is just a draft with an idea, how to do it compile time, I am not sure, that you like it. It does not build, it would need a bit more work. Otherwise I don't know how to do this compile time and it would need to be done run time via
dynamic_cast