Skip to content

Commit

Permalink
Update packages (pybind11 and catch2) and do not use nvidia-docker co…
Browse files Browse the repository at this point in the history
…mmand (#618)

### Update Catch2 to v3.4.0

Without upgrading Catch2, The following error occurs when building on
Ubuntu 22.04 due to glibc:
```
cucim/build-debug/_deps/deps-catch2-src/single_include/catch2/catch.hpp:10830:58: error: call to non-‘constexpr’ function
‘long int sysconf(int)’
10830 |     static constexpr std::size_t sigStackSize = 32768 >=  MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;
```
### Update pybind11 to v2.11.1

Even with the latest version of pybind11, we still have an issue
with `pybind11::array_t` when cuCIM is used in multithread without
importing numpy in the main thread.

See pybind/pybind11#4877

Will need to wait for the next release of pybind11.

### Use runtime option instead of using nvidia-docker command

nvidia-docker binary is not available if user doesn't install
nvidia-docker2 package. This change uses runtime option instead
of using nvidia-docker command.

### Apply pybind11 patch to avoid deadlock (until new release is available)

This applies the following patches to pybind11:

- pybind/pybind11#4857
- pybind/pybind11#4877

to avoid deadlock when using pybind11 without importing numpy in
multi-threaded environment.

Authors:
  - Gigon Bae (https://github.com/gigony)
  - Gregory Lee (https://github.com/grlee77)

Approvers:
  - Gregory Lee (https://github.com/grlee77)
  - https://github.com/jakirkham

URL: #618
  • Loading branch information
gigony authored Oct 30, 2023
1 parent 1589a0d commit cd54957
Show file tree
Hide file tree
Showing 24 changed files with 544 additions and 41 deletions.
7 changes: 4 additions & 3 deletions cpp/cmake/deps/catch2.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ if (NOT TARGET deps::catch2)
FetchContent_Declare(
deps-catch2
GIT_REPOSITORY https://github.com/catchorg/Catch2.git
GIT_TAG v2.13.1
GIT_TAG v3.4.0
GIT_SHALLOW TRUE
)
FetchContent_GetProperties(deps-catch2)
Expand All @@ -29,8 +29,9 @@ if (NOT TARGET deps::catch2)

add_subdirectory(${deps-catch2_SOURCE_DIR} ${deps-catch2_BINARY_DIR} EXCLUDE_FROM_ALL)

# Include Append catch2's cmake module path so that we can use `include(ParseAndAddCatchTests)`.
list(APPEND CMAKE_MODULE_PATH "${deps-catch2_SOURCE_DIR}/contrib")
# Include Append catch2's cmake module path so that we can use `include(Catch)`.
# https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md#catchcmake-and-catchaddtestscmake
list(APPEND CMAKE_MODULE_PATH "${deps-catch2_SOURCE_DIR}/extras")
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} PARENT_SCOPE)

add_library(deps::catch2 INTERFACE IMPORTED GLOBAL)
Expand Down
3 changes: 2 additions & 1 deletion cpp/cmake/deps/pybind11.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ if (NOT TARGET deps::pybind11)
FetchContent_Declare(
deps-pybind11
GIT_REPOSITORY https://github.com/pybind/pybind11.git
GIT_TAG v2.10.3
GIT_TAG v2.11.1
GIT_SHALLOW TRUE
PATCH_COMMAND git apply "${CMAKE_CURRENT_LIST_DIR}/pybind11_pr4857_4877.patch" || true
)
FetchContent_GetProperties(deps-pybind11)
if (NOT deps-pybind11_POPULATED)
Expand Down
247 changes: 247 additions & 0 deletions cpp/cmake/deps/pybind11_pr4857_4877.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 87ec103..f2f1d19 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -132,6 +132,7 @@ set(PYBIND11_HEADERS
include/pybind11/embed.h
include/pybind11/eval.h
include/pybind11/gil.h
+ include/pybind11/gil_safe_call_once.h
include/pybind11/iostream.h
include/pybind11/functional.h
include/pybind11/numpy.h
diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h
index 31a54c7..8906c1f 100644
--- a/include/pybind11/detail/common.h
+++ b/include/pybind11/detail/common.h
@@ -118,6 +118,14 @@
# endif
#endif

+#if defined(PYBIND11_CPP20)
+# define PYBIND11_CONSTINIT constinit
+# define PYBIND11_DTOR_CONSTEXPR constexpr
+#else
+# define PYBIND11_CONSTINIT
+# define PYBIND11_DTOR_CONSTEXPR
+#endif
+
// Compiler version assertions
#if defined(__INTEL_COMPILER)
# if __INTEL_COMPILER < 1800
diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h
index 570a558..da22f48 100644
--- a/include/pybind11/gil.h
+++ b/include/pybind11/gil.h
@@ -11,6 +11,8 @@

#include "detail/common.h"

+#include <cassert>
+
#if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT)
# include "detail/internals.h"
#endif
@@ -137,7 +139,9 @@ private:

class gil_scoped_release {
public:
+ // PRECONDITION: The GIL must be held when this constructor is called.
explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) {
+ assert(PyGILState_Check());
// `get_internals()` must be called here unconditionally in order to initialize
// `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
// initialization race could occur as multiple threads try `gil_scoped_acquire`.
@@ -201,7 +205,11 @@ class gil_scoped_release {
PyThreadState *state;

public:
- gil_scoped_release() : state{PyEval_SaveThread()} {}
+ // PRECONDITION: The GIL must be held when this constructor is called.
+ gil_scoped_release() {
+ assert(PyGILState_Check());
+ state = PyEval_SaveThread();
+ }
gil_scoped_release(const gil_scoped_release &) = delete;
gil_scoped_release &operator=(const gil_scoped_release &) = delete;
~gil_scoped_release() { PyEval_RestoreThread(state); }
diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h
new file mode 100644
index 0000000..58b90b8
--- /dev/null
+++ b/include/pybind11/gil_safe_call_once.h
@@ -0,0 +1,90 @@
+// Copyright (c) 2023 The pybind Community.
+
+
+#include "detail/common.h"
+#include "gil.h"
+
+#include <cassert>
+#include <mutex>
+
+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
+
+// Use the `gil_safe_call_once_and_store` class below instead of the naive
+//
+// static auto imported_obj = py::module_::import("module_name"); // BAD, DO NOT USE!
+//
+// which has two serious issues:
+//
+// 1. Py_DECREF() calls potentially after the Python interpreter was finalized already, and
+// 2. deadlocks in multi-threaded processes (because of missing lock ordering).
+//
+// The following alternative avoids both problems:
+//
+// PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> storage;
+// auto &imported_obj = storage // Do NOT make this `static`!
+// .call_once_and_store_result([]() {
+// return py::module_::import("module_name");
+// })
+// .get_stored();
+//
+// The parameter of `call_once_and_store_result()` must be callable. It can make
+// CPython API calls, and in particular, it can temporarily release the GIL.
+//
+// `T` can be any C++ type, it does not have to involve CPython API types.
+//
+// The behavior with regard to signals, e.g. `SIGINT` (`KeyboardInterrupt`),
+// is not ideal. If the main thread is the one to actually run the `Callable`,
+// then a `KeyboardInterrupt` will interrupt it if it is running normal Python
+// code. The situation is different if a non-main thread runs the
+// `Callable`, and then the main thread starts waiting for it to complete:
+// a `KeyboardInterrupt` will not interrupt the non-main thread, but it will
+// get processed only when it is the main thread's turn again and it is running
+// normal Python code. However, this will be unnoticeable for quick call-once
+// functions, which is usually the case.
+template <typename T>
+class gil_safe_call_once_and_store {
+public:
+ // PRECONDITION: The GIL must be held when `call_once_and_store_result()` is called.
+ template <typename Callable>
+ gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) {
+ if (!is_initialized_) { // This read is guarded by the GIL.
+ // Multiple threads may enter here, because the GIL is released in the next line and
+ // CPython API calls in the `fn()` call below may release and reacquire the GIL.
+ gil_scoped_release gil_rel; // Needed to establish lock ordering.
+ std::call_once(once_flag_, [&] {
+ // Only one thread will ever enter here.
+ gil_scoped_acquire gil_acq;
+ ::new (storage_) T(fn()); // fn may release, but will reacquire, the GIL.
+ is_initialized_ = true; // This write is guarded by the GIL.
+ });
+ // All threads will observe `is_initialized_` as true here.
+ }
+ // Intentionally not returning `T &` to ensure the calling code is self-documenting.
+ return *this;
+ }
+
+ // This must only be called after `call_once_and_store_result()` was called.
+ T &get_stored() {
+ assert(is_initialized_);
+ PYBIND11_WARNING_PUSH
+#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5
+ // Needed for gcc 4.8.5
+ PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing")
+#endif
+ return *reinterpret_cast<T *>(storage_);
+ PYBIND11_WARNING_POP
+ }
+
+ constexpr gil_safe_call_once_and_store() = default;
+ PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default;
+
+private:
+ alignas(T) char storage_[sizeof(T)] = {};
+ std::once_flag once_flag_ = {};
+ bool is_initialized_ = false;
+ // The `is_initialized_`-`storage_` pair is very similar to `std::optional`,
+ // but the latter does not have the triviality properties of former,
+ // therefore `std::optional` is not a viable alternative here.
+};
+
+PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h
index 36077ec..1217c8d 100644
--- a/include/pybind11/numpy.h
+++ b/include/pybind11/numpy.h
@@ -10,7 +10,10 @@
#pragma once

#include "pybind11.h"
+#include "detail/common.h"
#include "complex.h"
+#include "gil_safe_call_once.h"
+#include "pytypes.h"

#include <algorithm>
#include <array>
@@ -120,6 +123,20 @@ inline numpy_internals &get_numpy_internals() {
return *ptr;
}

+PYBIND11_NOINLINE module_ import_numpy_core_submodule(const char *submodule_name) {
+ module_ numpy = module_::import("numpy");
+ str version_string = numpy.attr("__version__");
+
+ module_ numpy_lib = module_::import("numpy.lib");
+ object numpy_version = numpy_lib.attr("NumpyVersion")(version_string);
+ int major_version = numpy_version.attr("major").cast<int>();
+
+ /* `numpy.core` was renamed to `numpy._core` in NumPy 2.0 as it officially
+ became a private module. */
+ std::string numpy_core_path = major_version >= 2 ? "numpy._core" : "numpy.core";
+ return module_::import((numpy_core_path + "." + submodule_name).c_str());
+}
+
template <typename T>
struct same_size {
template <typename U>
@@ -192,8 +209,8 @@ struct npy_api {
};

static npy_api &get() {
- static npy_api api = lookup();
- return api;
+ PYBIND11_CONSTINIT static gil_safe_call_once_and_store<npy_api> storage;
+ return storage.call_once_and_store_result(lookup).get_stored();
}

bool PyArray_Check_(PyObject *obj) const {
@@ -263,9 +280,13 @@ private:
};

static npy_api lookup() {
- module_ m = module_::import("numpy.core.multiarray");
+ module_ m = detail::import_numpy_core_submodule("multiarray");
auto c = m.attr("_ARRAY_API");
void **api_ptr = (void **) PyCapsule_GetPointer(c.ptr(), nullptr);
+ if (api_ptr == nullptr) {
+ raise_from(PyExc_SystemError, "FAILURE obtaining numpy _ARRAY_API pointer.");
+ throw error_already_set();
+ }
npy_api api;
#define DECL_NPY_API(Func) api.Func##_ = (decltype(api.Func##_)) api_ptr[API_##Func];
DECL_NPY_API(PyArray_GetNDArrayCFeatureVersion);
@@ -625,13 +646,14 @@ public:
char flags() const { return detail::array_descriptor_proxy(m_ptr)->flags; }

private:
- static object _dtype_from_pep3118() {
- static PyObject *obj = module_::import("numpy.core._internal")
- .attr("_dtype_from_pep3118")
- .cast<object>()
- .release()
- .ptr();
- return reinterpret_borrow<object>(obj);
+ static object &_dtype_from_pep3118() {
+ PYBIND11_CONSTINIT static gil_safe_call_once_and_store<object> storage;
+ return storage
+ .call_once_and_store_result([]() {
+ return detail::import_numpy_core_submodule("_internal")
+ .attr("_dtype_from_pep3118");
+ })
+ .get_stored();
}

dtype strip_padding(ssize_t itemsize) {
7 changes: 4 additions & 3 deletions cpp/plugins/cucim.kit.cumed/cmake/deps/catch2.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ if (NOT TARGET deps::catch2)
FetchContent_Declare(
deps-catch2
GIT_REPOSITORY https://github.com/catchorg/Catch2.git
GIT_TAG v2.13.1
GIT_TAG v3.4.0
GIT_SHALLOW TRUE
)
FetchContent_GetProperties(deps-catch2)
Expand All @@ -29,8 +29,9 @@ if (NOT TARGET deps::catch2)

add_subdirectory(${deps-catch2_SOURCE_DIR} ${deps-catch2_BINARY_DIR} EXCLUDE_FROM_ALL)

# Include Append catch2's cmake module path so that we can use `include(ParseAndAddCatchTests)`.
list(APPEND CMAKE_MODULE_PATH "${deps-catch2_SOURCE_DIR}/contrib")
# Include Append catch2's cmake module path so that we can use `include(Catch)`.
# https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md#catchcmake-and-catchaddtestscmake
list(APPEND CMAKE_MODULE_PATH "${deps-catch2_SOURCE_DIR}/extras")
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} PARENT_SCOPE)

add_library(deps::catch2 INTERFACE IMPORTED GLOBAL)
Expand Down
6 changes: 3 additions & 3 deletions cpp/plugins/cucim.kit.cumed/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@ target_include_directories(cumed_tests
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../src>
)

include(ParseAndAddCatchTests)
# See https://github.com/catchorg/Catch2/blob/master/docs/cmake-integration.md#parseandaddcatchtestscmake for other options
ParseAndAddCatchTests(cumed_tests)
include(Catch)
# See https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md#catchcmake-and-catchaddtestscmake for other options
catch_discover_tests(cumed_tests)
7 changes: 4 additions & 3 deletions cpp/plugins/cucim.kit.cumed/tests/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,16 @@
* limitations under the License.
*/

//#define CATCH_CONFIG_MAIN
//#include <catch2/catch.hpp>
// #define CATCH_CONFIG_MAIN
// #include <catch2/catch_test_macros.hpp>

// Implement main explicitly to handle additional parameters.
#define CATCH_CONFIG_RUNNER
#include "config.h"
#include "cucim/core/framework.h"

#include <catch2/catch.hpp>
#include <catch2/catch_test_macros.hpp>
#include <catch2/catch_session.hpp>
#include <string>
#include <fmt/format.h>

Expand Down
2 changes: 1 addition & 1 deletion cpp/plugins/cucim.kit.cumed/tests/test_basic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#include "config.h"

#include <catch2/catch.hpp>
#include <catch2/catch_test_macros.hpp>
#include <chrono>

TEST_CASE("Verify file", "[test_basic.cpp]")
Expand Down
7 changes: 4 additions & 3 deletions cpp/plugins/cucim.kit.cuslide/cmake/deps/catch2.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ if (NOT TARGET deps::catch2)
FetchContent_Declare(
deps-catch2
GIT_REPOSITORY https://github.com/catchorg/Catch2.git
GIT_TAG v2.13.1
GIT_TAG v3.4.0
GIT_SHALLOW TRUE
)
FetchContent_GetProperties(deps-catch2)
Expand All @@ -29,8 +29,9 @@ if (NOT TARGET deps::catch2)

add_subdirectory(${deps-catch2_SOURCE_DIR} ${deps-catch2_BINARY_DIR} EXCLUDE_FROM_ALL)

# Include Append catch2's cmake module path so that we can use `include(ParseAndAddCatchTests)`.
list(APPEND CMAKE_MODULE_PATH "${deps-catch2_SOURCE_DIR}/contrib")
# Include Append catch2's cmake module path so that we can use `include(Catch)`.
# https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md#catchcmake-and-catchaddtestscmake
list(APPEND CMAKE_MODULE_PATH "${deps-catch2_SOURCE_DIR}/extras")
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} PARENT_SCOPE)

add_library(deps::catch2 INTERFACE IMPORTED GLOBAL)
Expand Down
6 changes: 3 additions & 3 deletions cpp/plugins/cucim.kit.cuslide/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,6 @@ target_include_directories(cuslide_tests
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../src>
)

include(ParseAndAddCatchTests)
# See https://github.com/catchorg/Catch2/blob/master/docs/cmake-integration.md#parseandaddcatchtestscmake for other options
ParseAndAddCatchTests(cuslide_tests)
include(Catch)
# See https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md#catchcmake-and-catchaddtestscmake for other options
catch_discover_tests(cuslide_tests)
7 changes: 4 additions & 3 deletions cpp/plugins/cucim.kit.cuslide/tests/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,16 @@
* limitations under the License.
*/

//#define CATCH_CONFIG_MAIN
//#include <catch2/catch.hpp>
// #define CATCH_CONFIG_MAIN
// #include <catch2/catch_test_macros.hpp>

// Implement main explicitly to handle additional parameters.
#define CATCH_CONFIG_RUNNER
#include "config.h"
#include "cucim/core/framework.h"

#include <catch2/catch.hpp>
#include <catch2/catch_test_macros.hpp>
#include <catch2/catch_session.hpp>
#include <string>
#include <fmt/format.h>

Expand Down
2 changes: 1 addition & 1 deletion cpp/plugins/cucim.kit.cuslide/tests/test_philips_tiff.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include "cuslide/tiff/tiff.h"
#include "config.h"

#include <catch2/catch.hpp>
#include <catch2/catch_test_macros.hpp>
#include <chrono>

TEST_CASE("Verify philips tiff file", "[test_philips_tiff.cpp]")
Expand Down
Loading

0 comments on commit cd54957

Please sign in to comment.