From 674ed9dde345db9426b647de07490a0c403bf87e Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 3 Dec 2024 12:52:02 +0100 Subject: [PATCH 1/5] Add a ctest for #10842 --- src/EnergyPlus/CMakeLists.txt | 11 ++ .../api/TestRuntimeReleasesTheGIL.py | 66 ++++++++++++ .../TestRuntimeReleasesTheGIL/mcve_gil.idf | 102 ++++++++++++++++++ .../api/TestRuntimeReleasesTheGIL/mcve_gil.py | 1 + 4 files changed, 180 insertions(+) create mode 100644 tst/EnergyPlus/api/TestRuntimeReleasesTheGIL.py create mode 100644 tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.idf create mode 100644 tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py diff --git a/src/EnergyPlus/CMakeLists.txt b/src/EnergyPlus/CMakeLists.txt index 2570f70de51..174e2f0d693 100644 --- a/src/EnergyPlus/CMakeLists.txt +++ b/src/EnergyPlus/CMakeLists.txt @@ -1147,6 +1147,17 @@ if(BUILD_TESTING) COMMAND energyplus -D -d "${CLI_TEST_DIR}/PythonPlugin.FromOutside/out-${NON_ASCII_DIRNAME}" ${NON_ASCII_DIRNAME}/${TEST_CASE}.idf WORKING_DIRECTORY "${CLI_TEST_DIR}" ) + + set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/TestRuntimeReleasesTheGIL") + file(MAKE_DIRECTORY ${TEST_DIR}) + add_test(NAME "API.Runtime.PythonPlugin.TestRuntimeReleasesTheGIL" + COMMAND "${Python_EXECUTABLE}" "${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL.py" -d "${TEST_DIR}" -w "${EPW_FILE}" -D "${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.idf" + ) + set_tests_properties("API.Runtime.PythonPlugin.TestRuntimeReleasesTheGIL" + PROPERTIES + ENVIRONMENT PYTHONPATH=${DIR_WITH_PY_ENERGYPLUS} + TIMEOUT 10 # This used to timeout! and we expect it NOT to + ) endif() endif() diff --git a/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL.py b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL.py new file mode 100644 index 00000000000..95b854fd588 --- /dev/null +++ b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL.py @@ -0,0 +1,66 @@ +# EnergyPlus, Copyright (c) 1996-2024, The Board of Trustees of the University +# of Illinois, The Regents of the University of California, through Lawrence +# Berkeley National Laboratory (subject to receipt of any required approvals +# from the U.S. Dept. of Energy), Oak Ridge National Laboratory, managed by UT- +# Battelle, Alliance for Sustainable Energy, LLC, and other contributors. All +# rights reserved. +# +# NOTICE: This Software was developed under funding from the U.S. Department of +# Energy and the U.S. Government consequently retains certain rights. As such, +# the U.S. Government has been granted for itself and others acting on its +# behalf a paid-up, nonexclusive, irrevocable, worldwide license in the +# Software to reproduce, distribute copies to the public, prepare derivative +# works, and perform publicly and display publicly, and to permit others to do +# so. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# +# (1) Redistributions of source code must retain the above copyright notice, +# this list of conditions and the following disclaimer. +# +# (2) Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# +# (3) Neither the name of the University of California, Lawrence Berkeley +# National Laboratory, the University of Illinois, U.S. Dept. of Energy nor +# the names of its contributors may be used to endorse or promote products +# derived from this software without specific prior written permission. +# +# (4) Use of EnergyPlus(TM) Name. If Licensee (i) distributes the software in +# stand-alone form without changes from the version obtained under this +# License, or (ii) Licensee makes a reference solely to the software +# portion of its product, Licensee must refer to the software as +# "EnergyPlus version X" software, where "X" is the version number Licensee +# obtained under this License and may not use a different name for the +# software. Except as specifically required in this Section (4), Licensee +# shall not use in a company name, a product name, in advertising, +# publicity, or other promotional activities any name, trade name, +# trademark, logo, or other designation of "EnergyPlus", "E+", "e+" or +# confusingly similar designation, without the U.S. Department of Energy's +# prior written consent. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE +# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR +# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF +# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN +# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) +# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE +# POSSIBILITY OF SUCH DAMAGE. + +import sys + +from pyenergyplus.api import EnergyPlusAPI + +api = EnergyPlusAPI() +state = api.state_manager.new_state() +exit_code = api.runtime.run_energyplus(state, sys.argv[1:]) + +if exit_code == 0: + print("Expected EnergyPlus to return an error to the broken python plugin. Task Succeeded Unsuccessfully!") + sys.exit(1) diff --git a/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.idf b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.idf new file mode 100644 index 00000000000..e9fcffaefcd --- /dev/null +++ b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.idf @@ -0,0 +1,102 @@ +! The PythonPlugin referenced here has a broken import +! This is a test for #10842 + + Timestep,6; + + SimulationControl, + No, !- Do Zone Sizing Calculation + No, !- Do System Sizing Calculation + No, !- Do Plant Sizing Calculation + Yes, !- Run Simulation for Sizing Periods + No, !- Run Simulation for Weather File Run Periods + No, !- Do HVAC Sizing Simulation for Sizing Periods + 1; !- Maximum Number of HVAC Sizing Simulation Passes + + GlobalGeometryRules, + UpperLeftCorner, !- Starting Vertex Position + Counterclockwise, !- Vertex Entry Direction + Relative, !- Coordinate System + Relative; !- Daylighting Reference Point Coordinate System + + Building, + Building, !- Name + 0.0000, !- North Axis {deg} + City, !- Terrain + 0.0400, !- Loads Convergence Tolerance Value {W} + 0.2000, !- Temperature Convergence Tolerance Value {deltaC} + FullInteriorAndExterior, !- Solar Distribution + 25, !- Maximum Number of Warmup Days + 6; !- Minimum Number of Warmup Days + + Site:Location, + CHICAGO_IL_USA TMY2-94846, !- Name + 41.78000, !- Latitude {deg} + -87.75000, !- Longitude {deg} + -6.000000, !- Time Zone {hr} + 190.0000; !- Elevation {m} + +! CHICAGO_IL_USA Annual Heating 99.6%, MaxDB=-20.6°C + + SizingPeriod:DesignDay, + CHICAGO Ann Htg 99.6% Condns DB, !- Name + 1, !- Month + 21, !- Day of Month + WinterDesignDay, !- Day Type + -20.6, !- Maximum Dry-Bulb Temperature {C} + 0.0, !- Daily Dry-Bulb Temperature Range {deltaC} + , !- Dry-Bulb Temperature Range Modifier Type + , !- Dry-Bulb Temperature Range Modifier Day Schedule Name + Wetbulb, !- Humidity Condition Type + -20.6, !- Wetbulb or DewPoint at Maximum Dry-Bulb {C} + , !- Humidity Condition Day Schedule Name + , !- Humidity Ratio at Maximum Dry-Bulb {kgWater/kgDryAir} + , !- Enthalpy at Maximum Dry-Bulb {J/kg} + , !- Daily Wet-Bulb Temperature Range {deltaC} + 99063., !- Barometric Pressure {Pa} + 4.9, !- Wind Speed {m/s} + 270, !- Wind Direction {deg} + No, !- Rain Indicator + No, !- Snow Indicator + No, !- Daylight Saving Time Indicator + ASHRAEClearSky, !- Solar Model Indicator + , !- Beam Solar Day Schedule Name + , !- Diffuse Solar Day Schedule Name + , !- ASHRAE Clear Sky Optical Depth for Beam Irradiance (taub) {dimensionless} + , !- ASHRAE Clear Sky Optical Depth for Diffuse Irradiance (taud) {dimensionless} + 0.00; !- Sky Clearness + +! CHICAGO_IL_USA Annual Cooling (DB=>MWB) .4%, MaxDB=33.2°C MWB=23.8°C + + SizingPeriod:DesignDay, + CHICAGO Ann Clg .4% Condns DB=>MWB, !- Name + 7, !- Month + 21, !- Day of Month + SummerDesignDay, !- Day Type + 33.2, !- Maximum Dry-Bulb Temperature {C} + 10.7, !- Daily Dry-Bulb Temperature Range {deltaC} + , !- Dry-Bulb Temperature Range Modifier Type + , !- Dry-Bulb Temperature Range Modifier Day Schedule Name + Wetbulb, !- Humidity Condition Type + 23.8, !- Wetbulb or DewPoint at Maximum Dry-Bulb {C} + , !- Humidity Condition Day Schedule Name + , !- Humidity Ratio at Maximum Dry-Bulb {kgWater/kgDryAir} + , !- Enthalpy at Maximum Dry-Bulb {J/kg} + , !- Daily Wet-Bulb Temperature Range {deltaC} + 99063., !- Barometric Pressure {Pa} + 5.3, !- Wind Speed {m/s} + 230, !- Wind Direction {deg} + No, !- Rain Indicator + No, !- Snow Indicator + No, !- Daylight Saving Time Indicator + ASHRAEClearSky, !- Solar Model Indicator + , !- Beam Solar Day Schedule Name + , !- Diffuse Solar Day Schedule Name + , !- ASHRAE Clear Sky Optical Depth for Beam Irradiance (taub) {dimensionless} + , !- ASHRAE Clear Sky Optical Depth for Diffuse Irradiance (taud) {dimensionless} + 1.00; !- Sky Clearness + + PythonPlugin:Instance, + Heating Setpoint Override, !- Name + Yes, !- Run During Warmup Days + mcve_gil, !- Python Module Name + HeatingSetPoint; !- Plugin Class Name diff --git a/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py new file mode 100644 index 00000000000..13a5cfbdb25 --- /dev/null +++ b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py @@ -0,0 +1 @@ +from this_does_not_exist.hello_world import garbage From 0c73d2989ba393ab72d8ead77c2651f4ad3e941f Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 3 Dec 2024 13:00:45 +0100 Subject: [PATCH 2/5] Add that license text to that useless test py file so the license-check will not complain --- .../api/TestRuntimeReleasesTheGIL/mcve_gil.py | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py index 13a5cfbdb25..1d21a7dc08b 100644 --- a/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py +++ b/tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py @@ -1 +1,56 @@ +# EnergyPlus, Copyright (c) 1996-2024, The Board of Trustees of the University +# of Illinois, The Regents of the University of California, through Lawrence +# Berkeley National Laboratory (subject to receipt of any required approvals +# from the U.S. Dept. of Energy), Oak Ridge National Laboratory, managed by UT- +# Battelle, Alliance for Sustainable Energy, LLC, and other contributors. All +# rights reserved. +# +# NOTICE: This Software was developed under funding from the U.S. Department of +# Energy and the U.S. Government consequently retains certain rights. As such, +# the U.S. Government has been granted for itself and others acting on its +# behalf a paid-up, nonexclusive, irrevocable, worldwide license in the +# Software to reproduce, distribute copies to the public, prepare derivative +# works, and perform publicly and display publicly, and to permit others to do +# so. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# +# (1) Redistributions of source code must retain the above copyright notice, +# this list of conditions and the following disclaimer. +# +# (2) Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# +# (3) Neither the name of the University of California, Lawrence Berkeley +# National Laboratory, the University of Illinois, U.S. Dept. of Energy nor +# the names of its contributors may be used to endorse or promote products +# derived from this software without specific prior written permission. +# +# (4) Use of EnergyPlus(TM) Name. If Licensee (i) distributes the software in +# stand-alone form without changes from the version obtained under this +# License, or (ii) Licensee makes a reference solely to the software +# portion of its product, Licensee must refer to the software as +# "EnergyPlus version X" software, where "X" is the version number Licensee +# obtained under this License and may not use a different name for the +# software. Except as specifically required in this Section (4), Licensee +# shall not use in a company name, a product name, in advertising, +# publicity, or other promotional activities any name, trade name, +# trademark, logo, or other designation of "EnergyPlus", "E+", "e+" or +# confusingly similar designation, without the U.S. Department of Energy's +# prior written consent. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE +# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR +# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF +# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN +# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) +# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE +# POSSIBILITY OF SUCH DAMAGE. + from this_does_not_exist.hello_world import garbage From 552f5ddfa923d85d33dbef9835a411c3478f445e Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 3 Dec 2024 13:03:12 +0100 Subject: [PATCH 3/5] Fix #10842 - try catch around plugin.setup and make sure to release the Python GIL --- src/EnergyPlus/PluginManager.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/EnergyPlus/PluginManager.cc b/src/EnergyPlus/PluginManager.cc index 74b8addda9a..0b24d5a881b 100644 --- a/src/EnergyPlus/PluginManager.cc +++ b/src/EnergyPlus/PluginManager.cc @@ -615,7 +615,12 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s // IMPORTANT - CALL setup() HERE ONCE ALL INSTANCES ARE CONSTRUCTED TO AVOID DESTRUCTOR/MEMORY ISSUES DURING VECTOR RESIZING for (auto &plugin : state.dataPluginManager->plugins) { - plugin.setup(state); + try { + plugin.setup(state); + } catch (const FatalError &e) { + PyGILState_Release(gil); + throw e; + } } std::string const sGlobals = "PythonPlugin:Variables"; From 80b8a019be603233ee02155386008bd95daad7ea Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 3 Dec 2024 13:05:12 +0100 Subject: [PATCH 4/5] Fix #10842 - use an RAII struct to grab and release the GIL no matter what happens --- src/EnergyPlus/PluginManager.cc | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/EnergyPlus/PluginManager.cc b/src/EnergyPlus/PluginManager.cc index 0b24d5a881b..e5d72065375 100644 --- a/src/EnergyPlus/PluginManager.cc +++ b/src/EnergyPlus/PluginManager.cc @@ -484,8 +484,23 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s initPython(state, pathToPythonPackages); - // Take control of the global interpreter lock while we are here, make sure to release it... - PyGILState_STATE gil = PyGILState_Ensure(); + // GilGrabber is an RAII helper that will ensure we release the GIL (including if we end up throwing) + struct GilGrabber + { + GilGrabber() + { + gil = PyGILState_Ensure(); + } + ~GilGrabber() + { + PyGILState_Release(gil); + } + + PyGILState_STATE gil; + }; + + // Take control of the global interpreter lock + GilGrabber gil_grabber; // call this once to allow us to add to, and report, sys.path later as needed PyRun_SimpleString("import sys"); // allows us to report sys.path later @@ -615,12 +630,7 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s // IMPORTANT - CALL setup() HERE ONCE ALL INSTANCES ARE CONSTRUCTED TO AVOID DESTRUCTOR/MEMORY ISSUES DURING VECTOR RESIZING for (auto &plugin : state.dataPluginManager->plugins) { - try { - plugin.setup(state); - } catch (const FatalError &e) { - PyGILState_Release(gil); - throw e; - } + plugin.setup(state); } std::string const sGlobals = "PythonPlugin:Variables"; @@ -684,8 +694,8 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s } } - // Release the global interpreter lock - PyGILState_Release(gil); + // Release the global interpreter lock is done via RAII + // setting up output variables deferred until later in the simulation setup process #else // need to alert only if a plugin instance is found From 397c580b7bb434c0c46f815c065d5ad752741e40 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 3 Dec 2024 13:27:53 +0100 Subject: [PATCH 5/5] Use the GilGrabber in PluginInstance::run as well --- src/EnergyPlus/PluginManager.cc | 42 +++++++++++++++------------------ 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/src/EnergyPlus/PluginManager.cc b/src/EnergyPlus/PluginManager.cc index e5d72065375..daf07b3a53d 100644 --- a/src/EnergyPlus/PluginManager.cc +++ b/src/EnergyPlus/PluginManager.cc @@ -461,6 +461,22 @@ void initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages) // Py_Initialize(); Py_InitializeFromConfig(&config); } + +// GilGrabber is an RAII helper that will ensure we release the GIL (including if we end up throwing) +struct GilGrabber +{ + GilGrabber() + { + gil = PyGILState_Ensure(); + } + ~GilGrabber() + { + PyGILState_Release(gil); + } + + PyGILState_STATE gil; +}; + #endif // LINK_WITH_PYTHON PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(state.dataPluginManager->eplusRunningViaPythonAPI) @@ -484,22 +500,7 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s initPython(state, pathToPythonPackages); - // GilGrabber is an RAII helper that will ensure we release the GIL (including if we end up throwing) - struct GilGrabber - { - GilGrabber() - { - gil = PyGILState_Ensure(); - } - ~GilGrabber() - { - PyGILState_Release(gil); - } - - PyGILState_STATE gil; - }; - - // Take control of the global interpreter lock + // Take control of the global interpreter lock, which will be released via RAII GilGrabber gil_grabber; // call this once to allow us to add to, and report, sys.path later as needed @@ -1118,8 +1119,8 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF return false; } - // Get control of the global interpreter lock - PyGILState_STATE gil = PyGILState_Ensure(); + // Take control of the global interpreter lock, which will be released via RAII + GilGrabber gil_grabber; // then call the main function // static const PyObject oneArgObjFormat = Py_BuildValue)("O"); @@ -1134,7 +1135,6 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF } else { ShowContinueError(state, "This could happen for any number of reasons, check the plugin code."); } - PyGILState_Release(gil); ShowFatalError(state, format("Program terminates after call to {}() on {} failed!", functionNameAsString, this->stringIdentifier)); } if (PyLong_Check(pFunctionResponse)) { // NOLINT(hicpp-signed-bitwise) @@ -1142,12 +1142,10 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF if (exitCode == 0) { // success } else if (exitCode == 1) { - PyGILState_Release(gil); ShowFatalError(state, format("Python Plugin \"{}\" returned 1 to indicate EnergyPlus should abort", this->stringIdentifier)); } } else { std::string const functionNameAsString(functionName); // only convert to string if an error occurs - PyGILState_Release(gil); ShowFatalError( state, format("Invalid return from {}() on class \"{}, make sure it returns an integer exit code, either zero (success) or one (failure)", @@ -1156,10 +1154,8 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF } Py_DECREF(pFunctionResponse); // PyObject_CallFunction returns new reference, decrement if (state.dataPluginManager->apiErrorFlag) { - PyGILState_Release(gil); ShowFatalError(state, "API problems encountered while running plugin cause program termination."); } - PyGILState_Release(gil); return true; } #else