improve `SiStripHitEffFromCalibTree` and `SiStripHitResolFromCalibTree` #45083

mmusich · 2024-05-28T21:04:40Z

PR description:

Somewhat inspired by cms-sw/cms-bot#2228, trying to debug crashes reported at cms-sw/cms-bot#2228 (comment), improve the modules SiStripHitEffFromCalibTree and SiStripHitResolFromCalibTree:

close input calibTree files after reading them;
delete all new-ly allocated memory;
use named constants for k_END_OF_LAYERS and k_END_OF_LAYS_AND_RINGS in lieu of magic numbers

PR validation:

scram b runtests_testSiStripHitEfficiency and scram b runtests_testSiStripHitResolution run fine after the update.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Not a backport, might be backported to 14.0.X

cmsbuild · 2024-05-28T21:05:05Z

cms-bot internal usage

cmsbuild · 2024-05-28T21:10:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45083/40403

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CalibTracker/SiStripHitEfficiency/test/testHitEffWorker.py modified in PR(s): [Dummy] Try creating big-enough cache to cause splitting #44368

cmsbuild · 2024-05-28T21:10:53Z

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

It involves the following packages:

CalibTracker/SiStripHitEfficiency (alca)
CalibTracker/SiStripHitResolution (alca)

@cmsbuild, @saumyaphor4252, @perrotta, @consuegs can you please review it and eventually sign? Thanks.
@rsreds, @echabert, @gbenelli, @tocheng, @yuanchao, @mmusich, @robervalwalsh this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

mmusich · 2024-05-28T21:21:15Z

@cmsbuild, please test

cmsbuild · 2024-05-29T00:19:32Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e7715/39593/summary.html
COMMIT: 75a6c3d
CMSSW: CMSSW_14_1_X_2024-05-28-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45083/39593/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3338862
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3338833
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 202 log files, 165 edm output root files, 48 DQM output files
TriggerResults: no differences found

perrotta · 2024-05-29T06:22:28Z

CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEffFromCalibTree.cc

