Skip to content
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

Fix #10842 - When using pyenergyplus to run (via run_energyplus), PythonPlugin initializations errors lead to hang #10844

Merged
merged 5 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/EnergyPlus/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Comment on lines +1151 to +1160
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Register the ctest. NOTE THE TIMEOUT 10 (seconds), we do expect it to timeout to demonstrate #10842

endif()
endif()

Expand Down
23 changes: 19 additions & 4 deletions src/EnergyPlus/PluginManager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the issue was that we were NOT releasing the Python Global Interpreter Lock after acquiring it. (I tested a C version of the same test, which was correctly throwing and exiting as expected, so I figured it was the GIL)

Initially in 552f5dd I put a try / catch around plugin.setup()

    for (auto &plugin : state.dataPluginManager->plugins) {
+        try {
            plugin.setup(state);
+        } catch (const FatalError &e) {
+            PyGILState_Release(gil);
+            throw e;
 +       }
    }

But I realized it was probably a better idea to just use RAII, so that there's no chance we forget to release the Python GIL


// 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
Expand Down Expand Up @@ -679,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

Comment on lines +698 to +699
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to release the GIL manually anymore in the case it works fine either

// setting up output variables deferred until later in the simulation setup process
#else
// need to alert only if a plugin instance is found
Expand Down
66 changes: 66 additions & 0 deletions tst/EnergyPlus/api/TestRuntimeReleasesTheGIL.py
Original file line number Diff line number Diff line change
@@ -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)
Comment on lines +60 to +66
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run that file idf file with python plugin from API.

In the current state, the exit_code is never returned anyways because it just hangs. But testing that it did return an error is what we eventually want

102 changes: 102 additions & 0 deletions tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.idf
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +98 to +102
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCVE file that has nothing but this plugin, the required objects (Building, GlobalGeometry Rules) and design days.

56 changes: 56 additions & 0 deletions tst/EnergyPlus/api/TestRuntimeReleasesTheGIL/mcve_gil.py
Original file line number Diff line number Diff line change
@@ -0,0 +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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken Python Plugin import

Loading