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

Fix ownership problem in TMVAEvaluator (76X) #13003

Conversation

ferencek
Copy link
Contributor

Backport of #12675

The TMVAEvaluator was meant to own the GBRForest if the TMVAEvaluator
made the instance itself else it was not supposed to own it if it
was given a GBRForest externally. However, for both cases the code
used an std::unique_ptr and attempted to call 'release' in the case
of non-ownership. The problem was multiple calls to initializeGBRForest
still caused the code to delete a GBRForest which was not supposed to be
owned. The code was changed to use an std::shared_ptr where we use a custom
deallocator in the case of non-ownership (the deallocator does nothing).
This problem was found in the IB RelVals.
@cmsbuild
Copy link
Contributor

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

It involves the following packages:

CommonTools/Utils

@cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel 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 Jan 20, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 20, 2016

+1

for #13003 3600253

  • code is backported correctly (the same code diffs)
  • jenkins tests pass and comparisons with baseline show no differences as expected.

Once this goes to 76X IB, I expect that the 4.77 relval will no longer crash.

davidlange6 added a commit that referenced this pull request Jan 22, 2016
…lem-from-CMSSW_7_6_3

Fix ownership problem in TMVAEvaluator (76X)
@davidlange6 davidlange6 merged commit 9045d94 into cms-sw:CMSSW_7_6_X Jan 22, 2016
cmsbuild added a commit that referenced this pull request Jan 22, 2016
@ferencek ferencek deleted the fixTMVAEvaluatorMemoryProblem-from-CMSSW_7_6_3 branch June 29, 2016 11:41
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