Allow Plugin Plugins in Runtime API Calls #8913

Myoldmopar · 2021-07-22T14:42:57Z

Pull request overview

Background:

EnergyPlus has Python Plugins, where EnergyPlus calls out to the Python library and executes a series of commands that interpret, instantiate and run user-defined Python code.
EnergyPlus has a C API layer that exposes some functionality such as fluid and refrigerant properties, as well as allowing data exchange to and from outside the simulation from C-like languages.
A Python API layer is built on top of the C API layer that allows access from the Python language. API calls can be made from a thread outside of the simulation, calling EnergyPlus as a library, but Python API calls are also made from Python Plugins that EnergyPlus instantiates.

Up until now, there has been a limitation: when calling EnergyPlus as a library, a client could not run a file that executed a Python Plugin. If you were already inside a thread running Python when you called into EnergyPlus, then setting up a new Python interpreter and executing it internally would cause a segmentation fault. This was the state until this branch. I knew there had to be a means to make this work, and did some deeper research.

For this PR, I am gaining control of Python's global interpreter lock while inside EnergyPlus. This essentially localizes the internal interpreter operations so that it doesn't muck around with any Python interpreter that may be calling EnergyPlus from the outside. I have tested this in a number of ways, and asked @mitchute to test it out in a more complex environment where he is using third party Python dependencies, and everything seems to work just fine. We can now allow library calls that execute input files that have Python Plugins.

This was a great concern of a prominent interface developer when looking at using EnergyPlus API. And also came up as we convert EP-Launch to make API calls instead of calling EnergyPlus as a standalone executable.

To do:

Update documentation
Add test mixing API and Plugin
~~Potentially remove eplusRunningViaAPI variable and any accompanying API layer stuff.~~
Change variable name for GIL

Myoldmopar · 2021-07-22T14:43:26Z

Regression diffs are purely because the branch was out of date. I'll fix any documentation needed and pull develop in and it will be clean again.

Myoldmopar

Code walkthrough - pretty minimal.

Myoldmopar · 2021-07-22T14:45:12Z

src/EnergyPlus/PluginManager.cc

@@ -427,6 +427,10 @@ PluginManager::PluginManager(EnergyPlusData &state)
    // If arg 0, it skips init registration of signal handlers, which might be useful when Python is embedded.
    Py_InitializeEx(0);

+    // Take control of the global interpreter lock while we are here, make sure to release it...
+    PyGILState_STATE s = PyGILState_Ensure();


Hold a reference to the global interpreter lock while we instantiate the plugins at the beginning of the program.

Just a nitpick, but I would caution to use a better, more explicit name, for the variable. like gil_state or something.
I could see s being used for a dumb string at some point.

Myoldmopar · 2021-07-22T14:45:28Z

src/EnergyPlus/PluginManager.cc

@@ -583,6 +587,8 @@ PluginManager::PluginManager(EnergyPlusData &state)
        }
    }

+    // Release the global interpreter lock
+    PyGILState_Release(s);


Release the GIL once we are done interpreting the plugin.

Myoldmopar · 2021-07-22T14:45:42Z

src/EnergyPlus/PluginManager.cc

@@ -1021,6 +1027,9 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF
        return false;
    }

+    // Get control of the global interpreter lock
+    PyGILState_STATE s = PyGILState_Ensure();


Hold a reference to the GIL while we execute the plugin.

Myoldmopar · 2021-07-22T14:46:04Z

src/EnergyPlus/PluginManager.cc

@@ -1034,6 +1043,7 @@ bool PluginInstance::run(EnergyPlusData &state, EMSManager::EMSCallFrom iCalledF
        } else {
            EnergyPlus::ShowContinueError(state, "This could happen for any number of reasons, check the plugin code.");
        }
+        PyGILState_Release(s);


Make sure to release the GIL on each code path out of this function. There are several fatal error paths, so this is on each of them.

Myoldmopar · 2021-07-22T14:46:43Z

src/EnergyPlus/SimulationManager.cc

