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

Making the boosted double SV tagger thread-safe (76X) #12679

Conversation

ferencek
Copy link
Contributor

@ferencek ferencek commented Dec 6, 2015

fastjet::contrib::Njettiness is not thread-safe and therefore should not be used as a data member of an ESProducer. This PR addresses the problem by defining a local instance of fastjet::contrib::Njettiness instead of using a global instance defined as a data member of the CandidateBoostedDoubleSecondaryVertexComputer class.

This PR is urgent since it fixes CMSSW crashes occurring in the multi-threaded jobs.

Njettiness is not thread-safe and therefore should not be used as a data member
of an ESProducer
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2015

A new Pull Request was created by @ferencek (Dinko Ferenček) for CMSSW_7_6_X.

It involves the following packages:

RecoBTag/SecondaryVertex

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@ferencek, @acaudron, @pvmulder, @imarches this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@slava77
Copy link
Contributor

slava77 commented Dec 6, 2015

@cmsbuild please test
@ferencek how much slower is this solution?

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2015

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10165/console

@ferencek ferencek changed the title Making the boosted double SV tagger thread-safe Making the boosted double SV tagger thread-safe (76X) Dec 6, 2015
@ferencek
Copy link
Contributor Author

ferencek commented Dec 6, 2015

@slava77, haven't checked the speed but I'm afraid there is no alternative at the moment.

@Dr15Jones @rappoccio

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2015

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 8, 2015

@ferencek: Today it was reported that this bug fix is essential to enable multi-threaded 76X running, which is always crashing right now. Please add this information to the description of this PR. It is important for reviewers to know this PR is very urgent.

@ferencek
Copy link
Contributor Author

ferencek commented Dec 8, 2015

@cvuosalo, both @davidlange6 and @smuzaffar were included in the original email from @Dr15Jones so I think all relevant people are already in the loop and know that this PR and #12680 are critical to include.

@slava77
Copy link
Contributor

slava77 commented Dec 8, 2015

@ferencek
it should be clear from the PR that it fixes crashes in multi-threaded jobs (including CMSSW_7_6_2, if I understand correctly).
We need good documentation of the impact of the update for release self-documentation.
This can't be replaced with private email threads.

@ferencek
Copy link
Contributor Author

ferencek commented Dec 8, 2015

@slava77, point taken. The PRs were simply a reaction to the email Chris sent. I personally did not run any jobs and probably did not fully appreciate the scale of the problem. I have updated the PR description.

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 8, 2015

+1

For #12679 b7970d9

Urgent bug fix for to prevent multi-threaded jobs from crashing. There should be no change in monitored quantities. #12680 is the 80X version of this PR.

The code change is satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2015-12-04-1100 show no significant differences, as expected.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2015

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

davidlange6 added a commit that referenced this pull request Dec 10, 2015
…readSafe_from-CMSSW_7_6_1

Making the boosted double SV tagger thread-safe (76X)
@davidlange6 davidlange6 merged commit 9278472 into cms-sw:CMSSW_7_6_X Dec 10, 2015
@ferencek ferencek deleted the BoostedDoubleSVTaggerV2-ThreadSafe_from-CMSSW_7_6_1 branch December 10, 2015 22:09
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

5 participants