-
Notifications
You must be signed in to change notification settings - Fork 394
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
9202 9222 reset delete state python #9304
Changes from 10 commits
da627b2
ec094b0
e90ab4d
4acbbcc
789682f
ea6da8b
dd3ad25
9fb1859
2719bf6
5295194
3a76afa
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 |
---|---|---|
|
@@ -161,8 +161,12 @@ endif() | |
if(LINK_WITH_PYTHON) | ||
# find_package(Python) has the problem that on github actions in particular it'll pick up the most recent python (eg 3.9) from the tool cache | ||
# even if you have used the setup-python action and set it to 3.8, so we make the exact version required | ||
if(PYTHON_VERSION) | ||
find_package(Python ${PYTHON_VERSION} EXACT COMPONENTS Interpreter Development REQUIRED) | ||
set(Python_REQUIRED_VERSION "" CACHE STRING "The specific Python version you want to link to (Optional)") | ||
set(Python_ROOT_DIR "" CACHE PATH "The path to the root installation of your python version, if you want to explicitly set it (Optional)") | ||
mark_as_advanced(Python_REQUIRED_VERSION) | ||
mark_as_advanced(Python_ROOT_DIR) | ||
if(Python_REQUIRED_VERSION) | ||
find_package(Python ${Python_REQUIRED_VERSION} EXACT COMPONENTS Interpreter Development REQUIRED) | ||
else() | ||
find_package(Python 3.6 COMPONENTS Interpreter Development REQUIRED) | ||
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. My only comment is that this should probably be 3.7 and not 3.6 now since Python 3.6 reached End of Life Dec 2021 (https://endoflife.date/python). But this doesn't have to change in this PR necessarily. 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 would be annoying for people on Ubuntu 18 04 (or Gort, until yesterday) as the system python is 3.6. I personally dont care, I always use pyenv and much more recent pythons 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. @mbadams5 I do hear you there. Once Ubuntu 22.04 is released next month I plan to reimage our CI machines up to 20.04. That would still be one full LTS behind, which seems pretty safe. In that case, I'd be fully in support of going to 3.7 here. Let's chat about that soon. |
||
endif() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -976,9 +976,22 @@ if(BUILD_TESTING) | |
add_executable(TestAPI_Runtime_C ${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntime.c) | ||
target_link_libraries(TestAPI_Runtime_C PRIVATE energyplusapi project_options project_fp_options) | ||
# target_compile_options(TestAPI_Runtime_C PRIVATE -Wall -Wextra -Wno-unused) | ||
|
||
add_test(NAME "API.TestRuntimeC" COMMAND TestAPI_Runtime_C -d "${TEST_DIR}" -w "${EPW_FILE}" -D "${IDF_FILE}") | ||
|
||
set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/runtimeresetpython") | ||
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. I added 4 tests:
Without the fix the C ones pass, the python ones are exhibiting the segfault (tested ubuntu 20.04) 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. I always love TDD. |
||
file(MAKE_DIRECTORY ${TEST_DIR}) | ||
add_executable(TestAPI_RuntimeResetState_C ${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntimeResetState.c) | ||
target_link_libraries(TestAPI_RuntimeResetState_C PRIVATE energyplusapi project_options project_fp_options) | ||
# target_compile_options(TestAPI_RuntimeResetPython_C PRIVATE -Wall -Wextra -Wno-unused) | ||
add_test(NAME "API.TestRuntimeResetStateC" COMMAND TestAPI_RuntimeResetState_C -d "${TEST_DIR}" -w "${EPW_FILE}" -D "${IDF_FILE}") | ||
|
||
set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/runtimedeletepython") | ||
file(MAKE_DIRECTORY ${TEST_DIR}) | ||
add_executable(TestAPI_RuntimeDeleteState_C ${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntimeDeleteState.c) | ||
target_link_libraries(TestAPI_RuntimeDeleteState_C PRIVATE energyplusapi project_options project_fp_options) | ||
# target_compile_options(TestAPI_RuntimeDeletePython_C PRIVATE -Wall -Wextra -Wno-unused) | ||
add_test(NAME "API.TestRuntimeDeleteStateC" COMMAND TestAPI_RuntimeDeleteState_C -d "${TEST_DIR}" -w "${EPW_FILE}" -D "${IDF_FILE}") | ||
|
||
set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/exchange") | ||
file(MAKE_DIRECTORY ${TEST_DIR}) | ||
add_executable(TestAPI_DataTransfer_C ${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestDataTransfer.c) | ||
|
@@ -1011,6 +1024,18 @@ if(BUILD_TESTING) | |
endif() | ||
set_tests_properties("API.TestRuntimePython" PROPERTIES ENVIRONMENT PYTHONPATH=${DIR_WITH_PY_ENERGYPLUS}) | ||
|
||
# Runtime tests need a folder to be created for output files | ||
set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/runtime_resetstate_python") | ||
file(MAKE_DIRECTORY ${TEST_DIR}) | ||
add_test(NAME "API.TestRuntimeResetStatePython" COMMAND "${Python_EXECUTABLE}" "${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntimeResetState.py" -d "${TEST_DIR}" -w "${EPW_FILE}" -D "${IDF_FILE}") | ||
set_tests_properties("API.TestRuntimeResetStatePython" PROPERTIES ENVIRONMENT PYTHONPATH=${DIR_WITH_PY_ENERGYPLUS}) | ||
|
||
# Runtime tests need a folder to be created for output files | ||
set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/runtime_deletestate_python") | ||
file(MAKE_DIRECTORY ${TEST_DIR}) | ||
add_test(NAME "API.TestRuntimeDeleteStatePython" COMMAND "${Python_EXECUTABLE}" "${PROJECT_SOURCE_DIR}/tst/EnergyPlus/api/TestRuntimeDeleteState.py" -d "${TEST_DIR}" -w "${EPW_FILE}" -D "${IDF_FILE}") | ||
set_tests_properties("API.TestRuntimeDeleteStatePython" PROPERTIES ENVIRONMENT PYTHONPATH=${DIR_WITH_PY_ENERGYPLUS}) | ||
|
||
# Data exchange tests also need an output folder | ||
set(TEST_DIR "${PROJECT_BINARY_DIR}/tst/api/exchange_python") | ||
file(MAKE_DIRECTORY ${TEST_DIR}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -402,7 +402,7 @@ void PluginManager::setupOutputVariables([[maybe_unused]] EnergyPlusData &state) | |
#endif | ||
} | ||
|
||
PluginManager::PluginManager(EnergyPlusData &state) | ||
PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(state.dataPluginManager->eplusRunningViaPythonAPI) | ||
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. Ctor initializes the boolean from the PluginManagerData, so we can access it in the Dtor 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. It's all making sense now. Nice. |
||
{ | ||
#if LINK_WITH_PYTHON | ||
// this frozen flag tells Python that the package and library have been frozen for embedding, so it shouldn't warn about missing prefixes | ||
|
@@ -432,7 +432,10 @@ PluginManager::PluginManager(EnergyPlusData &state) | |
// now that we have set the path, we can initialize python | ||
// from https://docs.python.org/3/c-api/init.html | ||
// If arg 0, it skips init registration of signal handlers, which might be useful when Python is embedded. | ||
Py_InitializeEx(0); | ||
bool alreadyInitialized = (Py_IsInitialized() != 0); | ||
if (!alreadyInitialized) { | ||
Py_InitializeEx(0); | ||
} | ||
|
||
// Take control of the global interpreter lock while we are here, make sure to release it... | ||
PyGILState_STATE gil = PyGILState_Ensure(); | ||
|
@@ -648,7 +651,14 @@ PluginManager::PluginManager(EnergyPlusData &state) | |
PluginManager::~PluginManager() | ||
{ | ||
#if LINK_WITH_PYTHON | ||
Py_FinalizeEx(); | ||
if (!this->eplusRunningViaPythonAPI) { | ||
bool alreadyInitialized = (Py_IsInitialized() != 0); | ||
if (alreadyInitialized) { | ||
if (Py_FinalizeEx() < 0) { | ||
exit(120); | ||
} | ||
} | ||
} | ||
#endif // LINK_WITH_PYTHON | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -209,6 +209,8 @@ namespace PluginManagement { | |
static int getLocationOfUserDefinedPlugin(EnergyPlusData &state, std::string const &_programName); | ||
static void runSingleUserDefinedPlugin(EnergyPlusData &state, int index); | ||
static bool anyUnexpectedPluginObjects(EnergyPlusData &state); | ||
|
||
bool eplusRunningViaPythonAPI = false; | ||
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. This is on the PluginManager class 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. Again, it seems this is so that the dtor has access to a copy of the flag. |
||
}; | ||
|
||
struct PluginTrendVariable | ||
|
@@ -243,6 +245,9 @@ struct PluginManagerData : BaseGlobalStruct | |
bool apiErrorFlag = false; | ||
std::vector<std::string> const objectsToFind = { | ||
"PythonPlugin:OutputVariable", "PythonPlugin:SearchPaths", "PythonPlugin:Instance", "PythonPlugin:Variables", "PythonPlugin:TrendVariable"}; | ||
|
||
bool eplusRunningViaPythonAPI = false; | ||
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. This is on PluginManagerData, only so I can forward it to to PluginManager when it is finally instantiated. When 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. Interesting. This isn't code that is commonly touched, so I'm not going to rigor over an extra variable, but I am a bit unsure why you can't just always use the one from state, and not have one on the plugin manager at all. I guess it's because the plugin manager dtor doesn't take a state argument...so there is no state to access, only local members. |
||
|
||
void clear_state() override | ||
{ | ||
callbacks.clear(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,13 @@ EnergyPlusState stateNew() | |
return reinterpret_cast<EnergyPlusState>(state); | ||
} | ||
|
||
EnergyPlusState stateNewPython() | ||
{ | ||
auto *state = new EnergyPlus::EnergyPlusData; | ||
state->dataPluginManager->eplusRunningViaPythonAPI = true; | ||
return reinterpret_cast<EnergyPlusState>(state); | ||
} | ||
|
||
Comment on lines
+62
to
+68
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. When running from Python, set the boolean to true. 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. Nice. |
||
void stateReset(EnergyPlusState state) | ||
{ | ||
auto *this_state = reinterpret_cast<EnergyPlus::EnergyPlusData *>(state); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,8 +72,8 @@ class StateManager: | |
|
||
def __init__(self, api: cdll): | ||
self.api = api | ||
self.api.stateNew.argtypes = [] | ||
self.api.stateNew.restype = c_void_p | ||
self.api.stateNewPython.argtypes = [] | ||
self.api.stateNewPython.restype = c_void_p | ||
self.api.stateReset.argtypes = [c_void_p] | ||
self.api.stateReset.restype = c_void_p | ||
self.api.stateDelete.argtypes = [c_void_p] | ||
|
@@ -85,7 +85,7 @@ def new_state(self) -> c_void_p: | |
|
||
:return: A pointer to a new state object in memory | ||
""" | ||
return self.api.stateNew() | ||
return self.api.stateNewPython() | ||
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. state_new in python calls stateNewPython... That's unfortunate to add another extern C method, but given the lack of support in C for default argument values... 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. Yeah, it's not a big deal to me at all. This is hidden within the API anyway, so all good. |
||
|
||
def reset_state(self, state: c_void_p) -> None: | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
// EnergyPlus, Copyright (c) 1996-2020, 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. | ||
|
||
#include <EnergyPlus/api/runtime.h> | ||
#include <EnergyPlus/api/state.h> | ||
#include <stdio.h> | ||
#include <string.h> | ||
|
||
#ifdef _WIN32 | ||
#define __PRETTY_FUNCTION__ __FUNCSIG__ | ||
#endif | ||
|
||
int numWarnings = 0; | ||
int oneTimeHalfway = 0; | ||
|
||
void BeginNewEnvironmentHandler1(EnergyPlusState state) | ||
{ | ||
printf("CALLBACK: %s\n", __PRETTY_FUNCTION__); | ||
} | ||
|
||
void BeginNewEnvironmentHandler2(EnergyPlusState state) | ||
{ | ||
printf("CALLBACK: %s\n", __PRETTY_FUNCTION__); | ||
} | ||
|
||
int main(int argc, const char *argv[]) | ||
{ | ||
|
||
EnergyPlusState state = stateNew(); | ||
callbackBeginNewEnvironment(state, BeginNewEnvironmentHandler1); | ||
energyplus(state, argc, argv); | ||
|
||
// Delete, recreate and run again | ||
stateDelete(state); | ||
state = stateNew(); | ||
callbackBeginNewEnvironment(state, BeginNewEnvironmentHandler2); | ||
printf("Running EnergyPlus with Console Output Muted...\n"); | ||
setConsoleOutputState(state, 0); | ||
energyplus(state, argc, argv); | ||
printf("...and it is done.\n"); | ||
|
||
return 0; | ||
} |
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.
Do we still need to expose
PYTHON_INCLUDE_DIR
,PYTHON_LIBRARY
, andPYTHON_LIBRARY_DEBUG
or are those no longer needed?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.
@mitchute Those are artifacts from when the deprecated
find_package(PythonInterp)
andfind_package(PythonLibs)
were used. You don't see them on a clean build.cf https://cmake.org/cmake/help/latest/module/FindPythonLibs.html