Add warning that lists legacy modules if any are configured. #34577

wddgit · 2021-07-21T18:28:22Z

PR description:

This PR adds a warning that lists legacy modules that are configured in the current job. They print through the MessageLogger at the LogSystem level. Nothing prints if there are none. It looks like this:

%MSG-s LegacyModules:  AfterModConstruction 21-Jul-2021 20:10:16 CEST pre-events
The following legacy modules are configured. Support for legacy modules
is going to end soon. These modules need to be converted to have type
edm::global, edm::stream, edm::one, or in rare cases edm::limited.
  SecondaryProducer Thing
  EventContentAnalyzer Analysis
%MSG

This PR is split into 3 commits (at least when initially submitted). The 3rd commit actually adds the new warning message. The first two commits deal with unit test failures that are a consequence of this addition to the log file output. Some tests compare log file output to a reference file and fail when they are different. These need to be fixed if they contain legacy types modules.

In the first, many modules in FWCore/MessageService are migrated from legacy type to global or one type. The behavior of the modules is not changed. There is a significant amount of cleanup mixed in here. None of this should change the behavior or output of any of the test modules.

There are 3 unit tests in FWCore/Integration that start to fail. The second commit fixes these, mostly by updating the reference files.

I did not do a survey to find all legacy modules in the Framework. I only looked at ones where the unit tests started to fail. Also note that we are intentionally leaving some legacy modules in the Framework to ensure legacy modules continue to work until the point in time where support for legacy modules is completely dropped. We want them to continue to function properly in the temporary period while we migrate away from them. I don't know how many legacy modules remain in the Framework and whether all of them are needed for this purpose.

PR validation:

Mostly relies on existing unit tests. Some of these were updated to keep them working. Three test were modified to check the output of this new warning by comparing a log file with a reference file containing the warning.

These are test modules in FWCore/MessageService. When run, the output log is compared as part of a unit test. These unit tests fail when the new warning is added that lists legacy modules that are configured.

These unit tests compare log files and fail when the new warning listing configured legacy modules is added.

cmsbuild · 2021-07-21T18:46:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34577/24128

This PR adds an extra 96KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/src/EventProcessor.cc modified in PR(s): Added WaitingTask Chain #34413
- File FWCore/MessageService/test/UnitTestClient_C.cc modified in PR(s): Add support of printf()-like formatting to MessageLogger #30629
- File FWCore/MessageService/test/UnitTestClient_G.cc modified in PR(s): Add support of printf()-like formatting to MessageLogger #30629

cmsbuild · 2021-07-21T18:46:40Z

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

FWCore/Framework (core)
FWCore/Integration (core)
FWCore/MessageService (core)

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

wddgit · 2021-07-21T18:47:20Z

please test

cmsbuild · 2021-07-22T00:27:09Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5158da/17069/summary.html
COMMIT: 673972e
CMSSW: CMSSW_12_0_X_2021-07-20-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34577/17069/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test RecoMTDDetLayersTestDriver had ERRORS

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

No significant changes to the logs found
Reco comparison results: 4210 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2996268
DQMHistoTests: Total failures: 12373
DQMHistoTests: Total nulls: 20
DQMHistoTests: Total successes: 2983853
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 45.699 KiB( 38 files compared)
DQMHistoSizes: changed ( 140.53 ): 44.531 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): 1.172 KiB RPC/DCSInfo
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: found differences in 1 / 38 workflows

makortel · 2021-07-22T00:32:47Z

The unit test failure is

 
===== Test "RecoMTDDetLayersTestDriver" ====
 testing RecoMTD/DetLayers
===== Test "cmsRun mtd_cfg.py" ====
The output diff is 372:
1,7d0
< %MSG-s LegacyModules:  AfterModConstruction 21-Jul-2021 23:23:28 CEST pre-events
< The following legacy modules are configured. Support for legacy modules
< is going to end soon. These modules need to be converted to have type
< edm::global, edm::stream, edm::one, or in rare cases edm::limited.
<   MTDRecoGeometryAnalyzer prod
<   TestETLNavigation prod1
< %MSG
Failure in comparison for mtdDetLayerGeometry.log: status 255

---> test RecoMTDDetLayersTestDriver had ERRORS
TestTime:54
^^^^ End Test RecoMTDDetLayersTestDriver ^^^^

@fabiocos Can these two modules be converted to more efficient ones quickly, or shall we add this message as part of the reference log (that then has to be changed again when these modules will be converted)?

makortel · 2021-07-22T00:35:05Z

On the comparison differences, workflow 9.0 is showing differences across the board. I'm wondering if this could be related to #34448. I can't think of how this PR could cause those.

