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

EMTF 2017 PR #6 - Emulator #20286

Merged
merged 20 commits into from Oct 8, 2017
Merged

Conversation

abrinke1
Copy link
Contributor

@abrinke1 abrinke1 commented Aug 28, 2017

4th and last part of set of PRs following PR #19741 , #19743 , and #20006
PR #19744 is no longer necessary
PR #19928 (CondDB and O2O) will no longer be necessary

Author comments: This PR is huge. This was inevitable, and unavoidable. The 2016 emulator (with data formats) had 143 files. The 2017 emulator (with helper files for trigger primitives) has 126. So that's 269 out of the 295 total files changed. The remainder come from moving some files out of a deprecate folder (which have been, and continue to be in use), and changes for the new O2O/CondFormats files which are inseparable from the new emulator.**

I've tried to make it quite simple to track which changes happened where by grouping them in easy-to-understand commits:
https://github.com/abrinke1/cmssw/commits/EMTF_PR_2017_93X_part8c

Of course, it's still a lot of code to review. I suppose the first thing, though, is to figure out what the conflicts are that prevent this branch from being merged in its current state. It merges, compiles, and runs fine on CMSSW_9_3_0_pre4.

Best regards,
Andrew

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @abrinke1 for master.

It involves the following packages:

CondCore/L1TPlugins
CondFormats/DataRecord
CondTools/L1TriggerExt
DQM/L1TMonitor
DQM/L1TMonitorClient
DataFormats/L1TMuon
EventFilter/L1TRawToDigi
L1Trigger/L1TCommon
L1Trigger/L1TMuon
L1Trigger/L1TMuonBarrel
L1Trigger/L1TMuonEndCap
L1TriggerConfig/L1TConfigProducers
L1TriggerConfig/Utilities

@ghellwig, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @rekovic, @franzoni, @ggovi, @vanbesien, @mulhearn, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @kreczko, @tocheng, @Martin-Grunewald, @thomreis, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@abrinke1
Copy link
Contributor Author

abrinke1 commented Aug 28, 2017

*** N.B. on the O2O/CondDB changes ***
Because the new emulator depends on pT assignment files loaded from the CondDB database, we could not pull in the new emulator without the O2O/CondDB changes. Because the emulator (including the old one) produces certain conditions, we could not pull in the new O2O/CondDB changes without changing the emulator. So in the end these had to go in together.

You may notice that one of the commits involves renaming some existing files. This was done to remedy a disastrous situation that was created unintentionally a year ago, where some files used the "L1TMuonEndCapParams" convention, and some used "L1TMuonEndcapParams". In fact, the situation was so confused that it was necessary to have duplicate DataRecord files, one set with "C", and one set with "c", for the code to compile and run without crashing:

http://cmslxr.fnal.gov/search?_filestring=L1TMuonEndCapParams&_string=&_casesensitive=1
http://cmslxr.fnal.gov/search?_filestring=L1TMuonEndcapParams&_string=&_casesensitive=1

This had not caused too many practical issues up to this point because the O2O/CondDB conditions were actually used by the EMTF emulator. But this emulator does, and we will most likely need to port more EMTF functionalities into O2O/CondDB in the future. This means that the code needs to be usable / developable by someone other than the person who wrote it. Right now (before this PR), that is not the case. And if we add the new O2O/CondDB pieces needed by the 2017 emulator without resolving the naming confusion, the situation will only get much worse. It's a bit of pain in tracking file changes now, in exchange for long-term sanity.

Even so, because of the Record name in the existing Global Tags, we need to keep two duplicate files and one extra line of code for things to run without crashing, for the moment:
https://cms-conddb-prod.cern.ch/cmsDbBrowser/search/Prod/L1TMuonEndCapParams

Once this PR is in, we can use it to create a new GT, at which point those lines/files can be deleted, and the code will assume a state of serene and beautiful simplicity. Or something like that :)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

Pull request #20286 was updated. @ghellwig, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @rekovic, @franzoni, @ggovi, @vanbesien, @mulhearn, @lpernie can you please check and sign again.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20286/353

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20286/354

@rekovic
Copy link
Contributor

rekovic commented Aug 29, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 29, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22576/console Started: 2017/08/29 19:06

@cmsbuild
Copy link
Contributor

-1

Tested at: 44af81d

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
5e6a21c
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20286/22576/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20286/22576/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20286/22576/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests ClangBuild

  • Unit Tests:

I found errors in the following unit tests:

---> test TestTrackTools had ERRORS
---> test TestL1TriggerL1TMuonEndCap had ERRORS

  • Clang:

I found a compilation error while trying to compile with clang:
I used this command:
scram b vclean && scram build -k -j 16 USER_CXXFLAGS='-fsyntax-only' COMPILER='llvm compile'

