RFC: DQMStore index logic rewrite #22307

schneiml · 2018-02-22T17:16:20Z

This PR attempts to complete the complete the conversion from #22157, via #22218 and #22243 et. al. It eliminates streamId and enableMultiThread from the DQMStore.

This needs to be merged/cherry-picked with #22319 and #22364, it adds another change on top: re-doing the logic that queries MEs out of the set in the DQMStore.

It needs to be discussed if the handling of the runNumber is actually what we want.

This is likely to break multi-run harvesting and online (needs testing).

Also re-introduce the .cc file, implementations in the header are to likely to confuse the build system.

For now, step1 and Harvesting produce the same token, which is fine since we don't need to handle Sources and Harvesters in the same process yet. That way, the output modules will for sure wait for everything. There is a new type of token for the endRun, to make sure Lumi and Run transistions get ordered correctly.

Also protect the DQMStore using usesResource, since all of this is really dangerous.

Consume tokens, protect the DQMStore and watch all the transitions.

…bleMultiThread switch. Does not fix things, however.

cmsbuild · 2018-02-22T17:16:40Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-02-22T17:18:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22307/3508

cmsbuild · 2018-02-22T17:18:45Z

A new Pull Request was created by @schneiml (Marcel Schneider) for master.

It involves the following packages:

Configuration/PyReleaseValidation
DQMServices/Components
DQMServices/Core
DataFormats/Histograms
Validation/RecoTau

@smuzaffar, @prebello, @Dr15Jones, @vazzolini, @kmaeshima, @kpedro88, @fabozzi, @cmsbuild, @jfernan2, @GurpreetSinghChahal, @vanbesien, @dmitrijus can you please review it and eventually sign? Thanks.
@ghellwig, @barvic, @makortel, @felicepantaleo, @rovere, @Martin-Grunewald, @ebrondol this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2018-02-22T18:01:30Z

DQMServices/Components/plugins/MEtoEDMConverter.cc

@@ -81,6 +81,8 @@ MEtoEDMConverter::MEtoEDMConverter(const edm::ParameterSet & iPSet) :
  produces<MEtoEDM<TString>, edm::Transition::EndLuminosityBlock>(sName);

  consumesMany<DQMToken>();
+  consumesMany<DQMRunToken>();


These need to be

consumesMany<DQMToken, edm::InLumi>(); consumesMany<DQMRunToken, edm::InRun>();

in fact, I guess we could just use DQMToken for both Run and Lumi MEs, and let the framework differentiate based on the InLumi and InRun flags.

ok, let me try. Maybe this fixes our problmes...

obviously that would be too easy. Fixed, but #22281 continues...

Dr15Jones · 2018-02-22T18:03:38Z

DataFormats/Histograms/interface/DQMToken.h

@@ -25,5 +25,12 @@ class DQMToken
      DQMToken() {}
 };

+class DQMRunToken


There really is not a need for a seperate type just for Run.

@Dr15Jones I thought so. But having two produce calls with the same Tokens gave me an error, and this was the quick fix.

fwyzard · 2018-02-22T18:08:21Z

DQMServices/Components/plugins/MEtoEDMConverter.cc

@@ -81,6 +81,8 @@ MEtoEDMConverter::MEtoEDMConverter(const edm::ParameterSet & iPSet) :
  produces<MEtoEDM<TString>, edm::Transition::EndLuminosityBlock>(sName);

  consumesMany<DQMToken>();
+  consumesMany<DQMRunToken>();


in fact, I guess we could just use DQMToken for both Run and Lumi MEs, and let the framework differentiate based on the InLumi and InRun flags.

fwyzard · 2018-02-22T18:11:07Z

DQMServices/Core/src/DQMEDAnalyzer.cc

+    run.moduleCallingContext()->moduleDescription()->id());
+}
+
+void DQMEDAnalyzer::endRun(edm::Run const& run, edm::EventSetup const& setup) 


I would suggest to keep most methods - especially the empty ones - in the .h file.

I in general prefer that -- but if it gives me linker errors, I happily put them into a .cc file. My theory is that the build system gets more easily confused with implementation in headers (and honestly, I have no idea how this is supposed to work on the linker level, so putting back the .cc file was the obvious option).

No, the build system does not get "confused".

If you put the implementation of a method inside the class body, e.g.

class Class { public: void some_method() { ... } };

it is automatically inline, and should not create linker problems.

If you put non-inline out-of-body implementation, like

class Class { public: void some_method(); }; void Class::some_method() { ... }

in the .h file, then you do get one version in each .cc file that includes it, and the linker will rightfully compile.

