Skip to content

Conversation

@Dr15Jones
Copy link
Contributor

PR description:

  • Added time based memory sampling
  • Also added comments in fillDescriptions

PR validation:

Code compiles. Made a simple configuration to test the service change and it worked.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 20, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • FWCore/Services (core)

@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@fwyzard, @makortel, @missirol, @wddgit this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

<< "'sampleEventNSeconds' and 'moduleSummaryRequested' cannot be used together";
}
iReg.watchPostBeginJob(this, &SimpleMemoryCheck::startSamplingThread);
iReg.watchPreEndJob(this, &SimpleMemoryCheck::stopSamplingThread);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to listen the early termination signals too, or do we call PreEndJob after those?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess with #45434 we try to call EndJob if it is possible in any way (alternative being the job terminating via std::terminate() or similar)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the EndJob transition should be run after early termination.

Copy link
Contributor

@wddgit wddgit Sep 20, 2024

Choose a reason for hiding this comment

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

I agree also. That is what it should be doing.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-385143/41676/summary.html
COMMIT: ec059a1
CMSSW: CMSSW_14_2_X_2024-09-20-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46081/41676/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

  • 2023.1010012023.101001_RunDisplacedJet2023D_10k/step1_dasquery.log
  • 2024.0000012024.000001_RunJetMET02024D_10k/step1_dasquery.log
  • 2023.101001DAS Error
Expand to see more relval errors ...
  • 2024.000001

Comparison Summary

Summary:

@makortel
Copy link
Contributor

Comparison differences are related to #39803


if (sampleEveryNSeconds_ > 0) {
if (oncePerEventMode_) {
throw cms::Exception("BadConfigOptions")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use

Suggested change
throw cms::Exception("BadConfigOptions")
throw cms::Exception("Configuration")

or

Suggested change
throw cms::Exception("BadConfigOptions")
throw edm::Exception(edm::errors::Configuration)

?

Also added comments in fillDescriptions
@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #46081 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-385143/41712/summary.html
COMMIT: 14a946f
CMSSW: CMSSW_14_2_X_2024-09-23-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46081/41712/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@makortel
Copy link
Contributor

+core

@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. @rappoccio, @antoniovilela, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 0898d62 into cms-sw:master Sep 24, 2024
@Dr15Jones Dr15Jones deleted the sampleSimpleMemoryCheck branch September 27, 2024 19:19
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.

5 participants