RooFit::MultiProcess & TestStatistics part 2: ZeroMQ#8385
Conversation
|
Starting build on |
|
Build failed on mac11.0/cxx17. Warnings:
|
|
Build failed on mac1014/python3. Warnings:
|
|
The unused parameter warnings would be fixed with this PR: zeromq/libzmq#4211 The dtor-name warnings are fixed in a later ZeroMQ version. I first want to include the RooFitZMQ and MultiProcess things before I start changing the ZeroMQ version, to avoid introducing potential errors already (didn't try it yet). If a newer version doesn't work, I will suppress the warnings for now. |
|
Starting build on |
|
Starting build on |
The files in here are modified versions of the ZMQ convenience functions from https://gitlab.cern.ch/raaij/generate_and_sort, contributed by @roelaaij.
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
👉 View analysis in DeepCode’s Dashboard | Configure the bot👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin. |
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
This is a reimplementation of ZMQ's poll function that mimics the POSIX ppoll function. The difference with regular poll is that ppoll blocks incoming POSIX signals, allowing graceful handling of these signals. We use this in RooFit::MultiProcess to shut down forked child processes. The motivation behind this addition is discussed more in this blog post: https://blog.esciencecenter.nl/combining-zeromq-posix-signals-b754f6f29cd6, or more succinctly in Martin Sustrik's blog post, in the edit at the bottom: https://250bpm.com/blog:12/
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on mac1014/python3. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
The option builtin_zeromq is set to ON by default and controls whether RooFitZMQ is built. At some point, we should also add the option to use a system installed ZeroMQ, so that people can avoid the built-in, if they wish, e.g. for the conda-forge build.
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests:
|
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests:
|
This gets rid of deprecation warnings.
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
|
I have fixed remaining build warnings and added installation of the ZMQ headers to the CI clang-tidy script, which seems to fix all CI builds. The JupyROOT failure is something in master, unrelated to this PR. So, pending a review of @oshadura or @Axel-Naumann on the CMake built-in part, I think this PR should be ready for merging. |
|
@egpbos Hi, I just landed here while trying to catch up with messages on Mattermost's Pull Requests channel... My advice is to write the builtin option last, after all work on adding roofitZMQ is done. First, please provide just an implementation using only an external version of ZeroMQ. Then, after that is done, you can add a separate commit that adds the builtin option for ZeroMQ. The builtin option should be optional and never enabled by default (See ROOT-9385 in Jira for more info). Building rootfitZMQ should not be controlled by the option that enables the builtin, but by its own option, which should respect the option |
|
Thanks @amadio for the extensive input! Let me address it point by point: First: the current choice for "built-in only". The built-in is there for two reasons: 1) as a dependency, obviously, but 2) also because the full source is needed to build So, long story, I hope it is clear. Do you think that given this (temporary) situation, the choice for built-in only is acceptable? Then about the RooFitZMQ option: agreed, I should add an option for that. The only catch is that this PR is part of a bigger series and the "real" end-product that should be optional in my opinion is RooFit::MultiProcess, so I wanted to wait with such an option until that PR (hopefully I'll be able to submit that one today). In this PR, RooFitZMQ is not used for anything and later on it will just be a library for RooFit::MultiProcess as well, so there would typically not really be a point for a user to want to build RooFitZMQ but not RooFit::MultiProcess. Do you think in this situation it makes sense to add the option for RooFitZMQ already or should I just do it for RooFit::MultiProcess? So, this option (say I think that fail-on-missing is only relevant in case there would be a non built-in option as well, right? Finally, about git history: makes sense. I can do some reordering before merging. Otherwise, just squashing everything would make sense to me as well. The two independent things here are basically 1) the zmq builtin and 2) RooFitZMQ itself. These could indeed just be two commits, but since nothing really depends on RooFitZMQ at this point, including it with the libzmq builtin in one commit I think would give a clean history as well. |
Sorry, now I'm mixing things up :) The MultiProcess PR was already made as well: #8412. Shall I add the correct build option there? |
|
Hi, |
|
Indeed, we need to patch ZeroMQ. The ZeroMQ symbols is a good point, I'm not sure about that. How can we check for this? |
Where do you do that? Can you show the location in the relevant PR? |
|
It's the ppoll.h/.cpp files: I see now I have to still remove those copyright headers as well. |
|
Starting build on |
Load your RooFit libraries, then do If that shows anything but Bottom line: please use the system zmq. If that is impossible to pull off, then don't use zmq. Else things get extremely involved. We cannot merge this as is; or e.g. ALICE will be unable to use ROOT: they use RooFit and zmq themselves. |
But I thought you're linking to zmq, how do you replace these functions from zmq? |
|
There's no replacement, it's an addition, I'm adding |
So you're not patching? You can use a system libzmq? |
|
That's right, I could (and did) use a system libzmq, except to build the ppoll file, I need to include a few headers from the libzmq source which are not distributed with Linux packages. Previously, I had included those headers with RooFitZMQ. I ripped them out again, because with the built-in they became unnecessary. Edit: indeed, patching is probably not the right word, it's an addition, but it uses libzmq internal headers. Hope I'm making myself clear, sorry for the confusion. |
|
OK I think this is problematic for the above (technical) issues that need to be solved before this can be merged (symbols leaking in and out), and for sustainability reasons, such as "your" zmq library having security issues - who will be aware of this and update our copy? If someone proposes your addition to upstream zmq, once its integrated, and possibly slightly adjusted as part of the integration, who will sort out the differences, and how to we keep "old zmq + extension" and "new zmq" working? IMO the right steps are:
We can then use zmq as regular "builtin", i.e. we take the system one if it's new enough, else people can use the official package that we pull "into" ROOT. |
|
I agree, that is the best way to go in theory. In practice, I'm dealing with time constraints that will make this challenging. The timeline for including In any case, if this is the way we must go (and I can totally understand that you would want it this way), I will do my best to split up the other PRs as much as possible, so that those can be merged independently asap and merging conflicts will hopefully be minimized. |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
Sorry, but no, this is not acceptable if you ask me. In fact, the need for a patched zeromq raises a big red flag as that may make it not possible to ever use a system version of zeromq if your pull request gets stuck. I agree with Axel that the right course of action is to add the needed functionality to zeromq directly first, then add this work to ROOT. I don't think that time constraints should be used as a way to get hackish code into ROOT. Alternatively, if your addition is really independent to zeromq, you could just add it to the RooFitZMQ library instead, so that you can use the system version of zeromq as is. If in a later version they add your new functionality, you can then adapt RooFitZMQ to that in a new pull request.
If RooFitZMQ only makes sense as a support library for RooFit::MultiProcess, it's better to put them together in a single pull request. It will be much easier to ensure everything works together that way.
That is not what users generally think. They want
Ok, as long as it's possible to use a vanilla zeromq, and the builtin addition is the last commit in the series, it should be ok. |
Either way is fine by me. I could easily add the MultiProcess commits here as well, since the current MultiProcess branch is branched off this one anyway. I only separated them for clarity of discussion :) |
|
Made obsolete by #9078. |
6 participants
This PR is the second part of a split and clean-up of #8294, as suggested by @guitargeek.
In this PR, we introduce
threetwo packages:RooFit::MultiProcess,RooFitZMQ and a ZeroMQ builtin to ease dependency management.RooFit::MultiProcess is a task-based parallelization framework.It uses forked processes for parallelization, as opposed to threads. We chose this approach because A) the existing RooRealMPFE parallelization framework already made use of forks as well, so we had something to build on and B) it was at the time deemed infeasible to check the entire RooFit code for thread-safety. Moreover, we use MultiProcess to parallelize gradients -- i.e. the tasks to be executed in parallel are partial derivatives -- and these are sufficiently large tasks that communication in between tasks is not a big concern in the big fits that we aimed to parallelize.Edit: We decided to keep this PR for ZeroMQ only, MultiProcess will come in a next PR.
The communication between processes is done using ZeroMQ. The ZeroMQ dependency is wrapped in convenience classes contributed by @roelaaij which here are packaged as RooFitZMQ.
Note: still working on "cherry picking" all changes and organizing them into functionally sensible commits. Will un-draft the PR once this is done.Ready for review.