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

add option for sequential RootEmbeddedFileSequence to not recycle files #20403

Merged
merged 4 commits into from Sep 8, 2017

Conversation

dan131riley
Copy link

PR #9091 changed the behavior of RootInputFileSequence (now re-factored into RootEmbeddedFileSequence) such that, in sequential mode, when the secondary source reaches the end of the last file it loops back to the beginning of the first file instead of halting. This makes sense for the primary use in the mixing module; however, SiStripSpyEventMatcher depends on the old behavior. This PR adds an option (recycleFiles, defaults to true) that restores the old behavior when turned off, and modifies the SiStripSpyEventMatcher cfi to set recycleFiles to false. Fixes issue #20361

…ntial mode, and update spyEventMatching_cfg.py to use it
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

A new Pull Request was created by @dan131riley (Dan Riley) for master.

It involves the following packages:

DQM/SiStripMonitorHardware
IOPool/Input

@smuzaffar, @Dr15Jones, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @vanbesien can you please review it and eventually sign? Thanks.
@idebruyn, @threus, @fioriNTU, @wddgit, @hdelanno this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20403/527

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20403/527/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20403/527/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22751/console Started: 2017/09/06 16:36

@Dr15Jones
Copy link
Contributor

@dan131riley or do you think adding a different interface to call which does not loop would be easier for code to deal with? A configuration option implies that the calling code could handle either behavior. An interface explicitly says only one behavior is expected.

@dan131riley
Copy link
Author

@Dr15Jones good question. The interface that's actually used is VectorInputSource::loopOverEvents(), and there's two use cases:

  1. loop over n events, recycling files if necessary (pileup)
  2. loop once over all the events in the input files (SpyEventMatcher)

So changing the interface would mean adding interfaces to both RootEmbeddedFileSequence and VectorInputSource, maybe changing

    source_->loopOverEvents(*eventPrincipal_,fileNameHash,std::numeric_limits<size_t>::max(),boost::bind(&SpyEventMatcher::addNextEventToMap,this,_1));

to something like

    source_->loopOverEventsOnce(*eventPrincipal_,fileNameHash,std::numeric_limits<size_t>::max(),boost::bind(&SpyEventMatcher::addNextEventToMap,this,_1));

which, after sketching it out, does look better. So I'll update the PR to do that, and you won't be able to spot the changes since it'll also have all the clang-tidy diffs...

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2656498
  • DQMHistoTests: Total failures: 216
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2656093
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

…pset parameter; this touches FWCore/Sources/interface/VectorInputSource.h, so the changes are more intrusive than the pset hack
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22760/console Started: 2017/09/06 20:03

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

Pull request #20403 was updated. @smuzaffar, @Dr15Jones, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @vanbesien can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20403/539

@Dr15Jones
Copy link
Contributor

please test

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22761/console Started: 2017/09/06 21:31

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2017

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2656498
  • DQMHistoTests: Total failures: 208
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2656101
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 76961d8 into cms-sw:master Sep 8, 2017
@dan131riley dan131riley deleted the readOneSequential-norecycle branch September 12, 2017 13:59
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

4 participants