From 80a04a2b5004159a8310097f051c66e658dbdd16 Mon Sep 17 00:00:00 2001 From: Howard Butler Date: Tue, 2 Jan 2024 14:19:56 -0600 Subject: [PATCH 1/5] manage GIL acquisition when doing things inside of pdal::plang --- .github/workflows/build.yml | 2 +- pdal/filters/PythonFilter.cpp | 1 + pdal/plang/Environment.cpp | 3 +++ pdal/plang/Environment.hpp | 1 + pdal/plang/Invocation.cpp | 2 ++ pdal/plang/Redirector.cpp | 14 ++++++++++ pdal/plang/gil.hpp | 50 +++++++++++++++++++++++++++++++++++ setup.py | 2 +- 8 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 pdal/plang/gil.hpp diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 0bbc47e..d36044a 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -84,7 +84,7 @@ jobs: python setup.py sdist ls dist - - uses: pypa/gh-action-pypi-publish@master + - uses: pypa/gh-action-pypi-publish@release/v1 name: Publish package if: github.event_name == 'release' && github.event.action == 'published' with: diff --git a/pdal/filters/PythonFilter.cpp b/pdal/filters/PythonFilter.cpp index 993a8a4..38a4feb 100644 --- a/pdal/filters/PythonFilter.cpp +++ b/pdal/filters/PythonFilter.cpp @@ -148,6 +148,7 @@ PointViewSet PythonFilter::run(PointViewPtr view) log()->get(LogLevel::Debug5) << "filters.python " << *m_script << " processing " << (int)view->size() << " points." << std::endl; + plang::gil_scoped_acquire acquire; m_pythonMethod->execute(view, getMetadata()); PointViewSet viewSet; diff --git a/pdal/plang/Environment.cpp b/pdal/plang/Environment.cpp index fbded80..5f9e98d 100644 --- a/pdal/plang/Environment.cpp +++ b/pdal/plang/Environment.cpp @@ -116,6 +116,7 @@ EnvironmentPtr Environment::get() { g_environment = new Environment(); }; + gil_scoped_acquire acquire; std::call_once(flag, init); return g_environment; } @@ -150,8 +151,10 @@ Environment::Environment() throw pdal_error("unable to add redirector module!"); } + initNumpy(); PyImport_ImportModule("redirector"); + } Environment::~Environment() diff --git a/pdal/plang/Environment.hpp b/pdal/plang/Environment.hpp index d8c8433..fed4fd6 100644 --- a/pdal/plang/Environment.hpp +++ b/pdal/plang/Environment.hpp @@ -47,6 +47,7 @@ #include "Redirector.hpp" #include "Script.hpp" +#include "gil.hpp" namespace pdal { diff --git a/pdal/plang/Invocation.cpp b/pdal/plang/Invocation.cpp index b92b087..41b691d 100644 --- a/pdal/plang/Invocation.cpp +++ b/pdal/plang/Invocation.cpp @@ -122,6 +122,7 @@ Invocation::Invocation(const Script& script, MetadataNode m, m_script(script), m_inputMetadata(m), m_pdalargs(pdalArgs) { Environment::get(); + gil_scoped_acquire acquire; compile(); } @@ -357,6 +358,7 @@ PyObject* getPyJSON(std::string const& s) // Returns a new reference to a dictionary of numpy arrays/names. PyObject *Invocation::prepareData(PointViewPtr& view) { + gil_scoped_acquire acquire; PointLayoutPtr layout(view->m_pointTable.layout()); Dimension::IdList const& dims = layout->dims(); diff --git a/pdal/plang/Redirector.cpp b/pdal/plang/Redirector.cpp index d510db5..69f2774 100644 --- a/pdal/plang/Redirector.cpp +++ b/pdal/plang/Redirector.cpp @@ -7,6 +7,7 @@ // Blog article: http://mateusz.loskot.net/?p=2819 #include "Redirector.hpp" +#include "gil.hpp" #pragma warning(disable: 4127) // conditional expression is constant #pragma warning(disable: 4068) // gcc pragma warnings @@ -30,6 +31,7 @@ struct Stdout static PyObject* Stdout_write(PyObject* self, PyObject* args) { + gil_scoped_acquire acquire; std::size_t written(0); Stdout* selfimpl = reinterpret_cast(self); if (selfimpl->write) @@ -48,6 +50,7 @@ static PyObject* Stdout_write(PyObject* self, PyObject* args) static PyObject* Stdout_flush(PyObject* self, PyObject* /*args*/) { + gil_scoped_acquire acquire; Stdout *selfimpl = reinterpret_cast(self); if (selfimpl->flush) { @@ -151,9 +154,12 @@ static struct PyModuleDef redirectordef = { PyObject* Redirector::init() { + gil_scoped_acquire acquire; StdoutType.tp_new = PyType_GenericNew; if (PyType_Ready(&StdoutType) < 0) + { return NULL; + } PyObject* m = PyModule_Create(&redirectordef); if (m) { @@ -171,6 +177,8 @@ PyObject* Redirector::init() void Redirector::set_stdout(std::ostream* ostr) { + gil_scoped_acquire acquire; + stdout_write_type writeFunc = [ostr](std::string msg) { *ostr << msg; }; stdout_flush_type flushFunc = [ostr]{ ostr->flush(); }; @@ -180,6 +188,8 @@ void Redirector::set_stdout(std::ostream* ostr) void Redirector::set_stdout(stdout_write_type write, stdout_flush_type flush) { + gil_scoped_acquire acquire; + if (!m_stdout) { m_stdout_saved = @@ -196,11 +206,15 @@ void Redirector::set_stdout(stdout_write_type write, stdout_flush_type flush) void Redirector::reset_stdout() { + PyGILState_STATE gstate; + gstate = PyGILState_Ensure(); + if (m_stdout_saved) PySys_SetObject(const_cast("stdout"), m_stdout_saved); Py_XDECREF(m_stdout); m_stdout = 0; + PyGILState_Release(gstate); } } //namespace plang diff --git a/pdal/plang/gil.hpp b/pdal/plang/gil.hpp new file mode 100644 index 0000000..2c03aa4 --- /dev/null +++ b/pdal/plang/gil.hpp @@ -0,0 +1,50 @@ +/* + pybind11/gil.h: RAII helpers for managing the GIL + + Copyright (c) 2016 Wenzel Jakob + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#pragma once + +#include + + +namespace pdal +{ +namespace plang +{ + + +class gil_scoped_acquire { + PyGILState_STATE state; + +public: + gil_scoped_acquire() : state{PyGILState_Ensure()} {} + gil_scoped_acquire(const gil_scoped_acquire &) = delete; + gil_scoped_acquire &operator=(const gil_scoped_acquire &) = delete; + ~gil_scoped_acquire() { PyGILState_Release(state); } + void disarm() {} +}; + +class gil_scoped_release { + PyThreadState *state; + +public: + // 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); } + void disarm() {} +}; + + +} // plang + +} // pdal diff --git a/setup.py b/setup.py index e57bda2..7639d3b 100644 --- a/setup.py +++ b/setup.py @@ -5,7 +5,7 @@ setup( name="pdal-plugins", - version="1.2.1", + version="1.3.0", description="Point cloud data processing Python plugins", license="BSD", keywords="point cloud spatial", From fd8d4f206a9820c7c92fb89abc804e24ed92fb67 Mon Sep 17 00:00:00 2001 From: Howard Butler Date: Tue, 2 Jan 2024 14:26:30 -0600 Subject: [PATCH 2/5] healthify CI --- .github/workflows/build.yml | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d36044a..b1c640d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -24,19 +24,23 @@ jobs: fail-fast: true matrix: os: ['ubuntu-latest', 'macos-latest', 'windows-latest'] - python-version: ['3.8', '3.9', '3.10'] + python-version: ['3.9', '3.10', '3.11', '3.12'] steps: - name: Check out - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup micromamba - uses: mamba-org/provision-with-micromamba@main + uses: conda-incubator/setup-miniconda@v3 with: + miniforge-variant: Mambaforge + miniforge-version: latest + use-mamba: true + auto-update-conda: true environment-file: .github/environment.yml extra-specs: | python=${{ matrix.python-version }} - sel(linux): compilers + - name: Install shell: bash -l {0} @@ -67,11 +71,14 @@ jobs: python-version: ['3.9'] steps: - - uses: actions/checkout@v2 - - uses: conda-incubator/setup-miniconda@v2 + - uses: actions/checkout@v4 + - name: Setup micromamba + uses: conda-incubator/setup-miniconda@v3 with: - channels: conda-forge - python-version: ${{ matrix.python-version }} + miniforge-variant: Mambaforge + miniforge-version: latest + use-mamba: true + auto-update-conda: true mamba-version: "*" - name: Dependencies From 0c29a084ed3fa41a19d60c1c8a33163368430c9e Mon Sep 17 00:00:00 2001 From: Howard Butler Date: Tue, 2 Jan 2024 15:01:49 -0600 Subject: [PATCH 3/5] grab the GIL when our plugins are intitializing the interpreter --- pdal/plang/Environment.cpp | 13 ++++++++++++- pdal/plang/gil.hpp | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pdal/plang/Environment.cpp b/pdal/plang/Environment.cpp index 5f9e98d..a8e7c27 100644 --- a/pdal/plang/Environment.cpp +++ b/pdal/plang/Environment.cpp @@ -114,9 +114,20 @@ EnvironmentPtr Environment::get() auto init = []() { + PyGILState_STATE gstate; + + // If the interpreter is already initialized, we need to + // grab the GIL and hold it before we go do any Python + // stuff + bool alreadyInitialized(Py_IsInitialized()); + if (alreadyInitialized) + gstate = PyGILState_Ensure(); + g_environment = new Environment(); + + if (alreadyInitialized) + PyGILState_Release(gstate); }; - gil_scoped_acquire acquire; std::call_once(flag, init); return g_environment; } diff --git a/pdal/plang/gil.hpp b/pdal/plang/gil.hpp index 2c03aa4..5ce57e2 100644 --- a/pdal/plang/gil.hpp +++ b/pdal/plang/gil.hpp @@ -45,6 +45,8 @@ class gil_scoped_release { }; + + } // plang } // pdal From 82a52ac84eaa5a078896c4e0a947257c0c4e412d Mon Sep 17 00:00:00 2001 From: Howard Butler Date: Wed, 3 Jan 2024 09:08:35 -0600 Subject: [PATCH 4/5] ci tweaking --- .github/workflows/build.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b1c640d..31dc1a6 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -38,8 +38,6 @@ jobs: use-mamba: true auto-update-conda: true environment-file: .github/environment.yml - extra-specs: | - python=${{ matrix.python-version }} - name: Install From 99df2b94d286f0d8361dca685de0d8de5a3e4e2a Mon Sep 17 00:00:00 2001 From: Howard Butler Date: Wed, 3 Jan 2024 11:37:48 -0600 Subject: [PATCH 5/5] guard readers.numpy PDAL functions with GIL acquisition --- pdal/io/NumpyReader.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pdal/io/NumpyReader.cpp b/pdal/io/NumpyReader.cpp index 8983340..dd4c1ea 100644 --- a/pdal/io/NumpyReader.cpp +++ b/pdal/io/NumpyReader.cpp @@ -198,6 +198,7 @@ PyArrayObject* load_npy_script(std::string const& source, void NumpyReader::initialize() { plang::Environment::get(); + plang::gil_scoped_acquire acquire; m_numPoints = 0; m_chunkCount = 0; m_ndims = 0; @@ -424,6 +425,7 @@ void NumpyReader::addDimensions(PointLayoutPtr layout) { using namespace Dimension; + plang::gil_scoped_acquire acquire; wakeUpNumpyArray(); createFields(layout); @@ -485,6 +487,7 @@ void NumpyReader::addDimensions(PointLayoutPtr layout) void NumpyReader::ready(PointTableRef table) { + plang::gil_scoped_acquire acquire; plang::Environment::get()->set_stdout(log()->getLogStream()); // Set our iterators @@ -528,6 +531,8 @@ bool NumpyReader::nextPoint() // just advance by the stride. if (--m_chunkCount == 0) { + // Go grab the gil before we touch Python stuff again + plang::gil_scoped_acquire acquire; // If we can't fetch the next ite if (!m_iternext(m_iter)) return false; @@ -653,6 +658,7 @@ bool NumpyReader::processOne(PointRef& point) point_count_t NumpyReader::read(PointViewPtr view, point_count_t numToRead) { + plang::gil_scoped_acquire acquire; PointId idx = view->size(); point_count_t numRead(0); @@ -670,8 +676,8 @@ point_count_t NumpyReader::read(PointViewPtr view, point_count_t numToRead) void NumpyReader::done(PointTableRef) { + plang::gil_scoped_acquire acquire; // Dereference everything we're using - if (m_iter) NpyIter_Deallocate(m_iter);