makortel · 2021-07-22T00:42:43Z

@cmsbuild, please test

Let's see if the 9.0 differences persist

cmsbuild · 2021-07-22T08:57:42Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5158da/17075/summary.html
COMMIT: 673972e
CMSSW: CMSSW_12_0_X_2021-07-21-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34577/17075/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test RecoMTDDetLayersTestDriver had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2996268
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2996239
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

wddgit · 2021-07-22T20:02:42Z

@makortel @fabiocos

It looks to me like these two modules can be converted to global types. I will just go ahead do that myself so we can get this PR merged as soon as possible. I'll add it as a commit to this PR.

wddgit · 2021-07-23T15:48:00Z

please test

cmsbuild · 2021-07-23T21:11:03Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5158da/17152/summary.html
COMMIT: b848186
CMSSW: CMSSW_12_0_X_2021-07-22-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34577/17152/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test trackerMaterialAnalysisPlots had ERRORS
---> test materialBudgetTrackerPlots had ERRORS
---> test materialBudgetHGCalPlots had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2998564
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2998541
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

wddgit · 2021-07-23T21:14:15Z

The unit test failures in the last test are also occurring in the IBs. The unit test failure in the initial round of testing is fixed now.

makortel · 2021-07-23T21:36:49Z

+1

wddgit · 2021-07-23T21:50:01Z

The part of this PR outside of the Core involves the conversion of two test modules from legacy type to global type (plus in one of them I fixed a minor memory leak). Most of this PR is changes in the Core code.

fabiocos · 2021-07-25T09:01:23Z

@makortel @wddgit thanks for addressing this, I did not previously bother much about the efficiency of this code as this is just a standalone test.

slava77 · 2021-07-26T13:48:55Z

RecoMTD/DetLayers/test/MTDRecoGeometryAnalyzer.cc

 #include <sstream>

 #include "CLHEP/Random/RandFlat.h"

 using namespace std;
 using namespace edm;

