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

Better handling of TriggerResults consumes in PAT Trigger modules #19259

Conversation

Dr15Jones
Copy link
Contributor

If the module is told to automatically determine what process to
get the TriggerResults from, the code will now skip the current
process TriggerResults. This avoids problems where the module is
placed on a Path instead of an EndPath or being run unscheduled.

If the module is told to automatically determine what process to
get the TriggerResults from, the code will now skip the current
process TriggerResults. This avoids problems where the module is
placed on a Path instead of an EndPath or being run unscheduled.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @seemasharmafnal, @JyothsnaKomaragiri this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 15, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20618/console Started: 2017/06/15 22:08

@cmsbuild
Copy link
Contributor

-1

Tested at: 9bfcc20

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
3e65ff2
a08f0f6
35ad0e0
5205a46
e01c1ff
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20618/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20618/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20618/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
136.731 step1

DAS Error

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
3e65ff2
a08f0f6
35ad0e0
5205a46
e01c1ff
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20618/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20618/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 15, 2017

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

@cmsbuild
Copy link
Contributor

-1

Tested at: 9bfcc20

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
3e65ff2
a08f0f6
35ad0e0
5205a46
e01c1ff
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20623/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20623/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20623/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
136.731 step1

DAS Error

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
3e65ff2
a08f0f6
35ad0e0
5205a46
e01c1ff
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20623/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19259/20623/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@Dr15Jones
Copy link
Contributor Author

This change was shown to solve the problem seen in this hypernews threads:
https://hypernews.cern.ch/HyperNews/CMS/get/physTools/3547/1/1/1/1/1/1/1/1/1.html

@slava77
Copy link
Contributor

slava77 commented Jun 16, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 16, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20660/console Started: 2017/06/17 00:24

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • You potentially added 2 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1803136
  • DQMHistoTests: Total failures: 14571
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1788399
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@slava77
Copy link
Contributor

slava77 commented Jun 17, 2017

@Dr15Jones
was it previously possible to get TriggerResults from the current process in the producers modified in this PR?

@Dr15Jones
Copy link
Contributor Author

@slava77 Before the implementation of sub-Event level multi-threading, if the module was on an EndPath it could get the current process' TriggerResults while if it were on a Path that lookup would fail. The old code was relying on this 'fail to get current' in order to work.

After the implementation of sub-Event level multi-threading, the framework looks at the consumes information and can tell that if a module requests a TriggerResults for the current process but the module is on a Path that it is impossible for the TriggerResults to even be gotten and therefore there is a scheduling problem.

It is still possible to get this new code to get the current process, one just needs to explicitly say it must come from the process by using the process name and the module must either be run unscheduled or be explicitly placed on an EndPath.

@slava77
Copy link
Contributor

slava77 commented Jun 17, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Jun 17, 2017

+1

for #19259 9bfcc20

  • update relevant for non-standard workflows
  • jenkins tests pass and comparisons show no differences

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit f9fc63b into cms-sw:master Jun 19, 2017
@Dr15Jones Dr15Jones deleted the betterTriggerResultsHandlingInPatTriggerModules branch June 22, 2017 16:53
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