+    bool debug{false};
+
+    if (!tFile.IsZombie() && tFile.IsWritable() && debug) {
+      // Delete all objects in the file recursively


Why the objects in file are deleted and the files are written and closed only when the debug option is true?

it was an (unsuccessful) attempt to debug #45084. AFAIU normally the destructor of TFileService should do that automatically, but somewhere there is a double deletion.
This piece could be removed.

cmsbuild · 2024-05-29T15:19:29Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45083/40419

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CalibTracker/SiStripHitEfficiency/test/testHitEffWorker.py modified in PR(s): [Dummy] Try creating big-enough cache to cause splitting #44368

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45083/40419/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45083/40419/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2024-05-29T20:23:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45083/40425

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CalibTracker/SiStripHitEfficiency/test/testHitEffWorker.py modified in PR(s): [Dummy] Try creating big-enough cache to cause splitting #44368

cmsbuild · 2024-05-29T20:24:05Z

Pull request #45083 was updated. @saumyaphor4252, @cmsbuild, @consuegs, @perrotta can you please check and sign again.

perrotta · 2024-06-01T13:50:08Z

please test for CMSSW_14_1_ASAN_X

cmsbuild · 2024-06-01T18:15:45Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e7715/39659/summary.html
COMMIT: 45d22a4
CMSSW: CMSSW_14_1_ASAN_X_2024-05-31-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45083/39659/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 2 errors in the following unit tests:

---> test testSiStripHitResolution had ERRORS
---> test testSiStripHitEfficiency had ERRORS

mmusich · 2024-06-14T06:27:39Z

@cmsbuild please test with cms-sw/cms-bot#2228

cmsbuild · 2024-06-14T09:39:08Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e7715/39884/summary.html
COMMIT: 45d22a4
CMSSW: CMSSW_14_1_X_2024-06-13-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45083/39884/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 2 errors in the following unit tests:

---> test testSiStripHitResolution had ERRORS
---> test testSiStripHitEfficiency had ERRORS

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3345018
DQMHistoTests: Total failures: 54
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3344944
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 202 log files, 165 edm output root files, 48 DQM output files
TriggerResults: no differences found

cmsbuild · 2024-08-27T08:09:32Z

Milestone for this pull request has been moved to CMSSW_14_2_X. Please open a backport if it should also go in to CMSSW_14_1_X.

antoniovilela · 2024-09-03T09:36:03Z

ping (to make bot change milestone)

improve SiStripHitEffFromCalibTree and SiStripHitResolFromCalibTree

75a6c3d

cmsbuild added this to the CMSSW_14_1_X milestone May 28, 2024

cmsbuild added alca-pending pending-signatures tests-pending orp-pending code-checks-pending trk labels May 28, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels May 28, 2024

cmsbuild added tests-started and removed tests-pending labels May 28, 2024

cmsbuild added tests-approved and removed tests-started labels May 29, 2024

perrotta reviewed May 29, 2024

View reviewed changes

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels May 29, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels May 29, 2024

mmusich force-pushed the mm_dev_improveCalibraTrackerSiStrip branch from bb8ce72 to 08c6681 Compare May 29, 2024 15:21

cmsbuild added code-checks-pending and removed code-checks-rejected labels May 29, 2024

cmsbuild added tests-pending code-checks-pending and removed tests-started code-checks-approved labels May 29, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels May 29, 2024

cmsbuild added tests-started and removed tests-pending labels Jun 1, 2024

cmsbuild added tests-rejected and removed tests-started labels Jun 1, 2024

cmsbuild added requires-external tests-started and removed tests-rejected labels Jun 14, 2024

cmsbuild added tests-rejected and removed tests-started labels Jun 14, 2024

cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Aug 27, 2024

cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Sep 3, 2024

cmsbuild mentioned this pull request Sep 24, 2024

fix for cms-sw/cmssw#45084 #46111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve `SiStripHitEffFromCalibTree` and `SiStripHitResolFromCalibTree` #45083

improve `SiStripHitEffFromCalibTree` and `SiStripHitResolFromCalibTree` #45083

mmusich commented May 28, 2024

cmsbuild commented May 28, 2024 •

edited

Loading

cmsbuild commented May 28, 2024

cmsbuild commented May 28, 2024

mmusich commented May 28, 2024

cmsbuild commented May 29, 2024

perrotta May 29, 2024

mmusich May 29, 2024 •

edited

Loading

cmsbuild commented May 29, 2024

cmsbuild commented May 29, 2024

cmsbuild commented May 29, 2024

perrotta commented Jun 1, 2024

cmsbuild commented Jun 1, 2024

mmusich commented Jun 14, 2024

cmsbuild commented Jun 14, 2024

cmsbuild commented Aug 27, 2024

antoniovilela commented Sep 3, 2024

improve SiStripHitEffFromCalibTree and SiStripHitResolFromCalibTree #45083

Are you sure you want to change the base?

improve SiStripHitEffFromCalibTree and SiStripHitResolFromCalibTree #45083

Conversation

mmusich commented May 28, 2024

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented May 28, 2024 • edited Loading

cmsbuild commented May 28, 2024

cmsbuild commented May 28, 2024

mmusich commented May 28, 2024

cmsbuild commented May 29, 2024

Comparison Summary

perrotta May 29, 2024

Choose a reason for hiding this comment

mmusich May 29, 2024 • edited Loading

Choose a reason for hiding this comment

cmsbuild commented May 29, 2024

cmsbuild commented May 29, 2024

cmsbuild commented May 29, 2024

perrotta commented Jun 1, 2024

cmsbuild commented Jun 1, 2024

Unit Tests

mmusich commented Jun 14, 2024

cmsbuild commented Jun 14, 2024

Unit Tests

Comparison Summary

cmsbuild commented Aug 27, 2024

antoniovilela commented Sep 3, 2024

improve `SiStripHitEffFromCalibTree` and `SiStripHitResolFromCalibTree` #45083

improve `SiStripHitEffFromCalibTree` and `SiStripHitResolFromCalibTree` #45083

cmsbuild commented May 28, 2024 •

edited

Loading

mmusich May 29, 2024 •

edited

Loading