-                ShowFatalError(state, "Invalid Python Plugin object encounter causes program termination");
-            }
-        }
+        state.dataPluginManager->pluginManager = std::make_unique<EnergyPlus::PluginManagement::PluginManager>(state);


Loosen the constraint here. May want to eliminate the eplusRunningViaAPI variable since I don't think it should matter anymore.

Scratch that, don't delete it...a later branch wants to use this.

jmarrec · 2021-07-26T14:49:10Z

@Myoldmopar Could you clarify something for me please?

I understand the intent: being able to launch via API an E+ sim which itself have PythonPlugin:Instance objects.

But, if from python I make a thread pool, and run like 8 distinct E+ simulations... Do they still run in parallel?

I think that would be the case if I used threading.Thread, but not if I use multiprocessing.Pool probably, since the latter spaws a distinct python interpreter and memory space. (I would have used multiprocessing.Pool for this anyways TBH)

jmarrec · 2021-07-26T15:09:59Z

src/EnergyPlus/PluginManager.cc

@@ -427,6 +427,10 @@ PluginManager::PluginManager(EnergyPlusData &state)
    // If arg 0, it skips init registration of signal handlers, which might be useful when Python is embedded.
    Py_InitializeEx(0);

+    // Take control of the global interpreter lock while we are here, make sure to release it...
+    PyGILState_STATE s = PyGILState_Ensure();


Just a nitpick, but I would caution to use a better, more explicit name, for the variable. like gil_state or something.
I could see s being used for a dumb string at some point.

Myoldmopar · 2021-07-26T15:22:45Z

@jmarrec great question! I tested it out and have some quick results.

Reference Case

I took 5ZoneAirCooled and ran it in a single instance and then 7 parallel threads:

1 file completed in 5.5 seconds
7 files completed in ~6.1 seconds
There is, as expected, a slight slow down because I am now writing 7 output files to disk at (essentially) the exact same time.

Python Thread Case

I took PythonPluginCustomSchduled and did the same run with threads:

1 file comleted in 11 seconds
7 files completed in ~27 seconds
Definitely took a hit as the GIL was likely being held, but still shows a major improvement over running them in series, which would be ~77 seconds. A huge majority of the time the code is actually running parallel, just getting stuck when Python is being interpreted. And most of all, the runs all completed successfully.

Python Multiprocessing Case

I then created a simple multiprocessing pool and ran the same Python Plugin file:

1 file took, as expected, 11 seconds
7 files completed in ~13 seconds
Putting each EnergyPlus run in its own process space allows each to have their own separate global interpreter lock, and run independently, no cross talk, and no waiting.

Summary

However, this isn't just a free win. One of the key aspects of running via API is that you can, within the same process, spin up multiple EnergyPlus threads and have them talk to each other. This is a very easy task with multithreading because you can share the same Python variables and have the threads talk back in the same process space while they are running. That's not so easy with multiprocessing (someone prove me wrong).

However Number 2: If someone is going to be spinning up multiple E+ threads to run in parallel and ask them to be talking to each other, are they really also going to be asking each one to go out and run Python Plugins as well? I honestly can't imagine that to be the case. If they are tying the simulations together, then I would suspect that they are just using Python callbacks do the the integration and not also creating Python plugin files. I could be wrong. But if they are, then this workflow will still work, they will just notice some slowdown.

The real purpose of this PR is to simply allow interfaces that want to run E+ from their Python-based GUI, including sometimes running files that have Python Plugins. I think the overlap of Python GUI, Plugin, and cosimulation is probably a very, very small corner case for now. And, once again, we allow it, there is just a performance penalty for that tight integration. Maybe soon Python will allow first-class sub-interpreters and we'll be able to do this without a penalty... 👍

…AndAPI

Myoldmopar · 2021-07-27T15:04:16Z

I am going to proceed on polishing up this branch, updating docs to remove any notes about how API and Plugins are incompatible, adding a test where we call a plugin file from an API test, and also changing the GIL variable name per @jmarrec request. If there are other comments, please speak up, otherwise this branch should be ready to go once I push later today.

