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

DT calibration updates2019 #26201

Merged
merged 33 commits into from Apr 2, 2019
Merged

DT calibration updates2019 #26201

merged 33 commits into from Apr 2, 2019

Conversation

saghosh
Copy link
Contributor

@saghosh saghosh commented Mar 16, 2019

PR description:

The code changes include updates for the DT Calibration workflow, to merge the working version of the workflow with CMSSW and update the T0 calibration code, include muon matching and improve the workflow in general.

PR validation:

The code changes have been validated against the private version of the code used for the DT Calibration and found to be compatible.

The code has been validated using the basic tests:
runTheMatrix.py -l limited -i all --ibeos
the tests ran successfully with the final report produced runall-report-step123-.log with the final line being 👎
32 31 30 24 12 2 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 failed

The code was checked with the command:
scram build code-checks
and the suggested changes have been added as the final commit

Additional information

We do not require a backport, since we have a private version that we can use with git cms-merge-topic for the previous versions of CMSSW.

The code changes were built on top on CMSSW_10_5_0 since that is the latest stable release that we could find at the time of proposing the changes and the PR.

prebello and others added 29 commits February 22, 2019 09:33
[105X] removing three fragments from override (backport of #25955)
…oCond GT instead of hardcoded prompt GT for heavy Ion data relval
Backport : Update GT for ultra legacy - MC ECAL, GEM eMap
Since 2017 a new unpacker for the digis is used. If the option
"run-on-RAW" is used the digis need to be unpacked from the raw
data. Now the correct new unpacker is loaded when using this option.

(cherry picked from commit 3fa9140)
Bevor every analyzed event was printed in the cerr. Now it prints
less often with increasing time between two messages.

(cherry picked from commit f21547b)
Moved a lot of messages to debug level (like booking histos).
Results in much less output.

(cherry picked from commit e1bc42d)
I re-worked the T0 calibration. The first working version (lot of
commented code which will be removed) is included in this commit.
It is also now integrated in the DTWorkflow. You can run is using:
dtCalibration T0Wire all all --label LABEL --trial TRIAL --run=RUN
--globaltag=GLOBALTAG --datasetpath=/MiniDaq/ERA/RAW

(cherry picked from commit c6c6739)
crab submit --wait seems not to wait any more. Therefore I wait a
little bit longer using sleep in order to not crash when checking
the status.

(cherry picked from commit a9433c2)
A consumes needeed to be added in order to run in CMSSW10.

(cherry picked from commit dcff4bc)
The Mean Timer can now be submitted using the workflow tool. As I
do not fully understand the vdrift meantimer code I did not check
if everything works correctly (e.g. importing ttrig dbs).

(cherry picked from commit bb54b38)
One may want to rebase this commit.

(cherry picked from commit ce787b6)
- Remove debug output
- Rename some variables
- Remove old files from config

(cherry picked from commit aa8e565)
The history is kept.

(cherry picked from commit 69ccb11)
This exception was not definied before.

(cherry picked from commit 2080235)
I forgot to change class name, log files and header name imports
from DTT0CalibrationNew to DTT0Calibration.

(cherry picked from commit 0b76d7d)
Changed dtT0CalibrationNew to dtT0Calibration.

(cherry picked from commit afe5dbe)
Bug fixing of smart pointers. Could be rebased behind the cherry pick
merge commit.

(cherry picked from commit 90afc1f)
@cmsbuild cmsbuild added this to the CMSSW_10_6_X milestone Mar 16, 2019
@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 27, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33800/console Started: 2019/03/27 15:37

@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-26201/33800/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3114829
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3114631
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@andrius-k
Copy link

+1

@saghosh
Copy link
Contributor Author

saghosh commented Mar 31, 2019

Hello reviewers, could you please let me know if there is anything further to be done from my side to progress about this Pull Request. Thanks!

@pohsun
Copy link

pohsun commented Mar 31, 2019

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

process.load("DQMServices.Core.DQM_cfg")

process.source = cms.Source("PoolSource",
fileNames = cms.untracked.vstring()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saghosh is this supposed to be just a template?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos yes, this is a template that is imported in the workflow and then certain variables are updated as required for a specific action in the workflow

@@ -7,6 +7,10 @@
process.MessageLogger.destinations = cms.untracked.vstring('cerr')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saghosh if running on top of CMSSW_10_6_X_2019-04-01-1100 this cfg fails:

----- Begin Fatal Exception 01-Apr-2019 17:42:55 CEST-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing global begin Run run: 312774
   [1] Calling method for module DTDigiTask/'dtTPmonitor'
Exception Message:
No "MuonGeometryRecord" record found in the EventSetup.n
 The Record is delivered by an ESSource or ESProducer but there is no valid IOV for the synchronizatio value.
 Please check 
   a) if the synchronization value is reasonable and report to the hypernews if it is not.
   b) else check that all ESSources have been properly configured. 
----- End Fatal Exception -------------------------------------------------

Did you observe this in your tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saghosh Global tag is missing in the config file. And it doesn't look like GT will be configured by some external parameter either.
You may consider using
process.GlobalTag = GlobalTag(process.GlobalTag, 'auto:run2_data', '')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos @tocheng We tested this code as a part of our workflow for the calibration, and we did not see the error messages, mainly because this code is not used as in by running something like cmsRun, but actually the calibration workflow uses this file as a template and also adds the dataset and globalTag information in subsequent steps before the creating a final config file based on the user input that runs without any problems.
Also, please note, this file is not meant to be run on collisions data but is used for the T0 calibration that uses special test pulse runs. I hope this clarifies your comments, please let me know if the clarification is not satisfactory...

@fabiocos
Copy link
Contributor

fabiocos commented Apr 2, 2019

@saghosh I usually give a try to test cfgs to see whether they are functional, in this case I understand they are not supposed to be used out of the box. Fine for me, as you say that this was tested in the real workflow where it is supposed to serve (still based on cmsRun I assume, but not in the regular production workflows)

@fabiocos
Copy link
Contributor

fabiocos commented Apr 2, 2019

+1

@cmsbuild cmsbuild merged commit 99cd017 into cms-sw:master Apr 2, 2019
@fabiocos
Copy link
Contributor

fabiocos commented Apr 2, 2019

As far as I can see cms-merge-topic has been used in the preparation of this PR, unfortunately I caught it late. This should NOT be done, please see https://cms-sw.github.io/tutorial-resolve-conflicts.html .
The consequences in this case look limited, but this may spoil anyway the history.

@saghosh
Copy link
Contributor Author

saghosh commented Apr 3, 2019 via email

@fabiocos
Copy link
Contributor

fabiocos commented Apr 4, 2019

@saghosh well b7731d0 is a merge that should not be picked, it is more the commits used to make it that should be used...

@saghosh
Copy link
Contributor Author

saghosh commented Apr 4, 2019

@fabiocos okay, I see what you mean, sorry but it was not part of our original PR, it was added after suggestions from the cmsbuild bot here #26201 (comment) . Sorry about this, I will make sure to do it another way in the future.

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

8 participants