Modernize `CondTools/SiPixel` #35495

mmusich · 2021-09-30T18:06:45Z

PR description:

Folllow-up to #35421:

moved plugin modules from test to plugins folder;
migrate away from deprecated edm::EDAnalyzer ;
migrate to esConsumes;
clean-up of some unused methods;
removal of SealModules file
remove couts in favour of MessageLogger

Still possible improvements:

merge of header files within the implementation file;
introduce more unit test to cover the plugins;

PR validation:

It compiles.

if this PR is a backport please specify the original PR and why you need to backport that PR:

N/A

cmsbuild · 2021-09-30T18:14:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35495/25665

This PR adds an extra 212KB to repository
Found files with invalid states:
- CondTools/SiPixel/plugins/SealModules.cc:
  - Added: 21c8b3e
  - Deleted: 1e90ff6

cmsbuild · 2021-09-30T18:14:48Z

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

CondTools/SiPixel (db)

@ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please review it and eventually sign? Thanks.
@VinInn, @OzAmram, @ferencek, @mmusich, @dkotlins, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2021-09-30T18:26:54Z

@cmsbuild, please test

cmsbuild · 2021-09-30T18:36:38Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54096c/19297/summary.html
COMMIT: 1e90ff6
CMSSW: CMSSW_12_1_X_2021-09-30-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35495/19297/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 1 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54096c/19297/llvm-analysis/esrget-sa.txt for details.
CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54096c/19297/llvm-analysis/legacy-mod-sa.txt for details.

Build

I found compilation error when building:

>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelTemplateDBObjectReader.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelVCalDB.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelVCalReader.cc
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelVCalReader.h:35,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelVCalReader.cc:1:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/poison/CondTools/SiPixel/test/SiPixelVCalDB.h:1:2: error: #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
    1 | #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
      |  ^~~~~
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelVCalReader.cc:1:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-30-1100/src/CondTools/SiPixel/plugins/SiPixelVCalReader.h:47:19: error: 'TrackerGeometry' was not declared in this scope
   47 |   edm::ESGetToken tkGeomToken_;

tvami · 2021-09-30T18:48:50Z

I'd add to the todo list of the upcoming PR that unit tests should be added to ensure that these are running fine

tvami · 2021-09-30T18:49:31Z

FYI @SanjanaSekhar

cmsbuild · 2021-09-30T20:16:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35495/25672

This PR adds an extra 72KB to repository

cmsbuild · 2021-09-30T20:16:34Z

Pull request #35495 was updated. @ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please check and sign again.

mmusich · 2021-09-30T20:21:10Z

I found compilation error when building:

actually not sure why this happened. Locally it compiles.

mmusich · 2021-09-30T20:28:46Z

I can reproduce with:

cmsrel CMSSW_12_1_X_2021-09-30-1100/
cd CMSSW_12_1_X_2021-09-30-1100/src/
cmsenv
git cms-merge-topic 35495
scramv1 b -j 20

how does one get rid of the poisoned files?

smuzaffar · 2021-09-30T20:40:14Z

@mmusich , one of the file still use the deleted header

./CondTools/SiPixel/plugins/SiPixelVCalReader.h:#include "CondTools/SiPixel/test/SiPixelVCalDB.h"

cmsbuild · 2021-10-02T12:26:14Z

Pull request #35495 was updated. @ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please check and sign again.

mmusich · 2021-10-02T13:48:01Z

@perrotta all review comments have been addressed.

@tvami's #35495 (comment) about unexpected printouts in TESTING SiPixelLorentzAngle DB codes is addressed at e1de205

cmsbuild · 2021-10-02T13:53:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35495/25716

This PR adds an extra 208KB to repository

cmsbuild · 2021-10-02T13:53:31Z

Pull request #35495 was updated. @ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please check and sign again.

mmusich · 2021-10-02T13:54:21Z

@cmsbuild, please test

cmsbuild · 2021-10-02T17:10:45Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54096c/19339/summary.html
COMMIT: d8fac6d
CMSSW: CMSSW_12_1_X_2021-10-02-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35495/19339/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

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

tvami · 2021-10-02T17:19:37Z

+db

cmsbuild · 2021-10-02T17:19:54Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-10-02T17:28:33Z

+1

cmsbuild added this to the CMSSW_12_1_X milestone Sep 30, 2021

cmsbuild added code-checks-pending db-pending orp-pending pending-signatures tests-pending labels Sep 30, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 30, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 30, 2021

cmsbuild added tests-rejected and removed tests-started labels Sep 30, 2021

mmusich force-pushed the moderizeCondToolsSiPixel branch from 1e90ff6 to 676d8a0 Compare September 30, 2021 20:09

cmsbuild added code-checks-pending tests-pending and removed tests-rejected code-checks-approved labels Sep 30, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 30, 2021

mmusich added 2 commits October 2, 2021 15:37

reduce verbosity of SiPixelLorentzAngleDB_cfg.py

e1de205

cout -> edm::LogPrint

3ace84f

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 2, 2021

forgotten local includes

d8fac6d

mmusich force-pushed the moderizeCondToolsSiPixel branch from b1f6c56 to d8fac6d Compare October 2, 2021 13:46

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 2, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 2, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 2, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Oct 2, 2021

cmsbuild added orp-approved and removed orp-pending labels Oct 2, 2021

cmsbuild merged commit 5ad2e26 into cms-sw:master Oct 2, 2021

mmusich deleted the moderizeCondToolsSiPixel branch October 2, 2021 17:29

mmusich mentioned this pull request Nov 15, 2021

restructure CondTools/SiPixel #36132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize `CondTools/SiPixel` #35495

Modernize `CondTools/SiPixel` #35495

mmusich commented Sep 30, 2021 •

edited

cmsbuild commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

mmusich commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

tvami commented Sep 30, 2021

tvami commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

mmusich commented Sep 30, 2021

mmusich commented Sep 30, 2021

smuzaffar commented Sep 30, 2021 •

edited

cmsbuild commented Oct 2, 2021

mmusich commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

mmusich commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

tvami commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

perrotta commented Oct 2, 2021

Navigation Menu

Modernize CondTools/SiPixel #35495

Modernize CondTools/SiPixel #35495

Conversation

mmusich commented Sep 30, 2021 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

mmusich commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

Build

tvami commented Sep 30, 2021

tvami commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

mmusich commented Sep 30, 2021

mmusich commented Sep 30, 2021

smuzaffar commented Sep 30, 2021 • edited

cmsbuild commented Oct 2, 2021

mmusich commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

mmusich commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

Comparison Summary

tvami commented Oct 2, 2021

cmsbuild commented Oct 2, 2021

perrotta commented Oct 2, 2021

Modernize `CondTools/SiPixel` #35495

Modernize `CondTools/SiPixel` #35495

mmusich commented Sep 30, 2021 •

edited

smuzaffar commented Sep 30, 2021 •

edited