Myoldmopar · 2021-08-09T18:05:03Z

Pulled develop in and tweaked documentation. I don't know that I'll add even more testing in this PR, but it will be in the release package tester for sure.

Myoldmopar · 2021-08-17T15:42:35Z

tst/EnergyPlus/api/TestRuntime.c

-    setConsoleOutputState(state2, 0);
-    energyplus(state2, argc, argv);
-    printf("...and it is done.");
+    EnergyPlusState state3 = stateNew(); // stateReset(state);


Something a little funny was happening in the reused state so this just uses individual instances for now to get this running. I'll take a look later.

Myoldmopar · 2021-08-17T15:43:00Z

This branch was clean the last time around and it is still up to date with develop, so I'm merging it in.

Myoldmopar added 2 commits July 16, 2021 07:17

Relax the api/plugin incompatibility

bab002f

Merge remote-tracking branch 'origin/develop' into AllowPluginAndAPI

4aed9f2

Myoldmopar added the NewFeature Includes code to add a new feature to EnergyPlus label Jul 22, 2021

Myoldmopar commented Jul 22, 2021

View reviewed changes

jmarrec approved these changes Jul 26, 2021

View reviewed changes

Myoldmopar added 2 commits July 27, 2021 09:47

Merge remote-tracking branch 'origin/develop' into AllowPluginAndAPI

387d01a

Merge remote-tracking branch 'origin/AddAPIWorkflow' into AllowPlugin…

1d5ed14

…AndAPI

Myoldmopar mentioned this pull request Jul 27, 2021

Update EPLaunch Workflow to use API #8918

Merged

20 tasks

Myoldmopar added this to the EnergyPlus 9.6 IOFreeze milestone Aug 6, 2021

Myoldmopar added 5 commits August 9, 2021 08:54

Merge remote-tracking branch 'origin/develop' into AllowPluginAndAPI

1361611

Update documentation regarding API/Plugin

5e107ed

Update GIL variable name per @jmarrec request

e8580ad

Dont force Penumbra testing on

6e0ab11

Merge remote-tracking branch 'origin/develop' into AllowPluginAndAPI

9d7d39b

Debugging runtime API test

e3ee433

Myoldmopar commented Aug 17, 2021

View reviewed changes

Myoldmopar merged commit b353942 into develop Aug 17, 2021

Myoldmopar deleted the AllowPluginAndAPI branch August 17, 2021 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Plugin Plugins in Runtime API Calls #8913

Allow Plugin Plugins in Runtime API Calls #8913

Myoldmopar commented Jul 22, 2021 •

edited

Loading

Myoldmopar commented Jul 22, 2021

Myoldmopar left a comment

Myoldmopar Jul 22, 2021

jmarrec Jul 26, 2021

Myoldmopar Jul 22, 2021

Myoldmopar Jul 22, 2021

Myoldmopar Jul 22, 2021

Myoldmopar Jul 22, 2021

Myoldmopar Jul 22, 2021

jmarrec commented Jul 26, 2021 •

edited

Loading

jmarrec Jul 26, 2021

Myoldmopar commented Jul 26, 2021

Myoldmopar commented Jul 27, 2021

Myoldmopar commented Aug 9, 2021

Myoldmopar Aug 17, 2021

Myoldmopar commented Aug 17, 2021

Allow Plugin Plugins in Runtime API Calls #8913

Allow Plugin Plugins in Runtime API Calls #8913

Conversation

Myoldmopar commented Jul 22, 2021 • edited Loading

Pull request overview

Myoldmopar commented Jul 22, 2021

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec commented Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Myoldmopar commented Jul 26, 2021

Reference Case

Python Thread Case

Python Multiprocessing Case

Summary

Myoldmopar commented Jul 27, 2021

Myoldmopar commented Aug 9, 2021

Choose a reason for hiding this comment

Myoldmopar commented Aug 17, 2021

Myoldmopar commented Jul 22, 2021 •

edited

Loading

jmarrec commented Jul 26, 2021 •

edited

Loading