DQM: new DQMStore. #28622

And implementation stubs as well. Needs lots of adaptions.

DQMGlobalEDAnalyzer might need an import from the new DQMStore branch.

... from the IO modules. Getting the semantics right will be future work.

Mostly to get QReports into MEData. More random changes to make things compile. DQMStore.h is now un-templated, since we don't need the templated declarations for now.

This should only be called when a new object is needed, and can freely modify the ROOT object.

Including the heterogeneous queries into the DQMStore sets.

This is the hard part -- at least half of it. This logic creates clones if and only if they are required. The other half is the cleanup code to run after saving, that needs to delete all clones but a prototype after saving. Not entirely sure yet what to do with harvesting -- we will have global and local MEs as well, but I am not sure yet if we should separate them by module ID. get*() will probably just return global MEs.

This includes adding a restriction to the tempalte callback argument for the booking calls to prevent ambiguities.

plus some bugfixes in other places. syncCoreObject is pretty inefficient, and in general I replaced many complicated, efficient things with slow, safe, value-based code. The hope is that the compiler will optimize away some of the values and that this code does not run much anyways. Later improvements are possible.

Not sure if it works at all, for now.

Since we can have MEs whithout data attached, we need to keep a copy of the name somewhere: Either by using a map or inside the ME. Since DQMNet::CoreObject already has the name, we also use it to store the path. Doing that with this optimization would be possible, but complicated, so I just removed it with the minimum amount of changes.

Incluing adding an API to MonitorElement to check if there actually if there actually is data inside it.

This completes the ME life cycle.

Using commit 70fc23c on his "new DQMstore" branch. This might revert changes from cms-sw#28064, needs to be fixed later.

Not tested yet.

... to make things acceptably fast. The make_tuple in MEComparison is another hotspot, not fixed here.

Including reScope for harvesting!

The main thing is to make sure that only Scope::RUN histograms are used in dqmEndRun. The changes here are more than required, but still not complete. In the longer term, we may want to restrict dqmEndRun to DQMOneEDAnalyzer and completely ban endJob in DQM*EDAnalyzer. The manual setScope would not be required then, since DQMOneEDAnalyzer would use RUN scope by default. Alternatively, we *could* support endRun in edm::stream, running it only on a single stream, but that is unnecessarily complicated.

This is needed if there are multiple runs, and we book multiple times, to make sure the old histograms are already prepared in booking.

Involves bugfixes in DQMStore as well as the DQMRootSource. The current cleanup logic is pretty much dommed to fail, and it is not really possible to get DummyReadDQMStore (relying on legacy emulation behavior) to work now. We could fix DummyReadDQMStore to not rely on the legacy-keep-all-pointers-avlive behavior, but actually we should keep supporting that for whatever else needs it.

This is the recommended way to do it, and works even if we allocate to many copies.

Since the MEs read in the input source don't have local MEs with them, the enterLumi logic for recycling does not apply. But we need to recycle them, to hold up the guarantees for legacy modules.

I don't see how the old behavior was ever useful. Also filterOnRun is unused apart from tests, it should be removed. The tests should rather test lumisToProcess, which is used in production.

The main change is to make DummyReadDQMStore less stupid so it does not rely on the sequential ordering of things. There is a semantic change in read_merged_file1_file3_file4_cfg.py, which expected run to be not merged, while the new code merges whatever makes sense.

This includes lots of automated changes to remove "empty" dqmBeginRun and dqmEndRun. dqmBeginRun remains allowed in DQMEDAnalyzer; it is called before booking and ME cannot (and must not) be accessed there. Any local member intialization should be fine to do there; it will be called for each of the edm::stream instancs once we switch back to stream. Technically all the code could simply be moved into bookHistograms, but having a separate beginRun makes more sense logically. Many dqmEndRun that only contain log messages and printouts where removed, even if they were more than basic debugging output. The remaining modules with non-trivial dqmEndRuns are converted to DQMOneEDAnalyzer. Commands like sed -i '/dqmBeginRun/,+3d' $(ag \ 'dqm(Begin|End)Run[^\n]*\n[^\n]*LogInfo[^\n]*\n\s*}' -l) were used to delete "useless" bodies.

