PPS transport update #36589

civanch · 2021-12-24T15:24:21Z

PR description:

This PR should fix #36544 - thread safety is PPS proton transport is provided,
all class members in the sub-library are now initialized to zero, LogVerbatim is used instead of LogInfo. Because PPS transport is disabled for the time being, no change in results are expected.

PR validation:

private

cmsbuild · 2021-12-24T15:30:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36589/27555

This PR adds an extra 20KB to repository

cmsbuild · 2021-12-24T15:30:54Z

A new Pull Request was created by @civanch (Vladimir Ivantchenko) for master.

It involves the following packages:

SimTransport/PPSProtonTransport (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

civanch · 2021-12-24T15:45:03Z

please test

cmsbuild · 2021-12-24T19:44:39Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6314e0/21464/summary.html
COMMIT: d154254
CMSSW: CMSSW_12_3_X_2021-12-24-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36589/21464/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: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3461692
DQMHistoTests: Total failures: 16
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3461653
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 42 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

civanch · 2021-12-25T13:54:02Z

+1

this PR is a preparation to restore PPS simulation, itself should not affect any WF.

cmsbuild · 2021-12-25T13:54:27Z

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-12-26T13:18:09Z

@civanch, how did you check that the issue #36544 gets fixed with this PR?
If so, couldn't we also re-enable PPS simulation here?

If any further change is needed in this PR (e.g. re-enabling PPS sim), could you please profit and fix the following (unrelated) issues pointed out by the static analyzer in two files already touched by this PR?

SimTransport/PPSProtonTransport/src/HectorTransport.cc
- Please do not re-assign ZforPosition at L123
SimTransport/PPSProtonTransport/src/BaseProtonTransport.cc
- Please remove definition (L30) and modification (L38-39) of the variable theta, which is used nowhere in the code

cmsbuild · 2022-01-03T17:18:01Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36589/27618

ERROR: Build errors found during clang-tidy run.

--
SimTransport/PPSProtonTransport/src/OpticalFunctionsTransport.cc:192:57: error: 'fPPSRegionStart_45_' is a private member of 'BaseProtonTransport' [clang-diagnostic-error]
  b_y -= (abs(z_scoringPlane) - ((z_scoringPlane < 0) ? fPPSRegionStart_45_ : fPPSRegionStart_56_) * 1e3) * a_y;
                                                        ^
SimTransport/PPSProtonTransport/interface/BaseProtonTransport.h:82:10: note: declared private here
--
SimTransport/PPSProtonTransport/src/OpticalFunctionsTransport.cc:192:79: error: 'fPPSRegionStart_56_' is a private member of 'BaseProtonTransport' [clang-diagnostic-error]
  b_y -= (abs(z_scoringPlane) - ((z_scoringPlane < 0) ? fPPSRegionStart_45_ : fPPSRegionStart_56_) * 1e3) * a_y;
                                                                              ^
SimTransport/PPSProtonTransport/interface/BaseProtonTransport.h:83:10: note: declared private here
--
SimTransport/PPSProtonTransport/interface/BaseProtonTransport.h:36:3: error: C++ requires a type specifier for all declarations [clang-diagnostic-error]
  setBeamFileNames(const std::string& nam1, const std::string& nam2) {
  ^
SimTransport/PPSProtonTransport/interface/BaseProtonTransport.h:41:3: error: C++ requires a type specifier for all declarations [clang-diagnostic-error]
  setBeamParameters(double stx, double sty, double sx, double sy, double se) {
  ^
SimTransport/PPSProtonTransport/interface/BaseProtonTransport.h:49:3: error: C++ requires a type specifier for all declarations [clang-diagnostic-error]
  setCrossingAngles(double cx45, double cx56, double cy45, double cy56) {
  ^
Suppressed 1578 warnings (1577 in non-user code, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild · 2022-01-03T19:52:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36589/27622

This PR adds an extra 32KB to repository

cmsbuild · 2022-01-03T19:52:37Z

Pull request #36589 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

civanch · 2022-01-03T20:22:55Z

please test

cmsbuild · 2022-01-03T22:58:20Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6314e0/21521/summary.html
COMMIT: d0f9ffe
CMSSW: CMSSW_12_3_X_2022-01-03-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36589/21521/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: 1 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3461692
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3461670
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

civanch · 2022-01-04T09:17:20Z

+1

concerns to initialization are addressed without modifications of python files.

cmsbuild · 2022-01-04T09:17:42Z

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 · 2022-01-04T17:06:19Z

+1

The code would require further optimizations and rationalization (e.g non optimal usage of the class members in the base and derived classes, as pointed out in the comments in thread): I expect that @cms-sw/ctpps-dpg-l2 and/or @cms-sw/proton-pog-l2 will take care of in their forthcoming cleanings

civanch added 2 commits December 24, 2021 16:16

Make PPS transport thread safe

a4a5fbe

code format

d154254

cmsbuild added this to the CMSSW_12_3_X milestone Dec 24, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures simulation-pending tests-pending labels Dec 24, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 24, 2021

cmsbuild added tests-started and removed tests-pending labels Dec 24, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 24, 2021

slava77 mentioned this pull request Dec 24, 2021

Extending SeedingHitSet & SeedGeneratorFromProtoTracksEDProducer #36184

Merged

cmsbuild added fully-signed simulation-approved and removed simulation-pending pending-signatures labels Dec 25, 2021

civanch mentioned this pull request Dec 25, 2021

Thread-safety issue in TotemTransport #36544

Closed

fixed static analysis warnings

5184385

cmsbuild removed simulation-approved fully-signed labels Dec 26, 2021

cmsbuild added code-checks-pending tests-pending and removed tests-rejected code-checks-approved labels Jan 3, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 3, 2022

code format

d0f9ffe

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jan 3, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 3, 2022

cmsbuild added tests-started and removed tests-pending labels Jan 3, 2022

cmsbuild added tests-approved and removed tests-started labels Jan 3, 2022

cmsbuild removed simulation-pending pending-signatures labels Jan 4, 2022

cmsbuild added fully-signed simulation-approved labels Jan 4, 2022

cmsbuild added orp-approved and removed orp-pending labels Jan 4, 2022

cmsbuild merged commit 1bfe19e into cms-sw:master Jan 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PPS transport update #36589

PPS transport update #36589

civanch commented Dec 24, 2021 •

edited

cmsbuild commented Dec 24, 2021

cmsbuild commented Dec 24, 2021

civanch commented Dec 24, 2021

cmsbuild commented Dec 24, 2021

civanch commented Dec 25, 2021

cmsbuild commented Dec 25, 2021

perrotta commented Dec 26, 2021

cmsbuild commented Jan 3, 2022

cmsbuild commented Jan 3, 2022

cmsbuild commented Jan 3, 2022

civanch commented Jan 3, 2022

cmsbuild commented Jan 3, 2022

civanch commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

perrotta commented Jan 4, 2022

PPS transport update #36589

PPS transport update #36589

Conversation

civanch commented Dec 24, 2021 • edited

PR description:

PR validation:

cmsbuild commented Dec 24, 2021

cmsbuild commented Dec 24, 2021

civanch commented Dec 24, 2021

cmsbuild commented Dec 24, 2021

Comparison Summary

civanch commented Dec 25, 2021

cmsbuild commented Dec 25, 2021

perrotta commented Dec 26, 2021

cmsbuild commented Jan 3, 2022

cmsbuild commented Jan 3, 2022

cmsbuild commented Jan 3, 2022

civanch commented Jan 3, 2022

cmsbuild commented Jan 3, 2022

Comparison Summary

civanch commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

perrotta commented Jan 4, 2022

civanch commented Dec 24, 2021 •

edited