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

bsunanda:Run2-alca11 Tune+test several AlCaReco jobs and prepare DQM plots for IsoTrack Trigger #8943

Merged
merged 5 commits into from May 11, 2015

Conversation

bsunanda
Copy link
Contributor

@bsunanda bsunanda commented May 3, 2015

Tune and test several AlCaReco jobs and prepare DQM plots for IsoTrack Trigger

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2015

A new Pull Request was created by @bsunanda for CMSSW_7_5_X.

bsunanda:Run2-alca11 Tune+test several AlCaReco jobs and prepare DQM plots for IsoTrack Trigger

It involves the following packages:

Calibration/HcalAlCaRecoProducers
Calibration/HcalCalibAlgos
Calibration/HcalIsolatedTrackReco
Calibration/IsolatedParticles
DQMOffline/CalibCalo

@diguida, @danduggan, @cerminar, @cmsbuild, @nclopezo, @deguio, @mmusich can you please review it and eventually sign? Thanks.
@rociovilar this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2015

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8943/4501/summary.html

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @smuzaffar

@Dr15Jones
Copy link
Contributor

@diguida I looked and none of the modules are connected to a static or a thread-unsafe EventSetup product so these modules can be declared 'thread-safe'. As for making them edm::stream modules, that is more complicated.

AlCaHBHEMuonFilter, AlCaHBHEMuonProducer and AlCaIsoTracksProducer all have member variables which keep track of how # runs, # events and # events passed. Making them stream modules would makes those numbers inaccurate (since each stream would accumulate its own values). However, those values are only used to print an Info message at the end of the job and therefore shouldn't be used a s reason to make this module run 1/8th as fast as it could (we typically will use 8 threads). If it is an absolute requirement to keep that output then one could use a 'GlobalCache' ( https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkStreamModuleInterface#edm_GlobalCacheT).

HcalHBHEMuonAnalyzer and HcalIsoTrkAnalyzer both use the TFileService and therefore should be declared edm::one::EDAnalyzer<edm::one::SharedResources> and call usesResource("TFileService") in its constructor.

RecAnalyzerMinbias opens its own TFile and therefore should be a edm::one::EDAnalyzer<>.

The DQM modules are all fine as they are since they inherit from base classes which handle the threading.

All the modules should have the virtual functions which are being written declared with override in order to catch the cases where the member function signatures have changed.

@bsunanda
Copy link
Contributor Author

Thanks Chris for your analysis. Is there any presentation which I can refer to and make the changes required in these packages. I shall try to make that for the final 7_5 version (not backporting to 7_4). Best regards

Sunanda


From: Chris Jones [notifications@github.com]
Sent: 11 May 2015 20:48
To: cms-sw/cmssw
Cc: Sunanda Banerjee
Subject: Re: [cmssw] bsunanda:Run2-alca11 Tune+test several AlCaReco jobs and prepare DQM plots for IsoTrack Trigger (#8943)

@diguidahttps://github.com/diguida I looked and none of the modules are connected to a static or a thread-unsafe EventSetup product so these modules can be declared 'thread-safe'. As for making them edm::stream modules, that is more complicated.

AlCaHBHEMuonFilter, AlCaHBHEMuonProducer and AlCaIsoTracksProducer all have member variables which keep track of how # runs, # events and # events passed. Making them stream modules would makes those numbers inaccurate (since each stream would accumulate its own values). However, those values are only used to print an Info message at the end of the job and therefore shouldn't be used a s reason to make this module run 1/8th as fast as it could (we typically will use 8 threads). If it is an absolute requirement to keep that output then one could use a 'GlobalCache' ( https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkStreamModuleInterface#edm_GlobalCacheT).

HcalHBHEMuonAnalyzer and HcalIsoTrkAnalyzer both use the TFileService and therefore should be declared edm::one::EDAnalyzeredm::one::SharedResources and call usesResource("TFileService") in its constructor.

RecAnalyzerMinbias opens its own TFile and therefore should be a edm::one::EDAnalyzer<>.

The DQM modules are all fine as they are since they inherit from base classes which handle the threading.

All the modules should have the virtual functions which are being written declared with override in order to catch the cases where the member function signatures have changed.


Reply to this email directly or view it on GitHubhttps://github.com//pull/8943#issuecomment-101014702.

@davidlange6
Copy link
Contributor

+1

@diguida
Copy link
Contributor

diguida commented May 12, 2015

@Dr15Jones thanks a lot for your help. I will use your hints for the next PR :-)
@bsunanda at this point I propose the following:

  1. make a first PR fixing the points I raised in my comments;
  2. make a second PR fixing the threading behaviour;
  3. back port this PR and the one in point 1 into 74X, only after you validate them in the next pre-release.

@davidlange6 is this ok also for you?

@diguida
Copy link
Contributor

diguida commented May 12, 2015

@bsunanda look here for MT FWK.

HTH

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