-class MTDRecoGeometryAnalyzer : public EDAnalyzer {
+class MTDRecoGeometryAnalyzer : public global::EDAnalyzer<> {


this module is using CLHEP::RandFlat::shoot and I do not see HepRandomEngine
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEDMRandomNumberGeneratorService
I'm not sure it's a good idea to have a global module in this case.

I thought about this, but if you look here:
https://gitlab.cern.ch/CLHEP/CLHEP/-/blob/develop/Random/src/Random.cc

You see the following line of code, which makes the engines thread local
(unless CLHEP_THREAD_LOCAL is somewhere defined as not thread local
and I think other things would be broken badly if that were the case).

static CLHEP_THREAD_LOCAL defaults* theDefaults = defaultsForAllThreads.createNewDefaults();

If the engines are thread locals. there is no data race issue. It's OK
for them to be used by global modules.

This test module is not using the random number service. That would
break replay and it means it's not getting seeds from the service, but that
is all independent of whether or not this should be global or one.
It could have issues with the seeds not generating independent
sequences...

That said, I could change it to "one" just to avoid thinking about this.
The test modules are used in only one single threaded test. At some
level, it doesn't matter which way we go.

To be clear, this is not supposed to enter any production workflow, it is just a standalone test. The random numbers are not supposed to compete with any other random number source as far as I can see

Yes. Given that the module is only used in a standalone unit test, it isn't worth the effort to fix it to use the random service...

If someone still prefers this to be a "one", let me know and I will change it (it wouldn't take more than a few minutes). I'm most interested in getting the PR merged quickly and don't really have a preference whether it is "one" or "global". My main concern is getting the unit test to pass.

slava77 · 2021-07-26T21:58:10Z

+reconstruction

for #34577 b848186

code changes in reco packages are only in RecoMTD/DetLayers/test, they appear to be acceptable after the clarifications in Add warning that lists legacy modules if any are configured. #34577 (review)
jenkins tests pass and comparisons with the baseline show no differences and the MTD unit (RecoMTDDetLayersTestDriver) test apparently passes

srimanob · 2021-07-27T06:56:50Z

+Upgrade

The failure in the unit test is not related to this PR.

cmsbuild · 2021-07-27T06:57:13Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-07-27T07:21:37Z

+1

perrotta · 2021-07-27T07:25:19Z

merge
(Failing unit tests do not seem related to this PR. Validation/Geometry/test/materialBudgetHGCalPlots and src/Validation/Geometry/test/materialBudgetTrackerPlots are already giving segmentation fault in the IBs, while SimTracker/TrackerMaterialAnalysis/test/trackerMaterialAnalysisPlots although not failing in the IBs looks similar)

wddgit added 3 commits July 21, 2021 18:20

Convert some modules from legacy to global or one

8b4562f

These are test modules in FWCore/MessageService. When run, the output log is compared as part of a unit test. These unit tests fail when the new warning is added that lists legacy modules that are configured.

Update unit tests so they will pass

82fc9f2

These unit tests compare log files and fail when the new warning listing configured legacy modules is added.

Add warning message with configured legacy modules

673972e

cmsbuild added this to the CMSSW_12_0_X milestone Jul 21, 2021

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Jul 21, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 21, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 21, 2021

cmsbuild mentioned this pull request Jul 21, 2021

support global::OutputModule<ExternalWork> #34579

Merged

cmsbuild added tests-rejected and removed tests-started labels Jul 22, 2021

makortel mentioned this pull request Jul 22, 2021

Results for Run-1 SIM step are sensitive to order of module execution #34448

Open

cmsbuild added tests-started and removed tests-rejected labels Jul 22, 2021

cmsbuild added tests-rejected and removed tests-started labels Jul 22, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 23, 2021

cmsbuild added tests-rejected and removed tests-started labels Jul 23, 2021

cmsbuild added core-approved and removed core-pending labels Jul 23, 2021

makortel mentioned this pull request Jul 24, 2021

Issue LogError for legacy EDModules cms-sw/framework-team#98

Closed

slava77 reviewed Jul 26, 2021

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jul 26, 2021

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Jul 27, 2021

cmsbuild added orp-approved and removed orp-pending labels Jul 27, 2021

cmsbuild merged commit e207da6 into cms-sw:master Jul 27, 2021

cmsbuild mentioned this pull request Jul 27, 2021

Ecal laser update: Changed a system("mv") by a rename() and removed second attempt to read matacq file to speed up the processing when the file is missing #34623

Merged

wddgit deleted the addLegacyWarning branch November 16, 2021 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning that lists legacy modules if any are configured. #34577

Add warning that lists legacy modules if any are configured. #34577

wddgit commented Jul 21, 2021

cmsbuild commented Jul 21, 2021

cmsbuild commented Jul 21, 2021

wddgit commented Jul 21, 2021

cmsbuild commented Jul 22, 2021

makortel commented Jul 22, 2021

makortel commented Jul 22, 2021 •

edited

Loading

makortel commented Jul 22, 2021

cmsbuild commented Jul 22, 2021

wddgit commented Jul 22, 2021

wddgit commented Jul 23, 2021

cmsbuild commented Jul 23, 2021

wddgit commented Jul 23, 2021

makortel commented Jul 23, 2021

wddgit commented Jul 23, 2021

fabiocos commented Jul 25, 2021

slava77 Jul 26, 2021

wddgit Jul 26, 2021

fabiocos Jul 26, 2021

wddgit Jul 26, 2021

slava77 commented Jul 26, 2021

srimanob commented Jul 27, 2021

cmsbuild commented Jul 27, 2021

perrotta commented Jul 27, 2021

perrotta commented Jul 27, 2021

Add warning that lists legacy modules if any are configured. #34577

Add warning that lists legacy modules if any are configured. #34577

Conversation

wddgit commented Jul 21, 2021

PR description:

PR validation:

cmsbuild commented Jul 21, 2021

cmsbuild commented Jul 21, 2021

wddgit commented Jul 21, 2021

cmsbuild commented Jul 22, 2021

Unit Tests

Comparison Summary

makortel commented Jul 22, 2021

makortel commented Jul 22, 2021 • edited Loading

makortel commented Jul 22, 2021

cmsbuild commented Jul 22, 2021

Unit Tests

Comparison Summary

wddgit commented Jul 22, 2021

wddgit commented Jul 23, 2021

cmsbuild commented Jul 23, 2021

Unit Tests

Comparison Summary

wddgit commented Jul 23, 2021

makortel commented Jul 23, 2021

wddgit commented Jul 23, 2021

fabiocos commented Jul 25, 2021

slava77 Jul 26, 2021

Choose a reason for hiding this comment

wddgit Jul 26, 2021

Choose a reason for hiding this comment

fabiocos Jul 26, 2021

Choose a reason for hiding this comment

wddgit Jul 26, 2021

Choose a reason for hiding this comment

slava77 commented Jul 26, 2021

srimanob commented Jul 27, 2021

cmsbuild commented Jul 27, 2021

perrotta commented Jul 27, 2021

perrotta commented Jul 27, 2021

makortel commented Jul 22, 2021 •

edited

Loading