We shuold eventually remove it entirely, but there are a few weird corner cases to think about.

Very inefficiently, but these things should be deprecated.

Else the cwd in IGetter and IBooker can get out of sync, since the DQMStore overrides are not always used.

And migrate all the usages.

They also don't seem very important.

Wonder how we got that far with this bug...

This covers all single- and multi run Harvesting. Easier to make it the default than hunt down all the places where the source is configured. Also worrying that no unit tests fail with this change.

It never worked, I suspect.

Maybe this fixes the default for harvesting jobs?

Mostly untested.

@makortel

Thanks @makortel for the tip

It turns out, cd actually does not cd. It is identical to setCurrentFolder.

It was never used here. The idea originates in product-based harvesting, where we'd copy-on-write histograms borrowed out of edm products. Since we still have the edm::Serivce<DQMStore>, this is not required now: we can have mutable 'products'.

In harvesting, we need to make sure verything stays at JOB Scope so things actually get saved to the legacy output file at endJob.

It is equivalent to the DQMRootSource in role. The default config always sets it to JOB, since that is what *all* existing workflows expect. The not-JOB option is only used for the unit tests. An issue is that harvesting code now can no longer tell if an ME was saved per lumi or not.

That the legacy output is not saved is arguably a bug, but it is planned like this: It could be fixed by booking into RUN scope in the test legacy modules. However, that is not what the legacy modules will do in practice. It would be nice to have an ouptut test for the Analyzer->TDirectory case which is what legacy modules will commonly do.

This is mostly to make the tests happy: When reading pre-run scalars from split DQMIO files, the result could be the value from any file, since it dependes on processing order. We can't do anything about that in general (we might have a job processing lumi 1 and 4, and another 2 and 3, and then merge the results; which value should be used?), but for the tests we can at least make the behaviour detetministic for the test case.

Apart from bug fixes in the tests, there are a few expected changes: - Output of legacy modules is JOB scope, so it will not be save to DQMIO. - DQMIO->TDirectory conversion needs reScope option. Other limitaitons existed before but are more clear now: - Values of scalar values that are not constant over a run/lumi split into JOBs are arbitrary. - Histograms filled per job or per lumi do depend on job splitting, since the number of beginRun/beginLumi transitions changes. New tests/checks cover fastHadd, concurrent lumis and in-order harvesting. The testing of actual histogram contents (correct merging, no double counting etc.) is still very limtied here; mostly these test ensure that there are no crashes and that objects make it into the respective output formats.

… it.

This is required for all legacy modules running outside harvesting.

@Dr15Jones

As suggested by @Dr15Jones. Also some comment cleanups.

That does not fix the unit test failure, though.

- Remove unused methods (also their bodies). - Restrict access, use friends (this needed also some changes in other places) - Provide a switchObject() to prevent a race when using forceReplace when booking. - Change mutable_ to a normal pointer. - Reorder declarations to make a bit more sense. Still many things to be improved (the API of MonitorElement is huge and messy) but better than before.

Not sure it the update() in beammonitor was really unnecessary, but getTH1() should also mark the ME as dirty.

This is not functionally required, but makes debugging much easier, in case a different thread continues using the IBooker while the first thread handles the exception.

Commits on Feb 4, 2020

Change std::cout to edm::LogInfo.

schneiml committed Feb 4, 2020

Configuration menu

View commit details

Copy full SHA for 2f51722

Browse repository at this point

Copy the full SHA

2f51722 View commit details

Browse the repository at this point in the history

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DQM: new DQMStore. #28622

DQM: new DQMStore. #28622

Commits on Feb 3, 2020

Commits on Feb 4, 2020