-
Notifications
You must be signed in to change notification settings - Fork 1.5k
RooFit::MultiProcess & TestStatistics part 2 redo: RooFitZMQ & MultiProcess #9078
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,35 @@ | ||||||
| include(ExternalProject) | ||||||
|
|
||||||
| ExternalProject_Add(BUILTIN_cppzmq | ||||||
| URL https://github.com/zeromq/cppzmq/archive/refs/tags/v4.8.1.tar.gz | ||||||
| URL_HASH SHA256=7a23639a45f3a0049e11a188e29aaedd10b2f4845f0000cf3e22d6774ebde0af | ||||||
| CONFIGURE_COMMAND "" | ||||||
| BUILD_COMMAND "" | ||||||
| INSTALL_COMMAND "" | ||||||
| DEPENDS libzmq | ||||||
| ) | ||||||
|
|
||||||
| # manually copy header (inspired by nlohmann builtin) | ||||||
| set(cppzmq_HEADER_PATH ${CMAKE_CURRENT_BINARY_DIR}/BUILTIN_cppzmq-prefix/src/BUILTIN_cppzmq) | ||||||
| add_custom_command( | ||||||
| OUTPUT ${CMAKE_BINARY_DIR}/include/zmq.hpp | ||||||
| COMMAND ${CMAKE_COMMAND} -E copy ${cppzmq_HEADER_PATH}/zmq.hpp ${CMAKE_BINARY_DIR}/include/zmq.hpp | ||||||
| COMMENT "Copying zmq.hpp header to ${CMAKE_BINARY_DIR}/include" | ||||||
| DEPENDS BUILTIN_cppzmq) | ||||||
|
|
||||||
| add_custom_target(builtin_cppzmq_incl | ||||||
| DEPENDS ${CMAKE_BINARY_DIR}/include/zmq.hpp) | ||||||
|
|
||||||
| set_property(GLOBAL APPEND PROPERTY ROOT_HEADER_TARGETS builtin_cppzmq_incl) | ||||||
|
|
||||||
| install(FILES ${cppzmq_HEADER_PATH}/zmq.hpp | ||||||
| DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/) | ||||||
|
Comment on lines
+25
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The |
||||||
|
|
||||||
| # find_package emulation | ||||||
| set(cppzmq_FOUND TRUE CACHE BOOL "" FORCE) | ||||||
| set(cppzmq_INCLUDE_DIR ${CMAKE_BINARY_DIR}/include CACHE INTERNAL "" FORCE) | ||||||
| set(cppzmq_INCLUDE_DIRS ${cppzmq_INCLUDE_DIR} CACHE INTERNAL "" FORCE) | ||||||
|
|
||||||
| add_library(cppzmq INTERFACE IMPORTED GLOBAL) | ||||||
| target_include_directories(cppzmq INTERFACE $<BUILD_INTERFACE:${cppzmq_INCLUDE_DIR}>) | ||||||
| add_dependencies(cppzmq BUILTIN_cppzmq builtin_cppzmq_incl) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| include(ExternalProject) | ||
|
|
||
| set(ZeroMQ_PREFIX ${CMAKE_CURRENT_BINARY_DIR}/BUILTIN_ZeroMQ-prefix) | ||
| set(ZeroMQ_LIBNAME ${CMAKE_STATIC_LIBRARY_PREFIX}zmq${CMAKE_STATIC_LIBRARY_SUFFIX}) | ||
|
|
||
| set(ZeroMQ_LIBRARY ${ZeroMQ_PREFIX}/lib/${ZeroMQ_LIBNAME} CACHE INTERNAL "" FORCE) | ||
| set(ZeroMQ_LIBRARIES ${ZeroMQ_LIBRARY} CACHE INTERNAL "" FORCE) | ||
|
|
||
| if(NOT WIN32) | ||
| # 2021-10-07: libzmq's minimum CMake version does not yet support CXX_VISIBILITY_PRESET, so we need to pass the flag manually | ||
| set(cxx_visibility_flag "-fvisibility=hidden") | ||
| else() | ||
| set(cxx_visibility_flag "") | ||
| endif() | ||
|
egpbos marked this conversation as resolved.
|
||
|
|
||
| ExternalProject_Add(BUILTIN_ZeroMQ | ||
| URL https://github.com/zeromq/libzmq/archive/7c2df78b49a3aa63e654b3f3526adf71ed091534.tar.gz | ||
| URL_HASH SHA256=fcc1b0648afa5d92e0ff0e6e93beb28cbbe008a5f98c228ff97144ba6e4a6c3e | ||
| CMAKE_ARGS | ||
| -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR> | ||
| -DCMAKE_INSTALL_LIBDIR=lib | ||
| -DWITH_PERF_TOOL=OFF | ||
| -DBUILD_TESTS=OFF | ||
| -DENABLE_DRAFTS=ON | ||
| -DENABLE_NO_EXPORT=ON | ||
| -DENABLE_CURVE=OFF | ||
| -DWITH_DOCS=OFF | ||
| -DENABLE_WS=OFF | ||
| -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} | ||
| -DCMAKE_CXX_FLAGS=${cxx_visibility_flag}\ ${CMAKE_CXX_FLAGS}\ -w | ||
| BUILD_BYPRODUCTS ${ZeroMQ_LIBRARIES} | ||
| ) | ||
|
|
||
| set(ZeroMQ_FOUND TRUE CACHE BOOL "" FORCE) | ||
|
|
||
| set(ZeroMQ_INCLUDE_DIR ${ZeroMQ_PREFIX}/include CACHE INTERNAL "" FORCE) | ||
| set(ZeroMQ_INCLUDE_DIRS ${ZeroMQ_PREFIX}/include CACHE INTERNAL "" FORCE) | ||
|
guitargeek marked this conversation as resolved.
|
||
|
|
||
| # Workaround to propagate INTERFACE_INCLUDE_DIRECTORIES, see https://stackoverflow.com/a/47358004/1199693 | ||
| file(MAKE_DIRECTORY ${ZeroMQ_INCLUDE_DIR}) | ||
|
|
||
| add_library(libzmq INTERFACE IMPORTED GLOBAL) | ||
| target_include_directories(libzmq INTERFACE $<BUILD_INTERFACE:${ZeroMQ_INCLUDE_DIR}>) | ||
| target_link_libraries(libzmq INTERFACE $<BUILD_INTERFACE:${ZeroMQ_LIBRARIES}>) | ||
| add_dependencies(libzmq BUILTIN_ZeroMQ) | ||
|
|
||
| set(ZeroMQ_DIR ${ZeroMQ_PREFIX}/share/cmake/ZeroMQ CACHE INTERNAL "" FORCE) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # Based on https://github.com/zeromq/cppzmq/blob/a98fa4a91d868a3844e5456741d6782cc1a8d98b/libzmq-pkg-config/FindZeroMQ.cmake | ||
| # MIT Licensed: | ||
| # Permission is hereby granted, free of charge, to any person obtaining a copy | ||
| # of this software and associated documentation files (the "Software"), to | ||
| # deal in the Software without restriction, including without limitation the | ||
| # rights to use, copy, modify, merge, publish, distribute, sublicense, and/or | ||
| # sell copies of the Software, and to permit persons to whom the Software is | ||
| # furnished to do so, subject to the following conditions: | ||
| # | ||
| # The above copyright notice and this permission notice shall be included in | ||
| # all copies or substantial portions of the Software. | ||
| # | ||
| # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING | ||
| # FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS | ||
| # IN THE SOFTWARE. | ||
|
|
||
| set(PKG_CONFIG_USE_CMAKE_PREFIX_PATH ON) | ||
| find_package(PkgConfig) | ||
| pkg_check_modules(PC_LIBZMQ QUIET libzmq) | ||
|
|
||
| set(ZeroMQ_VERSION ${PC_LIBZMQ_VERSION}) | ||
|
|
||
| find_path(ZeroMQ_INCLUDE_DIR zmq.h | ||
| PATHS ${ZeroMQ_DIR}/include | ||
| ${PC_LIBZMQ_INCLUDE_DIRS}) | ||
|
|
||
| find_library(ZeroMQ_LIBRARY | ||
| NAMES zmq | ||
| PATHS ${ZeroMQ_DIR}/lib | ||
| ${PC_LIBZMQ_LIBDIR} | ||
| ${PC_LIBZMQ_LIBRARY_DIRS}) | ||
|
|
||
| set ( ZeroMQ_LIBRARIES ${ZeroMQ_LIBRARY} ) | ||
| set ( ZeroMQ_INCLUDE_DIRS ${ZeroMQ_INCLUDE_DIR} ) | ||
|
|
||
| # check for zmq_ppoll | ||
| if(ZeroMQ_FOUND) | ||
| SET(SAVE_CMAKE_REQUIRED_DEFINITIONS "${CMAKE_REQUIRED_DEFINITIONS}") | ||
| SET(CMAKE_REQUIRED_DEFINITIONS "-DZMQ_BUILD_DRAFT_API") | ||
| check_cxx_symbol_exists(zmq_ppoll zmq.h ZeroMQ_HAS_PPOLL) | ||
| SET(CMAKE_REQUIRED_DEFINITIONS "${SAVE_CMAKE_REQUIRED_DEFINITIONS}") | ||
| endif() | ||
|
|
||
| include ( FindPackageHandleStandardArgs ) | ||
| # handle the QUIETLY and REQUIRED arguments and set ZeroMQ_FOUND to TRUE | ||
| # if all listed variables are TRUE | ||
| find_package_handle_standard_args ( ZeroMQ DEFAULT_MSG ZeroMQ_LIBRARIES ZeroMQ_INCLUDE_DIRS ZeroMQ_HAS_PPOLL) | ||
|
|
||
| if(ZeroMQ_FOUND) | ||
| if(NOT TARGET libzmq) | ||
| add_library(libzmq UNKNOWN IMPORTED) | ||
| set_target_properties(libzmq PROPERTIES | ||
| IMPORTED_LOCATION ${ZeroMQ_LIBRARIES} | ||
| INTERFACE_INCLUDE_DIRECTORIES ${ZeroMQ_INCLUDE_DIRS}) | ||
| endif() | ||
| endif() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| if (NOT (ZeroMQ_FOUND OR TARGET libzmq)) | ||
| message(FATAL_ERROR "Search for libzmq first!") | ||
| endif() | ||
|
|
||
| find_path(cppzmq_INCLUDE_DIRS "zmq.hpp" | ||
| HINTS "${ZeroMQ_INCLUDE_DIR}" | ||
| PATH_SUFFIXES "include" "cppzmq" | ||
| ) | ||
| mark_as_advanced(cppzmq_INCLUDE_DIRS) | ||
|
|
||
| include(FindPackageHandleStandardArgs) | ||
| find_package_handle_standard_args (cppzmq DEFAULT_MSG cppzmq_INCLUDE_DIRS) | ||
|
|
||
| if(cppzmq_FOUND) | ||
| add_library(cppzmq INTERFACE IMPORTED) | ||
| set_target_properties(cppzmq PROPERTIES | ||
| INTERFACE_INCLUDE_DIRECTORIES "${CPPZMQ_INCLUDE_DIRS}" | ||
| INTERFACE_LINK_LIBRARIES libzmq | ||
| ) | ||
| set(CPPZMQ_LIBRARIES cppzmq) | ||
| endif() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1862,6 +1862,75 @@ if(testing AND NO_CONNECTION) | |
| endif() | ||
| endif() | ||
|
|
||
| #---Check for ZeroMQ when building RooFit::MultiProcess-------------------------------------------- | ||
|
|
||
| if (roofit_multiprocess AND NOT MSVC) | ||
| if(NOT builtin_zeromq) | ||
| message(STATUS "Looking for ZeroMQ (libzmq)") | ||
| # Clear cache before calling find_package(ZeroMQ), | ||
| # necessary to be able to toggle builtin_zeromq and | ||
| # not have find_package(ZeroMQ) find builtin ZeroMQ. | ||
| foreach(suffix FOUND INCLUDE_DIR INCLUDE_DIRS LIBRARY LIBRARIES) | ||
| unset(ZeroMQ_${suffix} CACHE) | ||
| endforeach() | ||
|
|
||
| # Temporarily prefer config mode over module mode, so that a CMake-installed system version | ||
| # gets detected before looking for an autotools-installed system version (which the | ||
| # FindZeroMQ.cmake module does). | ||
| set(CMAKE_FIND_PACKAGE_PREFER_CONFIG_ORIGINAL_VALUE ${CMAKE_FIND_PACKAGE_PREFER_CONFIG}) | ||
| set(CMAKE_FIND_PACKAGE_PREFER_CONFIG TRUE) | ||
|
|
||
| if(fail-on-missing) | ||
| find_package(ZeroMQ REQUIRED) | ||
| else() | ||
| find_package(ZeroMQ) | ||
| if(NOT ZeroMQ_FOUND) | ||
| message(STATUS "ZeroMQ not found. Switching on builtin_zeromq option") | ||
| set(builtin_zeromq ON CACHE BOOL "Enabled because ZeroMQ not found (${builtin_zeromq_description})" FORCE) | ||
| endif() | ||
| endif() | ||
|
|
||
| # Reset default find_package mode | ||
| set(CMAKE_FIND_PACKAGE_PREFER_CONFIG ${CMAKE_FIND_PACKAGE_PREFER_CONFIG_ORIGINAL_VALUE}) | ||
| unset(CMAKE_FIND_PACKAGE_PREFER_CONFIG_ORIGINAL_VALUE) | ||
| endif() | ||
|
|
||
| if(builtin_zeromq) | ||
| list(APPEND ROOT_BUILTINS ZeroMQ) | ||
| add_subdirectory(builtins/zeromq/libzmq) | ||
| endif() | ||
|
|
||
| if(NOT builtin_cppzmq) | ||
| message(STATUS "Looking for ZeroMQ C++ bindings (cppzmq)") | ||
| # Clear cache before calling find_package(cppzmq), | ||
| # necessary to be able to toggle builtin_cppzmq and | ||
| # not have find_package(cppzmq) find builtin cppzmq. | ||
| foreach(suffix FOUND INCLUDE_DIR INCLUDE_DIRS) | ||
| unset(cppzmq_${suffix} CACHE) | ||
| endforeach() | ||
| if(fail-on-missing) | ||
| find_package(cppzmq REQUIRED) | ||
| else() | ||
| find_package(cppzmq QUIET) | ||
| if(NOT cppzmq_FOUND) | ||
| message(STATUS "ZeroMQ C++ bindings not found. Switching on builtin_cppzmq option") | ||
| set(builtin_cppzmq ON CACHE BOOL "Enabled because ZeroMQ C++ bindings not found (${builtin_cppzmq_description})" FORCE) | ||
| endif() | ||
| endif() | ||
| endif() | ||
|
|
||
| if(builtin_cppzmq) | ||
| list(APPEND ROOT_BUILTINS cppzmq) | ||
| add_subdirectory(builtins/zeromq/cppzmq) | ||
| endif() | ||
|
|
||
| # zmq_ppoll is still in the draft API, so enable that transitively | ||
| target_compile_definitions(libzmq INTERFACE ZMQ_BUILD_DRAFT_API) | ||
| target_compile_definitions(libzmq INTERFACE ZMQ_NO_EXPORT) | ||
| target_compile_definitions(cppzmq INTERFACE ZMQ_BUILD_DRAFT_API) | ||
| target_compile_definitions(cppzmq INTERFACE ZMQ_NO_EXPORT) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about people not using CMake (i.e. just using this stuff from the prompt or compiling macros by hand)? Won't they have problems if these are not defined? I think the wrapping header solution proposed by @guitargeek is safer in this regard. And also, how is this working when ZeroMQ and cppzmq are not builtin?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is a very good point. I was discussing with @guitargeek looking into whether we can hide away the ZeroMQ includes from the regular RooFit(Core) user, also connected to @Axel-Naumann's question about whether users have to include Job.h (regular RooFit users shouldn't, but
I apply the compile_definitions on the targets after the builtin-or-not branches. So they are also applied to non-builtin found targets that would be defined by |
||
| endif (roofit_multiprocess AND NOT MSVC) | ||
|
|
||
| #---Download googletest-------------------------------------------------------------- | ||
| if (testing) | ||
| # FIXME: Remove our version of gtest in roottest. We can reuse this one. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| ############################################################################ | ||
| # CMakeLists.txt file for building ROOT RooFitMultiProcess package | ||
| # @author Patrick Bos, Netherlands eScience Center | ||
| ############################################################################ | ||
|
|
||
| ROOT_LINKER_LIBRARY(RooFitMultiProcess | ||
| src/worker.cxx | ||
| src/Messenger.cxx | ||
| src/ProcessManager.cxx | ||
| src/util.cxx | ||
| src/Queue.cxx | ||
| src/JobManager.cxx | ||
| src/Job.cxx | ||
| DEPENDENCIES | ||
| RooFitZMQ | ||
| ) | ||
|
|
||
| target_link_libraries(RooFitMultiProcess PUBLIC RooFitZMQ) | ||
| set(RooFitMultiProcess_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/res ${RooFitZMQ_INCLUDE_DIRS}) | ||
| target_include_directories(RooFitMultiProcess PRIVATE ${RooFitMultiProcess_INCLUDE_DIRS}) | ||
|
|
||
| ROOT_ADD_TEST_SUBDIRECTORY(test) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
|
egpbos marked this conversation as resolved.
|
||
| * Project: RooFit | ||
| * Authors: | ||
| * PB, Patrick Bos, Netherlands eScience Center, p.bos@esciencecenter.nl | ||
| * IP, Inti Pelupessy, Netherlands eScience Center, i.pelupessy@esciencecenter.nl | ||
| * | ||
| * Copyright (c) 2021, CERN | ||
| * | ||
| * Redistribution and use in source and binary forms, | ||
| * with or without modification, are permitted according to the terms | ||
| * listed in LICENSE (http://roofit.sourceforge.net/license.txt) | ||
| */ | ||
| #ifndef ROOT_ROOFIT_MultiProcess_Job_decl | ||
| #define ROOT_ROOFIT_MultiProcess_Job_decl | ||
|
|
||
| #include <string> | ||
| #include <zmq.hpp> | ||
|
|
||
| namespace RooFit { | ||
| namespace MultiProcess { | ||
|
|
||
| // forward declaration | ||
| class JobManager; | ||
|
|
||
| class Job { | ||
| public: | ||
| explicit Job(); | ||
| Job(const Job &other); | ||
|
|
||
| ~Job(); | ||
|
|
||
| virtual void evaluate_task(std::size_t task) = 0; | ||
| virtual void update_state(); | ||
|
|
||
| virtual void send_back_task_result_from_worker(std::size_t task) = 0; | ||
| virtual bool receive_task_result_on_master(const zmq::message_t &message) = 0; | ||
|
|
||
| void gather_worker_results(); | ||
|
|
||
| std::size_t get_state_id(); | ||
|
|
||
| protected: | ||
| JobManager *get_manager(); | ||
|
|
||
| std::size_t id_; | ||
| std::size_t state_id_ = 0; | ||
|
|
||
| private: | ||
| // do not use _manager directly, it must first be initialized! use get_manager() | ||
| JobManager *_manager = nullptr; | ||
| }; | ||
|
|
||
| } // namespace MultiProcess | ||
| } // namespace RooFit | ||
|
|
||
| #endif // ROOT_ROOFIT_MultiProcess_Job_decl | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about what's correct here, but what is the reason that you only have
BUILTIN_cppzmqand notBUILTIN_ZeroMQ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because clang-tidy would complain otherwise, because it couldn't find
zmq.hppto include.zmq.his not directly included in the files that clang-tidy checks themselves, so it's not necessary to add libzmq headers at this point.Note that currently clang-tidy CI is not running at all anymore. I made a PR to fix this, perhaps good to merge that soon #8530
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it was clang-tidy or clang-format actually (the PR adds clang-format), but anyway, one of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can revert this change now, because
zmq.hppis not included in any RooFit interfaces.