RooFit::MultiProcess & TestStatistics part 7: MultiProcess based TestStatistics classes by egpbos · Pull Request #9349 · root-project/root

egpbos · 2021-11-29T20:26:01Z

In this PR, we introduce two new classes: LikelihoodJob and LikelihoodGradientJob. These are RooFit::MultiProcess based test-statistic calculators that parallelize likelihood and likelihood gradient calculation respectively over multiple processes. These classes can be used internally by RooMinimizer to speed up fitting with migrad. The way to use them is to use the new RooMinimizer constructor that takes a (pointer to a) RooAbsL likelihood class that was introduced in previous PR #8700.

This PR is the seventh and final part of a split and clean-up of #8294.

Changes or fixes:

Adds LikelihoodJob and LikelihoodGradientJob under the RooFit::TestStatistics namespace.
Adds the LikelihoodGradientJob test case, which also covers the rest of the TestStatistics framework, as promised in RooFit::MultiProcess & TestStatistics part 6: add RooFit::TestStatistics #8700 (comment).
Adds two KahanSum constructors that allow for initialization of the full internal state. This is necessary for serializing and rematerializing KahanSums so they can be sent over ZeroMQ sockets.
RooMinimizer templated constructors and create factory functions were removed. These are replaced with enum class flags that allow the user to choose the type of RooAbsMinimizerFcn (this was already in place) and the Likelihood(Gradient)Wrapper implementations to use, i.e. the classes introduced in this PR. Similar changes were made in MinuitFcnGrad, which is now also template-free.
RooMinimizer's (now) two constructors use two helper functions now to avoid code duplication.
There were some mistakes in the build setup of RooFitZMQ and RooFit::MultiProcess that only came to light now when building these classes that depend on RooFit::MultiProcess. These are fixed.
A few functions had to be added to LikelihoodGradientWrapper and MinuitFcnGrad for passing along previous gradient, second derivative and step size information from Minuit2 to RooFit and back, i.e. using the functionality introduced in RooFit::MultiProcess & TestStatistics part 5b: test RooGradMinimizerFcn #8694.

Checklist:

tested changes locally
update the docs (if necessary)
unify copyright/license headers with previous commits
includes: correct order (matching header, RooFit, ROOT, std) and ROOT includes in quotation marks
refactor member names: underscore suffix
remove commented out code and TODOs and other junk
clang-tidy up
rebase into two commits (one for KahanSum and one for the rest)
make Readme.md for developers with some examples to run

egpbos · 2021-11-29T20:30:35Z

@phsft-bot build with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON

egpbos · 2021-11-30T07:22:01Z

@phsft-bot build with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON

egpbos · 2021-11-30T15:14:46Z

@phsft-bot build with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON

phsft-bot · 2021-11-30T15:14:51Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14 with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON
How to customize builds

phsft-bot · 2021-11-30T15:50:44Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roofit.roofitcore.test.gtest_roofit_roofitcore_test_testLikelihoodGradientJob

phsft-bot · 2021-11-30T15:51:05Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Warnings:

[2021-11-30T15:18:03.662Z] /data/sftnight/workspace/root-pullrequests-build/root/graf2d/mathtext/src/table/adobeglyphlist.h:128:44: warning: unpaired UTF-8 bidirectional character detected [-Wbidirectional=]
[2021-11-30T15:18:03.662Z] /data/sftnight/workspace/root-pullrequests-build/root/graf2d/mathtext/src/table/adobeglyphlist.h:128:55: warning: unpaired UTF-8 bidirectional character detected [-Wbidirectional=]
[2021-11-30T15:23:37.929Z] /data/sftnight/workspace/root-pullrequests-build/root/graf2d/postscript/src/AdobeGlyphList.h:110:44: warning: unpaired UTF-8 bidirectional character detected [-Wbidirectional=]
[2021-11-30T15:23:37.929Z] /data/sftnight/workspace/root-pullrequests-build/root/graf2d/postscript/src/AdobeGlyphList.h:110:55: warning: unpaired UTF-8 bidirectional character detected [-Wbidirectional=]

Failing tests:

projectroot.roofit.roofitcore.test.gtest_roofit_roofitcore_test_testLikelihoodGradientJob

phsft-bot · 2021-11-30T17:11:00Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-11-30T17:09:20.330Z] C:\build\workspace\root-pullrequests-build\root\roofit\roofitcore\res\TestStatistics/LikelihoodJob.h(17,10): fatal error C1083: Cannot open include file: 'RooFit/MultiProcess/Job.h': No such file or directory [C:\build\workspace\root-pullrequests-build\build\roofit\roofitcore\RooFitCore.vcxproj]

phsft-bot · 2021-11-30T17:42:59Z

Build failed on mac1015/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

egpbos · 2021-11-30T19:01:57Z

@phsft-bot build on windows10/cxx14 with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON

phsft-bot · 2021-11-30T19:02:02Z

Starting build on windows10/cxx14 with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON
How to customize builds

phsft-bot · 2021-12-02T15:55:49Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

egpbos · 2021-12-02T15:58:11Z

@phsft-bot build with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON

phsft-bot · 2021-12-02T15:58:17Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14 with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON
How to customize builds