One other thing to pay attention to is if you have virtual methods, you may need to implement at least one of them in a .cc file, so the compiler knows "where" to emit the vtable and the type information.

I would be curious what problem you ran into here, and if we can find a less drastic solution than moving all methods' implementations to the .cc file.

One other thing to pay attention to is if you have virtual methods, you may need to implement at least one of them in a .cc file, so the compiler knows "where" to emit the vtable and the type information.

This one. So my gut feeling of putting things into a .cc file was justifed.

And yes, this inline story works fine as long as the methods are not virtual. Then it gets interesting.

schneiml · 2018-02-22T18:11:28Z

DQMServices/Core/src/DQMEDAnalyzer.cc

+
+DQMEDAnalyzer::DQMEDAnalyzer() {
+  produces<DQMToken,edm::Transition::EndLuminosityBlock>();
+  produces<DQMRunToken,edm::Transition::EndRun>();


@Dr15Jones when I remove the Run in this line, I got an framework error, saying I cannot have more than one produce call for the same product. I guess I have to specify both transitions in one call?

Hmm. Our framework test works:

cmssw/FWCore/Integration/test/ThingProducer.cc

Line 14 in ede02fc

blToken_ = produces<ThingCollection, edm::Transition::BeginLuminosityBlock>("beginLumi");

However, that does use unique product instance names, which was not meant to be required.

fwyzard · 2018-02-22T18:12:37Z

DQMServices/Components/plugins/MEtoEDMConverter.cc

@@ -92,7 +94,7 @@ void
 MEtoEDMConverter::beginJob()
 {
  // Determine if we are running multithreading asking to the DQMStore. Not to be moved in the ctor
-  enableMultiThread_ = dbe->enableMultiThread_;
+  //enableMultiThread_ = dbe->enableMultiThread_;


just delete the commented-out lines (and drop the empty methods)

This commit says HACK in the message for a good reason. After #22218, enableMultiThread has exactly no reason for existence, but I'll delay removing it everywhere to after #22281 is resolved.

fwyzard · 2018-02-22T18:13:45Z

DQMServices/Components/plugins/MEtoEDMConverter.h

                                                     edm::EndLuminosityBlockProducer,
-                                                     edm::EndRunProducer>
+                                                     edm::EndRunProducer,
+                                                     edm::one::SharedResources>


what is the purpose of the SharedResources ?

I also do not understand its usage.

It gives the usesResource call, which is a quick way to get a global lock for legacy stuff. Exactly what legacy DQM needs.

Though ideally, the final version of this PR will simply make that all thread save and we can get rid of usesResource("DQMStore") everywhere.

fwyzard · 2018-02-22T18:16:50Z

I don't understand one thing: the DQMEDProducer now produces the token, and the METoEDM consumes it.
But what about the DQMEDHarvesters ?

They serve no purpose any longer, only cause confusion, and should be eradicated.

cmsbuild · 2018-02-23T19:04:10Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-02-23T19:06:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22307/3543

cmsbuild · 2018-02-23T19:06:49Z

Pull request #22307 was updated. @smuzaffar, @prebello, @Dr15Jones, @vazzolini, @kmaeshima, @kpedro88, @fabozzi, @cmsbuild, @jfernan2, @GurpreetSinghChahal, @vanbesien, @dmitrijus can you please check and sign again.

schneiml · 2018-02-23T19:14:22Z

This is still WIP and probably does not even compile yet.

... and fix the logic of it.

fwyzard · 2018-02-26T16:38:41Z

@schneiml , if you agree that this is superseded by #22319, could you close it ?

schneiml · 2018-02-27T08:48:42Z

@fwyzard they are rather diverged by now, because I was doing things that are no longer covered by the title. I'll close it, and make a new PR once theses things are done.

There might be more missing, per-lumi things, online, and multi-run harvesting. But the usual sequences run fine, now multi-threaded.

cmsbuild · 2018-02-27T11:09:14Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-02-27T11:11:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22307/3582

cmsbuild · 2018-02-27T11:11:49Z

Pull request #22307 was updated. @smuzaffar, @prebello, @Dr15Jones, @vazzolini, @kmaeshima, @kpedro88, @fabozzi, @cmsbuild, @jfernan2, @GurpreetSinghChahal, @vanbesien, @dmitrijus can you please check and sign again.

Dmitrijus Bugelskis and others added 8 commits February 21, 2018 17:05

Disable multi thread for ALCAHARVEST.

ace8ec6

Merged refs/pull/22285/head from repository cms-sw with cms-merge-topic

d724709

Switch to EDProducer.

6ea3613

Also re-introduce the .cc file, implementations in the header are to likely to confuse the build system.

Another DQMEDAnalyzer config change

85a9c26

Convert EDMtoMEConverter to a producer, similar to DQMEDHarvester.

fdbe8d7

Also protect the DQMStore using usesResource, since all of this is really dangerous.

Some formal changes to MEtoEDMConverter.

ebe0a87

Consume tokens, protect the DQMStore and watch all the transitions.

HACK: make MEtoEDMConverter not delete things, and force-off that ena…

764b4a6

…bleMultiThread switch. Does not fix things, however.

cmsbuild added this to the CMSSW_10_1_X milestone Feb 22, 2018

cmsbuild added code-checks-pending comparison-pending core-pending dqm-pending orp-pending pdmv-pending pending-signatures tests-pending upgrade-pending labels Feb 22, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 22, 2018

schneiml mentioned this pull request Feb 22, 2018

DQMEDHarvesters seg faulting in nightly IB #22281

Closed

Dr15Jones reviewed Feb 22, 2018

View reviewed changes

fwyzard suggested changes Feb 22, 2018

View reviewed changes

schneiml commented Feb 22, 2018

View reviewed changes

fwyzard reviewed Feb 22, 2018

View reviewed changes

schneiml added 3 commits February 23, 2018 18:23

Force-0 streamId.

ab2ae8d

They serve no purpose any longer, only cause confusion, and should be eradicated.

Add new logic to getAllContents()

e9c57cc

Completely eradicate the streamId

a9a73cf

cmsbuild added code-checks-pending comparison-pending tests-pending and removed code-checks-approved comparison-notrun tests-rejected labels Feb 23, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 23, 2018

Replace custom logic in save() with getAllContents()

cbaa098

... and fix the logic of it.

schneiml closed this Feb 27, 2018

Fix more cases in the getAllContents logic.

af9c2d6

There might be more missing, per-lumi things, online, and multi-run harvesting. But the usual sequences run fine, now multi-threaded.

schneiml reopened this Feb 27, 2018

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 27, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 27, 2018

schneiml changed the title ~~RFC: DQM Step one migration to DQMToken~~ RFC: DQMStore index logic rewrite Feb 27, 2018

schneiml closed this Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: DQMStore index logic rewrite #22307

RFC: DQMStore index logic rewrite #22307

schneiml commented Feb 22, 2018 •

edited

cmsbuild commented Feb 22, 2018

cmsbuild commented Feb 22, 2018

cmsbuild commented Feb 22, 2018

Dr15Jones Feb 22, 2018

fwyzard Feb 22, 2018

schneiml Feb 22, 2018

schneiml Feb 22, 2018

Dr15Jones Feb 22, 2018

schneiml Feb 22, 2018

fwyzard Feb 22, 2018

fwyzard Feb 22, 2018

schneiml Feb 22, 2018

fwyzard Feb 23, 2018

schneiml Feb 23, 2018

schneiml Feb 22, 2018 •

edited

Dr15Jones Feb 22, 2018

fwyzard Feb 22, 2018

schneiml Feb 22, 2018

fwyzard Feb 22, 2018

Dr15Jones Feb 22, 2018

schneiml Feb 22, 2018

fwyzard commented Feb 22, 2018

cmsbuild commented Feb 23, 2018

cmsbuild commented Feb 23, 2018

cmsbuild commented Feb 23, 2018

schneiml commented Feb 23, 2018

fwyzard commented Feb 26, 2018

schneiml commented Feb 27, 2018

cmsbuild commented Feb 27, 2018

cmsbuild commented Feb 27, 2018

cmsbuild commented Feb 27, 2018

RFC: DQMStore index logic rewrite #22307

RFC: DQMStore index logic rewrite #22307

Conversation

schneiml commented Feb 22, 2018 • edited

cmsbuild commented Feb 22, 2018

cmsbuild commented Feb 22, 2018

cmsbuild commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneiml Feb 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard commented Feb 22, 2018

cmsbuild commented Feb 23, 2018

cmsbuild commented Feb 23, 2018

cmsbuild commented Feb 23, 2018

schneiml commented Feb 23, 2018

fwyzard commented Feb 26, 2018

schneiml commented Feb 27, 2018

cmsbuild commented Feb 27, 2018

cmsbuild commented Feb 27, 2018

cmsbuild commented Feb 27, 2018

schneiml commented Feb 22, 2018 •

edited

schneiml Feb 22, 2018 •

edited