From e361b479df666b862784ac80155c25696bcc9532 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 22 Dec 2023 10:35:11 +0100 Subject: [PATCH 1/3] Use PyConfig * Isolate https://github.com/NREL/EnergyPlus/pull/10340 into it's own PR * remove blocks on python version (use PyConfig for 3.8 too), * Put back type_traits * Improve error messages via a fmt formatter for PyStatus * Replace C style casts with static_casts --- src/EnergyPlus/PluginManager.cc | 185 +++++++++++++++++++++++--------- src/EnergyPlus/PluginManager.hh | 2 + 2 files changed, 134 insertions(+), 53 deletions(-) diff --git a/src/EnergyPlus/PluginManager.cc b/src/EnergyPlus/PluginManager.cc index dc55cf482d2..19eed55b70c 100644 --- a/src/EnergyPlus/PluginManager.cc +++ b/src/EnergyPlus/PluginManager.cc @@ -57,17 +57,49 @@ #include #include +#if LINK_WITH_PYTHON +#include +namespace fmt { +template <> struct formatter +{ + // parse is inherited from formatter. + constexpr auto parse(format_parse_context &ctx) -> format_parse_context::iterator + { + return ctx.begin(); + } + + auto format(const PyStatus &status, format_context &ctx) const -> format_context::iterator + { + if (!PyStatus_Exception(status)) { + return ctx.out(); + } + if (PyStatus_IsExit(status)) { + return fmt::format_to(ctx.out(), "Exited with code {}", status.exitcode); + } + if (PyStatus_IsError(status)) { + auto it = ctx.out(); + it = fmt::format_to(it, "Fatal Python error: "); + if (status.func) { + it = fmt::format_to(it, "{}: ", status.func); + } + it = fmt::format_to(it, "{}", status.err_msg); + return it; + } + return ctx.out(); + } +}; +} // namespace fmt +#endif + namespace EnergyPlus::PluginManagement { PluginTrendVariable::PluginTrendVariable(EnergyPlusData &state, std::string _name, int _numValues, int _indexOfPluginVariable) : name(std::move(_name)), numValues(_numValues), indexOfPluginVariable(_indexOfPluginVariable) { - // initialize the deque so it can be queried immediately, even with just zeroes + // initialize the deque, so it can be queried immediately, even with just zeroes for (int i = 1; i <= this->numValues; i++) { this->values.push_back(0); - } - for (int loop = 1; loop <= _numValues; ++loop) { - this->times.push_back(-loop * state.dataGlobal->TimeStepZone); + this->times.push_back(-i * state.dataGlobal->TimeStepZone); } } @@ -379,6 +411,82 @@ void PluginManager::setupOutputVariables([[maybe_unused]] EnergyPlusData &state) #endif } +#if LINK_WITH_PYTHON +void PluginManager::initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages) +{ + PyStatus status; + + // first pre-config Python so that it can speak UTF-8 + PyPreConfig preConfig; + PyPreConfig_InitPythonConfig(&preConfig); + preConfig.utf8_mode = 1; + status = Py_PreInitialize(&preConfig); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not pre-initialize Python to speak UTF-8... {}", status)); + } + + PyConfig config; + PyConfig_InitIsolatedConfig(&config); + config.isolated = 1; + + status = PyConfig_SetBytesString(&config, &config.program_name, PluginManagement::programName); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not initialize program_name on PyConfig... {}", status)); + } + + status = PyConfig_Read(&config); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not read back the PyConfig... {}", status)); + } + + if constexpr (std::is_same_v) { + // PyConfig_SetString copies the wide character string str into *config_str. + std::wstring const ws = pathToPythonPackages.generic_wstring(); + const wchar_t *wcharPath = ws.c_str(); + + status = PyConfig_SetString(&config, &config.home, wcharPath); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not set home to {} on PyConfig... {}", pathToPythonPackages.generic_string(), status)); + } + status = PyConfig_SetString(&config, &config.base_prefix, wcharPath); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not set base_prefix to {} on PyConfig... {}", pathToPythonPackages.generic_string(), status)); + } + config.module_search_paths_set = 1; + status = PyWideStringList_Append(&config.module_search_paths, wcharPath); + if (PyStatus_Exception(status)) { + ShowFatalError(state, + fmt::format("Could not add {} to module_search_paths on PyConfig... {}", pathToPythonPackages.generic_string(), status)); + } + + } else { + // PyConfig_SetBytesString takes a `const char * str` and decodes str using Py_DecodeLocale() and set the result into *config_str + // But we want to avoid doing it three times, so we PyDecodeLocale manually + // Py_DecodeLocale can be called because Python has been PreInitialized. + wchar_t *wcharPath = Py_DecodeLocale(pathToPythonPackages.generic_string().c_str(), nullptr); // This allocates! + + status = PyConfig_SetString(&config, &config.home, wcharPath); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not set home to {} on PyConfig... {}", pathToPythonPackages.generic_string(), status)); + } + status = PyConfig_SetString(&config, &config.base_prefix, wcharPath); + if (PyStatus_Exception(status)) { + ShowFatalError(state, fmt::format("Could not set base_prefix to {} on PyConfig... {}", pathToPythonPackages.generic_string(), status)); + } + config.module_search_paths_set = 1; + status = PyWideStringList_Append(&config.module_search_paths, wcharPath); + if (PyStatus_Exception(status)) { + ShowFatalError(state, + fmt::format("Could not add {} to module_search_paths on PyConfig... {}", pathToPythonPackages.generic_string(), status)); + } + + PyMem_RawFree(wcharPath); + } + + Py_InitializeFromConfig(&config); +} +#endif // LINK_WITH_PYTHON + PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(state.dataPluginManager->eplusRunningViaPythonAPI) { // Now read all the actual plugins and interpret them @@ -390,9 +498,6 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s } #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 - Py_FrozenFlag = 1; - // we'll need the program directory for a few things so get it once here at the top and sanitize it fs::path programDir; if (state.dataGlobal->installRootOverride) { @@ -400,34 +505,9 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s } else { programDir = FileSystem::getParentDirectoryPath(FileSystem::getAbsolutePath(FileSystem::getProgramPath())); } - - // I think we need to set the python path before initializing the library - // make this relative to the binary fs::path const pathToPythonPackages = programDir / "python_standard_lib"; - if constexpr (std::is_same_v) { - std::wstring const ws = pathToPythonPackages.generic_wstring(); - Py_SetPath(ws.c_str()); - Py_SetPythonHome(ws.c_str()); - } else { - // TODO: Py_DecodeLocale shouldn't be called before Python is PreInitialized. Also, this should be replaced by PyConfig - wchar_t *a = Py_DecodeLocale(pathToPythonPackages.generic_string().c_str(), nullptr); // This allocates! - Py_SetPath(a); - Py_SetPythonHome(a); - PyMem_RawFree(a); - } - // must be called before Py_Initialize - // tells the interpreter the value of argv[0] to the main() function - // used by some functions to find run-time libraries relative to the interpreter executable - Py_SetProgramName((wchar_t *)programName); - - // 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. - bool alreadyInitialized = (Py_IsInitialized() != 0); - if (!alreadyInitialized) { - Py_InitializeEx(0); - } + PluginManager::initPython(state, pathToPythonPackages); // Take control of the global interpreter lock while we are here, make sure to release it... PyGILState_STATE gil = PyGILState_Ensure(); @@ -690,7 +770,7 @@ void PluginInstance::reportPythonError([[maybe_unused]] EnergyPlusData &state) return; } - unsigned long numVals = PyList_Size(pyth_val); + Py_ssize_t numVals = PyList_Size(pyth_val); if (numVals == 0) { EnergyPlus::ShowContinueError(state, "No traceback available"); return; @@ -700,7 +780,7 @@ void PluginInstance::reportPythonError([[maybe_unused]] EnergyPlusData &state) EnergyPlus::ShowContinueError(state, "```"); - for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { + for (Py_ssize_t itemNum = 0; itemNum < numVals; itemNum++) { PyObject *item = PyList_GetItem(pyth_val, itemNum); if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning std::string traceback_line = PyUnicode_AsUTF8(item); @@ -769,16 +849,14 @@ void PluginInstance::setup([[maybe_unused]] EnergyPlusData &state) // import from database or something ShowFatalError(state, "Could not get full path"); } else { - PyObject *pStrObj = PyUnicode_AsUTF8String(pFullPath); - char *zStr = PyBytes_AsString(pStrObj); - std::string s(zStr); - Py_DECREF(pStrObj); // PyUnicode_AsUTF8String returns a new reference, decrement it - ShowMessage(state, format("PythonPlugin: Class {} imported from: {}", className, s)); + const char *zStr = PyUnicode_AsUTF8(pFullPath); + std::string sHere(zStr); + ShowMessage(state, format("PythonPlugin: Class {} imported from: {}", className, sHere)); } PyObject *pClass = PyDict_GetItemString(pModuleDict, className.c_str()); // Py_DECREF(pModuleDict); // PyModule_GetDict returns a borrowed reference, DO NOT decrement if (!pClass) { - EnergyPlus::ShowSevereError(state, format("Failed to get class type \"{}\" from module \"{}\"", className, modulePath.generic_string())); + EnergyPlus::ShowSevereError(state, format(R"(Failed to get class type "{}" from module "{}")", className, modulePath.generic_string())); if (PyErr_Occurred()) { PluginInstance::reportPythonError(state); } else { @@ -816,7 +894,7 @@ void PluginInstance::setup([[maybe_unused]] EnergyPlusData &state) PyObject *detectFunction = PyObject_GetAttrString(this->pClassInstance, detectOverriddenFunctionName.c_str()); if (!detectFunction || !PyCallable_Check(detectFunction)) { EnergyPlus::ShowSevereError(state, - format("Could not find or call function \"{}\" on class \"{}.{}\"", + format(R"(Could not find or call function "{}" on class "{}.{}")", detectOverriddenFunctionName, this->modulePath.generic_string(), this->className)); @@ -841,14 +919,14 @@ void PluginInstance::setup([[maybe_unused]] EnergyPlusData &state) if (!PyList_Check(pFunctionResponse)) { // NOLINT(hicpp-signed-bitwise) EnergyPlus::ShowFatalError(state, format("Invalid return from _detect_overridden() on class \"{}\", this is weird", this->stringIdentifier)); } - unsigned long numVals = PyList_Size(pFunctionResponse); + Py_ssize_t numVals = PyList_Size(pFunctionResponse); // at this point we know which base class methods are being overridden by the derived class // we can loop over them and based on the name check the appropriate flag and assign the function pointer if (numVals == 0) { EnergyPlus::ShowFatalError( state, format("Python plugin \"{}\" did not override any base class methods; must override at least one", this->stringIdentifier)); } - for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { + for (Py_ssize_t itemNum = 0; itemNum < numVals; itemNum++) { PyObject *item = PyList_GetItem(pFunctionResponse, itemNum); if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning std::string functionName = PyUnicode_AsUTF8(item); @@ -1067,7 +1145,7 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF format("Program terminates after call to {}() on {} failed!", functionNameAsString, this->stringIdentifier)); } if (PyLong_Check(pFunctionResponse)) { // NOLINT(hicpp-signed-bitwise) - int exitCode = PyLong_AsLong(pFunctionResponse); + long exitCode = PyLong_AsLong(pFunctionResponse); if (exitCode == 0) { // success } else if (exitCode == 1) { @@ -1102,9 +1180,9 @@ bool PluginInstance::run([[maybe_unused]] EnergyPlusData &state, [[maybe_unused] std::vector PluginManager::currentPythonPath() { PyObject *sysPath = PySys_GetObject("path"); // Borrowed reference - size_t const n = PyList_Size(sysPath); // Py_ssize_t + Py_ssize_t const n = PyList_Size(sysPath); // Py_ssize_t std::vector pathLibs(n); - for (size_t i = 0; i < n; ++i) { + for (Py_ssize_t i = 0; i < n; ++i) { PyObject *element = PyList_GetItem(sysPath, i); // Borrowed reference pathLibs[i] = std::string{PyUnicode_AsUTF8(element)}; } @@ -1180,16 +1258,17 @@ void PluginManager::addGlobalVariable([[maybe_unused]] EnergyPlusData &state, [[ int PluginManager::getGlobalVariableHandle(EnergyPlusData &state, const std::string &name, bool const suppress_warning) { // note zero is a valid handle std::string const varNameUC = EnergyPlus::Util::makeUPPER(name); - auto const it = std::find(state.dataPluginManager->globalVariableNames.begin(), state.dataPluginManager->globalVariableNames.end(), varNameUC); - if (it != state.dataPluginManager->globalVariableNames.end()) { - return std::distance(state.dataPluginManager->globalVariableNames.begin(), it); + auto const &gVarNames = state.dataPluginManager->globalVariableNames; + auto const it = std::find(gVarNames.begin(), gVarNames.end(), varNameUC); + if (it != gVarNames.end()) { + return static_cast(std::distance(gVarNames.begin(), it)); } else { if (suppress_warning) { return -1; } else { EnergyPlus::ShowSevereError(state, "Tried to retrieve handle for a nonexistent plugin global variable"); EnergyPlus::ShowContinueError(state, format("Name looked up: \"{}\", available names: ", varNameUC)); - for (auto const &gvName : state.dataPluginManager->globalVariableNames) { + for (auto const &gvName : gVarNames) { EnergyPlus::ShowContinueError(state, format(" \"{}\"", gvName)); } EnergyPlus::ShowFatalError(state, "Plugin global variable problem causes program termination"); @@ -1213,7 +1292,7 @@ int PluginManager::getTrendVariableHandle(EnergyPlusData &state, const std::stri for (size_t i = 0; i < state.dataPluginManager->trends.size(); i++) { auto &thisTrend = state.dataPluginManager->trends[i]; if (thisTrend.name == varNameUC) { - return i; + return static_cast(i); } } return -1; @@ -1407,7 +1486,7 @@ int PluginManager::getLocationOfUserDefinedPlugin(EnergyPlusData &state, std::st for (size_t handle = 0; handle < state.dataPluginManager->plugins.size(); handle++) { auto const &thisPlugin = state.dataPluginManager->plugins[handle]; if (Util::makeUPPER(thisPlugin.emsAlias) == Util::makeUPPER(_programName)) { - return handle; + return static_cast(handle); } } return -1; diff --git a/src/EnergyPlus/PluginManager.hh b/src/EnergyPlus/PluginManager.hh index 34cf2084174..bafb7550bf5 100644 --- a/src/EnergyPlus/PluginManager.hh +++ b/src/EnergyPlus/PluginManager.hh @@ -183,6 +183,8 @@ namespace PluginManagement { explicit PluginManager(EnergyPlusData &state); ~PluginManager(); + static void initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages); + static int numActiveCallbacks(EnergyPlusData &state); static void addToPythonPath(EnergyPlusData &state, const fs::path &includePath, bool userDefinedPath); static void setupOutputVariables(EnergyPlusData &state); From d2125a3171f5e4c8677052a95b8b0e8aef3ee1f6 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 22 Dec 2023 10:38:15 +0100 Subject: [PATCH 2/3] Request at least python 3.8 when LINK_WITH_PYTHON is enabled --- CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ceca25eec6e..4f8672e0ade 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -174,6 +174,7 @@ endif() # we are making *a Python 3.6 Interpreter* a required dependency, so find it here # If LINK_WITH_PYTHON, also request the Development (libs) at the same time, to ensure consistent version between interpreter and Development +# and ask for at least 3.8 (for the PyConfig stuff). 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 @@ -184,7 +185,7 @@ if(LINK_WITH_PYTHON) 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) + find_package(Python 3.8 COMPONENTS Interpreter Development REQUIRED) endif() else() find_package(Python 3.6 COMPONENTS Interpreter REQUIRED) From 561d0fb4e901f685c13b81d51f8b953e31b7f05b Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 22 Dec 2023 15:52:46 +0100 Subject: [PATCH 3/3] Make initPython non member (no point - and avoids having to define it even if not LINK_WITH_PYTHON) --- src/EnergyPlus/PluginManager.cc | 4 ++-- src/EnergyPlus/PluginManager.hh | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/EnergyPlus/PluginManager.cc b/src/EnergyPlus/PluginManager.cc index 19eed55b70c..5bda370ae44 100644 --- a/src/EnergyPlus/PluginManager.cc +++ b/src/EnergyPlus/PluginManager.cc @@ -412,7 +412,7 @@ void PluginManager::setupOutputVariables([[maybe_unused]] EnergyPlusData &state) } #if LINK_WITH_PYTHON -void PluginManager::initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages) +void initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages) { PyStatus status; @@ -507,7 +507,7 @@ PluginManager::PluginManager(EnergyPlusData &state) : eplusRunningViaPythonAPI(s } fs::path const pathToPythonPackages = programDir / "python_standard_lib"; - PluginManager::initPython(state, pathToPythonPackages); + initPython(state, pathToPythonPackages); // Take control of the global interpreter lock while we are here, make sure to release it... PyGILState_STATE gil = PyGILState_Ensure(); diff --git a/src/EnergyPlus/PluginManager.hh b/src/EnergyPlus/PluginManager.hh index bafb7550bf5..34cf2084174 100644 --- a/src/EnergyPlus/PluginManager.hh +++ b/src/EnergyPlus/PluginManager.hh @@ -183,8 +183,6 @@ namespace PluginManagement { explicit PluginManager(EnergyPlusData &state); ~PluginManager(); - static void initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages); - static int numActiveCallbacks(EnergyPlusData &state); static void addToPythonPath(EnergyPlusData &state, const fs::path &includePath, bool userDefinedPath); static void setupOutputVariables(EnergyPlusData &state);