phsft-bot · 2021-12-02T16:09:36Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-12-02T16:09:35.520Z] CMake Error at cmake/modules/RootBuildOptions.cmake:353 (message):
[2021-12-02T16:09:35.520Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1128 (message):

egpbos · 2021-12-02T18:45:26Z

Building successfully with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON (Windows failure is as expected, since we set it to error when activating roofit_multiprocess):

phsft-bot · 2021-12-02T18:57:58Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-12-03T12:07:08Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

guitargeek

Thanks @egpbos for this very well organized PR, respecting many good practices! It was smooth to review with this commit history and there is nothing that blocks us from merging.

One thing that we need to still follow up on is the leaking of zeromq symbols. I will check again if they still leak in my environment, and if yes, open a GitHub issue so we have a communication thread to talk about this further.

…o_workers Also use perfect forwarding to avoid copying movable messages.

Replaces custom variables, which were not working anyway, because they are not defined outside of their scope, in parent (or sibling) directories.

The default setting for roofit_multiprocess on Windows is now OFF. If -Droofit_multiprocess=ON is passed manually, CMake configuration will emit a fatal error.

Three things that also impact existing files (not just the ones added in this commit): - RooMinimizer templated constructors and create factory functions removed. Replaced with enum class flags that allow the user to choose the type of RooAbsMinimizerFcn and the Likelihood(Gradient)Wrapper implementations to use. Same for TestStatistics::MinuitFcnGrad. - add *WithPrevResults implementations to pass along previous gradient, second derivative and step size information between Minuit2 and RooFit. - Add compiler definition to disable MultiProcess based classes (also in MinuitFcnGrad) when not building with MultiProcess (e.g. in Windows).

phsft-bot · 2021-12-03T13:58:29Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

guitargeek · 2021-12-03T13:59:49Z

@phsft-bot build with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON

phsft-bot · 2021-12-03T13:59:54Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14 with flags -Droofit_multiprocess=ON -Dbuiltin_zeromq=ON -Dbuiltin_cppzmq=ON
How to customize builds

phsft-bot · 2021-12-03T14:06:25Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-12-03T14:06:24.028Z] CMake Error at cmake/modules/RootBuildOptions.cmake:354 (message):
[2021-12-03T14:06:24.304Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1128 (message):

lmoneta

LGTM !
Thank you Patrick for this contribution!

egpbos requested review from bellenot, guitargeek, hageboeck and lmoneta as code owners November 29, 2021 20:26

This comment has been minimized.

Sign in to view

root-project deleted a comment from phsft-bot Nov 30, 2021

This comment has been minimized.

Sign in to view

root-project deleted a comment from phsft-bot Nov 30, 2021

This comment has been minimized.

Sign in to view

egpbos force-pushed the RooFit_MultiProcess_PR_7_MultiProcess_TestStatistics branch from ce08c20 to 807564d Compare December 2, 2021 18:57

lmoneta assigned guitargeek Dec 3, 2021

egpbos force-pushed the RooFit_MultiProcess_PR_7_MultiProcess_TestStatistics branch from 807564d to fb952c0 Compare December 3, 2021 12:06

guitargeek approved these changes Dec 3, 2021

View reviewed changes

egpbos mentioned this pull request Dec 3, 2021

RooFit::MultiProcess & TestStatistics part 2 redo: RooFitZMQ & MultiProcess #9078

Merged

16 tasks

egpbos added 8 commits December 3, 2021 14:56

Math::KahanSum: add constructors for full state initialization

ac1d3ea

zmq builtin: don't install zmq.hpp, use from build dir

43e6390

RooFit::MultiProcess: use multipart messages in publish_from_master_t…

c00b28e

…o_workers Also use perfect forwarding to avoid copying movable messages.

RooFit::MultiProcess: turn off by default

f07af77

RooFit::TestStatistics: fix copyright/license headers

98ade42

RooFitZMQ/MultiProcess: properly export include paths via CMake targets

0dd510c

Replaces custom variables, which were not working anyway, because they are not defined outside of their scope, in parent (or sibling) directories.

RooFit::MultiProcess: don't build on Windows

221cdad

The default setting for roofit_multiprocess on Windows is now OFF. If -Droofit_multiprocess=ON is passed manually, CMake configuration will emit a fatal error.

egpbos force-pushed the RooFit_MultiProcess_PR_7_MultiProcess_TestStatistics branch from fb952c0 to 8341962 Compare December 3, 2021 13:58

lmoneta approved these changes Dec 3, 2021

View reviewed changes

guitargeek mentioned this pull request Dec 3, 2021

RooFit MultiProcess and TestStatistics #8294

Closed

guitargeek merged commit 62d3323 into root-project:master Dec 3, 2021

guitargeek mentioned this pull request Dec 6, 2021

Fix warning of unused variable in roofit #9368

Closed

2 tasks

guitargeek added the in:RooFit label Aug 3, 2022

Conversation

egpbos commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes or fixes:

Checklist:

Uh oh!

This comment has been minimized.

egpbos commented Nov 29, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

egpbos commented Nov 30, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

egpbos commented Nov 30, 2021

Uh oh!

phsft-bot commented Nov 30, 2021

Uh oh!

phsft-bot commented Nov 30, 2021

Failing tests:

Uh oh!

phsft-bot commented Nov 30, 2021

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Nov 30, 2021

Errors:

Uh oh!

phsft-bot commented Nov 30, 2021

Failing tests:

Uh oh!

This comment has been minimized.

egpbos commented Nov 30, 2021

Uh oh!

phsft-bot commented Nov 30, 2021

Uh oh!

phsft-bot commented Dec 2, 2021

Uh oh!

egpbos commented Dec 2, 2021

Uh oh!

phsft-bot commented Dec 2, 2021

Uh oh!

phsft-bot commented Dec 2, 2021

Errors:

Uh oh!

egpbos commented Dec 2, 2021

Uh oh!

phsft-bot commented Dec 2, 2021

Uh oh!

phsft-bot commented Dec 3, 2021

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Dec 3, 2021

Uh oh!

guitargeek commented Dec 3, 2021

Uh oh!

phsft-bot commented Dec 3, 2021

Uh oh!

phsft-bot commented Dec 3, 2021

Errors:

Uh oh!

lmoneta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

egpbos commented Nov 29, 2021 •

edited

Loading