RooFit::MultiProcess & TestStatistics part 2 redo: RooFitZMQ & MultiProcess #9078
Conversation
|
Starting build on |
|
Build failed on mac1014/python3. Warnings:
|
|
Build failed on mac11.0/cxx17. Warnings:
Failing tests: |
There was a problem hiding this comment.
This has improved a lot over the previous version. I think with just a bit of work to ensure the build system parts work in both builtin and non-builtin mode, it can be merged. I will leave review of things other than the build system for others in the team that are more familiar with RooFit.
BTW, it would also be really nice to have the final git history with fixups and formatting changes already squashed into the appropriate commits if you can do that. Thanks a lot!
|
Thanks for the build system review @amadio, very useful comments. Indeed, I will rebase stuff into probably just two commits, one for RooFitZMQ and one for MultiProcess. Maybe one more for the builtins, but probably not, because they don't make sense without RooFitZMQ. |
|
Starting build on |
|
Build failed on mac1014/python3. Warnings:
|
|
Build failed on mac11.0/cxx17. Warnings:
Failing tests: |
|
Starting build on |
2 similar comments
|
Starting build on |
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
|
@lmoneta indeed, it looks like |
|
@phsft-bot build with flags -Droofit_multiprocess=ON |
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
|
Build failed on ROOT-ubuntu2004/soversion. Errors:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on mac1014/python3. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on mac11.0/cxx17. Errors:
|
|
Starting build on |
|
@phsft-bot build with flags -Droofit_multiprocess=ON |
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
guitargeek
dismissed stale reviews from amadio and Axel-Naumann
PR has been updated in the meantime and requested changes addressed
|
@egpbos Given my comment in your previous merge request, I am quite disappointed to learn that this has been merged as enabled by default when it depends on a yet unreleased version of ZeroMQ. This, for example, has caused failures in pretty much all the LCG builds today: https://cdash.cern.ch/index.php?project=LCGSoft&date=2021-11-29. Please disable |
|
@amadio You're right, we overlooked that. We should discuss in the RooFit dev meeting how to do this, also in the long run. I guess for the time being, setting it to If this is indeed what we do, it would be quite important (imho) that CI still builds with |
|
Hi @amadio! I thought we had addressed this problem by making the unreleased What exactly is the problem you have at configuration times in these LCG builds? I can't see the full log. I hope it's just one of your configurations checks that doesn't work in your environment and we can fix that. |
|
Ah, I wasn't aware that that was also meant to solve @amadio's problem, thx @guitargeek. |
|
The problem is that LCG cannot use builtins (and also sets |
|
Hm ok, actually it does also work with a custom built/installed from source version of the unreleased libzmq library, so that would be an option to avoid builtins in those situations. But I guess that's not something they will want to invest time in. |
|
Makes sense in general, that building without builtins should be possibe in LCG and distros! But why can't we use builtins at compile time only? I don't see why not, and that would also be a big restriction :) |
That's only half-true, of course we do have builtins that LGC builds with just fine (libAfterImage, llvm, etc). But then we must not expose it as "override-able". So either it's not configurable and we always build our "hacked" zmq (exporting neither headers nor symbols, so the world doesn't care), or we wait until it's released. |
|
100 % agreed. So I'll open a PR to always use our "hacked" zmq (and yes we expose neither headers nor symbols). When it's release we can change it back |
ZeroMQ is already part of LCG, so unless we are absolutely sure that no symbol will be leaked, we should use the external one. Like with LLVM, which has bitten with this many times because of mesa using it too and linking to it, there's always the risk of having multiple incompatible versions of symbols loaded into the same running process. Builtins are evil, and patched builtins are extra evil. Please just disable it by default, else you are running the risk of breaking experiment software that uses both ROOT and ZeroMQ because of enabling the builtin. Remember what builtin LZMA without optimizations has caused to CMS already and please don't repeat that mistake. |
|
As for requiring a patched version of something, have a look a this bug: https://bugs.gentoo.org/824018 for an example of the consequences. |
|
What's the release schedule for zeromq versus our mid January ROOT v6.26? |
|
Ok fair enough.
Is that okay? |
|
Sounds reasonable to me. I checked previously that there were no ZeroMQ symbols in RooFitZMQ, so I'm surprised they are back. Indeed, adding a unit test seems ideal to make sure they stay out. On CI, we can still enable |
@Axel-Naumann I don't know actually, I'll ask on Gitter. |
|
@guitargeek How did you find zmq symbols in the libraries? I don't see them with the following check: This is on macOS with most settings default, so with modules. Perhaps the PCH build leaks symbols somehow? |
|
I have seen them with this command (I got it from here): nm libRooFitZMQ.so | c++filt | cut -d ' ' -f 3-99 | sort | uniqI think this was a false alarm, because I forgot to look only for dynamic symbols (passing the But indeed, I think there is no symbol leak because I can't use #include <zmq.h>
int main(int argc, char** argv) {
return ((int*)(&zmq_poll))[argc];
}Compiled with Meaning that as far as I can tell, there is no symbol leak. Still, we have switched off |
Just read $ nm libRooFitZMQ.so | awk '/ T /{print $3}' | c++filtand look for ZeroMQ symbols in the output. |
|
Indeed, this shows no ZeroMQ symbols in @guitargeek Indeed, in #9349, |
In the v6.26 development cycle cycle, some RooFit helper header files
whose names don't start with the `Roo` prefix sneaked into the
RooFitCore library. These headers have the overly generic names
`Floats.h` and `UniqueId.h`, which calls for trouble in environments
where the ROOT headers are directly installed in the main system include
path (most Linux distributions).
To prevent any possible issues, this commit proposes and follows new
conventions for RooFit headers:
* Installed RooFit headers must start with `Roo*` or must be located
in a subdirectory starting with `Roo*` (e.g. RooFit or RooStats).
* Similarly, if the class name doesn't start with Roo, it has to go in
a `Roo*` namespace (usually `RooFit`)
* Free functions always need to go in this namespace
* For implmentation details that we can't avoid installing, we can use
a `Roo*::Detail` namespace like we have with `ROOT::Detail` (same
with `Experimental`)
We should also keep in mind what we established in PR root-project#9078:
* `inc/` is for installed headers
* `res/` is for headers that are only used at compile time of other
ROOT components
* Headers only used within a library go to `src/`
This means that there is no need in specifiying the headers to install
manually in `roofit/rofitcore/CMakeLists.txt`. We can glob for all the
headers fulfilling the criteria above.
In the v6.26 development cycle cycle, some RooFit helper header files
whose names don't start with the `Roo` prefix sneaked into the
RooFitCore library. These headers have the overly generic names
`Floats.h` and `UniqueId.h`, which calls for trouble in environments
where the ROOT headers are directly installed in the main system include
path (most Linux distributions).
To prevent any possible issues, this commit proposes and follows new
conventions for RooFit headers:
* Installed RooFit headers must start with `Roo*` or must be located
in a subdirectory starting with `Roo*` (e.g. RooFit or RooStats).
* Similarly, if the class name doesn't start with Roo, it has to go in
a `Roo*` namespace (usually `RooFit`)
* Free functions always need to go in this namespace
* For implmentation details that we can't avoid installing, we can use
a `Roo*::Detail` namespace like we have with `ROOT::Detail` (same
with `Experimental`)
We should also keep in mind what we established in PR root-project#9078:
* `inc/` is for installed headers
* `res/` is for headers that are only used at compile time of other
ROOT components
* Headers only used within a library go to `src/`
This means that there is no need in specifiying the headers to install
manually in `roofit/rofitcore/CMakeLists.txt`. We can glob for all the
headers fulfilling the criteria above.
In the v6.26 development cycle cycle, some RooFit helper header files
whose names don't start with the `Roo` prefix sneaked into the
RooFitCore library. These headers have the overly generic names
`Floats.h` and `UniqueId.h`, which calls for trouble in environments
where the ROOT headers are directly installed in the main system include
path (most Linux distributions).
To prevent any possible issues, this commit proposes and follows new
conventions for RooFit headers:
* Installed RooFit headers must start with `Roo*` or must be located
in a subdirectory starting with `Roo*` (e.g. RooFit or RooStats).
* Similarly, if the class name doesn't start with Roo, it has to go in
a `Roo*` namespace (usually `RooFit`)
* Free functions always need to go in this namespace
* For implmentation details that we can't avoid installing, we can use
a `Roo*::Detail` namespace like we have with `ROOT::Detail` (same
with `Experimental`)
We should also keep in mind what we established in PR root-project#9078:
* `inc/` is for installed headers
* `res/` is for headers that are only used at compile time of other
ROOT components
* Headers only used within a library go to `src/`
This means that there is no need in specifiying the headers to install
manually in `roofit/rofitcore/CMakeLists.txt`. We can glob for all the
headers fulfilling the criteria above.
In the v6.26 development cycle cycle, some RooFit helper header files
whose names don't start with the `Roo` prefix sneaked into the
RooFitCore library. These headers have the overly generic names
`Floats.h` and `UniqueId.h`, which calls for trouble in environments
where the ROOT headers are directly installed in the main system include
path (most Linux distributions).
To prevent any possible issues, this commit proposes and follows new
conventions for RooFit headers:
* Installed RooFit headers must start with `Roo*` or must be located
in a subdirectory starting with `Roo*` (e.g. RooFit or RooStats).
* Similarly, if the class name doesn't start with Roo, it has to go in
a `Roo*` namespace (usually `RooFit`)
* Free functions always need to go in this namespace
* For implmentation details that we can't avoid installing, we can use
a `Roo*::Detail` namespace like we have with `ROOT::Detail` (same
with `Experimental`)
We should also keep in mind what we established in PR #9078:
* `inc/` is for installed headers
* `res/` is for headers that are only used at compile time of other
ROOT components
* Headers only used within a library go to `src/`
This means that there is no need in specifiying the headers to install
manually in `roofit/rofitcore/CMakeLists.txt`. We can glob for all the
headers fulfilling the criteria above.
This PR is a do-over of #8385 and #8412 and, as such, again the second part of a split and clean-up of #8294. The most important blocker in those PRs was the inclusion of a patched libzmq in RooFitZMQ itself. This patch has now been included in libzmq proper. Another blocking review comment was that libzmq symbols must not be allowed to be exported through our libraries. This has been solved in theory, and in practice is pending another PR to libzmq. Having fixed these two blockers, we should now be able to continue.
To recap:
Will un-draft the PR once the following is done (based on previous review comments by @guitargeek @hageboeck @amadio @lmoneta and also some other things from myself):
use-> next PRenum classinstead of template parameters for minimizer function implementation choiceEdit 18 Nov 2021: the following list is to keep track of unaddressed (at time of writing) comments made in this thread (because the thread is so long that it is very inconvenient to navigate on GitHub which doesn't load it all at once):
inctoresin RooFitZMQ and MultiProcess and only include these zmq header exposing include directories to specific targets that need them usingtarget_include_directories. This way, we don't transitively expose zmq includes to ROOT users.ZMQ_ENABLE_DRAFTpreprocessor defines.Let me know if you find additional items for the todo list.