>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/MP7BufferDumpToRaw.cc 
2 warnings generated.
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/AMCDumpToRaw.cc 
2 warnings generated.
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/L1TDigiToRaw.cc 
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/L1Trigger/L1TMuonEndCap/test/tools/MakePtLUT.cc:125:19: error: comparison of integers of different signs: 'PtLUTWriter::address_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
  for ( ; address < abs(num_ * (PTLUT_SIZE / denom_)); ++address) {
          ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
2 warnings generated.
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/L1TCaloTowersFilter.cc 


The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
5e6a21c
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20286/22576/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20286/22576/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison job queued.

@jiafulow
Copy link
Contributor

Hi Andrew,

For the Unit Tests failure, the following should fix it:

diff --git a/L1TMuonEndCap/test/BuildFile.xml b/L1TMuonEndCap/test/BuildFile.xml
index cbf172a..9a10c30 100644
--- a/L1TMuonEndCap/test/BuildFile.xml
+++ b/L1TMuonEndCap/test/BuildFile.xml
@@ -1,9 +1,4 @@
 <environment>
-  <bin name="TestL1TriggerL1TMuonEndCap" file="unittests/TestDriver.cpp">
-    <flags TEST_RUNNER_ARGS=" /bin/bash L1Trigger/L1TMuonEndCap/test/unittests runtests.sh"/>
-    <use name="FWCore/Utilities"/>
-  </bin>
-
   <bin name="TestPhiMemoryImage" file="unittests/TestPhiMemoryImage.cpp">
     <use name="L1Trigger/L1TMuonEndCap"/>
     <use name="cppunit"/>
diff --git a/L1TMuonEndCap/test/unittests/TestTrackTools.cpp b/L1TMuonEndCap/test/unittests/TestTrackTools.cpp
index 8b4248a..19962c0 100644
--- a/L1TMuonEndCap/test/unittests/TestTrackTools.cpp
+++ b/L1TMuonEndCap/test/unittests/TestTrackTools.cpp
@@ -53,9 +53,9 @@ void TestTrackTools::test_theta()
 {
   for (int i = 0; i < 1801; ++i) {
     double theta = 0. + 0.1 * static_cast<double>(i);  // theta=[0,180,step=0.1]
-    int endcap = (theta >= 90.) ? 2 : 1;
+    int endcap = (theta >= 90.) ? -1 : 1;
     double eps = 0.28515625;
-    CPPUNIT_ASSERT_DOUBLES_EQUAL((endcap == 2 ? (180. - theta) : theta), calc_theta_deg_from_int(calc_theta_int(theta, endcap)), eps);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL((endcap == -1 ? (180. - theta) : theta), calc_theta_deg_from_int(calc_theta_int(theta, endcap)), eps);
   }
 }
 

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20286/23234/summary.html

Comparison Summary:

  • You potentially added 2990 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2691683
  • DQMHistoTests: Total failures: 1403
  • DQMHistoTests: Total nulls: 24
  • DQMHistoTests: Total successes: 2690067
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 15 edm output root files, 26 DQM output files

@abrinke1
Copy link
Contributor Author

@rekovic @davidlange6 @dmitrijus @Dr15Jones @smuzaffar
Excellent! Looks like all systems go from the comparison standpoint.

@rekovic
Copy link
Contributor

rekovic commented Sep 28, 2017

+1

@rekovic
Copy link
Contributor

rekovic commented Sep 28, 2017

To facilitate further review of the PR, here is the list of files that are simply moved from directory
L1Trigger/L1TMuonEndCap/src/ to its subdirectory bdt/ and their new paths are:

L1Trigger/L1TMuonEndCap/src/bdt/Forest.cc
L1Trigger/L1TMuonEndCap/src/bdt/Node.cc
L1Trigger/L1TMuonEndCap/src/bdt/Tree.cc
L1Trigger/L1TMuonEndCap/src/bdt/Utilities.cc

@rekovic
Copy link
Contributor

rekovic commented Sep 28, 2017

(thanks @abrinke1 )

@rekovic
Copy link
Contributor

rekovic commented Oct 2, 2017

@abrinke1
Can we also have a 92x version of this PR, please, so that we can run the tests. Thanks.

@abrinke1
Copy link
Contributor Author

abrinke1 commented Oct 2, 2017

@rekovic Do you have a suggestion on how to do this? I started cherry-picking my commits into a 92X release, but on the second commit, I got this message:

git cherry-pick 33fd87f
error: Commit 33fd87f is a merge but no -m option was given.
fatal: cherry-pick failed

Is there a standard way to back-port four separate PRs into an earlier CMSSW?

Also, any news on the code review? I would assume the deadline for MC production is pretty soon, and I doubt the analyses want last year's trigger in their MC.

@rekovic
Copy link
Contributor

rekovic commented Oct 7, 2017

For the record, a [92x] rebase is available in #20768.

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit b3b7d36 into cms-sw:master Oct 8, 2017
@Martin-Grunewald
Copy link
Contributor

Hi,
I believe this PR creates problems in TSG tests like this:

----- Begin Fatal Exception 09-Oct-2017 07:02:28 CEST-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing  Event run: 295606 lumi: 1 event: 37589 stream: 2
   [1] Running path 'L1RePack_step'
   [2] Calling method for module L1TMuonEndCapTrackProducer/'simEmtfDigis'
Exception Message:
No "L1TMuonEndCapParamsRcd" record found in the EventSetup for synchronization v
alue
Run: 295606 LuminosityBlock: 1 Event: 0 Time: 6425898591012852480
 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------

The above error usually points to the payload being in the DB, but not for the run in question!
This real-data test is running on old(er) 2017 data.

@abrinke1
Copy link
Contributor Author

abrinke1 commented Oct 9, 2017

Hi @Martin-Grunewald
Thanks for spotting this. Can you give us the command sequence or type of job you are running that generates this error?
Thanks,
Andrew

@Martin-Grunewald
Copy link
Contributor

My tests used the IB CMSSW_9_4_X_2017-10-08-2300 plus PR #20577. It looks like it this PR and #20577 step on each other, as the error is not present if I use just CMSSW_9_4_X_2017-10-08-2300
without adding #20577. I have thus re-triggered jenkins tests for #20577 which should now show the same errors, as the new jenkins tests will be based on CMSSW_9_4_X_2017-10-08-2300.

@Martin-Grunewald
Copy link
Contributor

Indeed, #20577 no longer passes tests. So I suppose a fix needs to be added there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet