Migrate to pyenergyplus #541

TShapinsky · 2025-01-06T21:34:37Z

Finished

Added Unit to the output of a point metadata query.
LOGLEVEL environment variable determines which logs are seen via stdout.
Removed Haystack dependency from modelica.
Remove BCVTB/ExternalInterface/mlep.
Add Framework for created stepped simulation jobs which have external processes which want to control the main thread.
Moved Job Exceptions into their own file.
Remove GOAWS, replaced by redis.
Upgraded dependencies (excluding influxdb package).
Created test suite for different failure modes of pyeneregyplus to make sure cases are handled.

In Progress

Endpoint was added to serve raw logs (not just error logs) for a run.
- Stream log handler created to push logs into redis.
- log_subprocess function added to job which will automatically run a subprocess, pushing its output to the logger.

Todo

Clean up log messages to properly use log levels.
Remove commented out code.
Remove alfalfa ruby gem.
Roll alfalfa_metadata measure into CreateRun job using openstudio module.
Better code documentation for methods in StepRunBase and StepRunProcess
Use redis stream handler for normal logs and error logs

TShapinsky · 2025-01-30T16:42:12Z

alfalfa_web/server/api-v2.js

@@ -164,6 +164,13 @@ router.get("/runs/:runId/time", async (req, res, next) => {
    .catch(next);
 });

+router.get("/runs/:runId/log", async (req, res, next) => {


This would allow job/run logs to be accessed from the web api.

TShapinsky · 2025-01-30T16:45:42Z

alfalfa_worker/Dockerfile

-RUN pip3.8 install virtualenv \
-  && pip3.8 install \
-  scipy \
-  symfit


The worker is no longer providing builtin python dependencies outside of what base alfalfa needs.

This is partially due to removing the refrig_case test which required symfit. However symfit doesn't support python 3.12 and the project in general seems semi-abandoned so I removed the test case and replaced it with a small office based test model.

So in other words, if someone has an EnergyPlus model with Python EMS that uses third party Python modules, they're out of luck with Alfalfa. Is that right? It seems reasonable, as Alfalfa cannot anticipate every module that someone might want to use.

Can we say that Alfalfa supports any module that comes bundled with EnergyPlus?

Not quite. https://github.com/NREL/alfalfa/wiki/How-to-Migrate-EnergyPlus-Python-Plugins you can provide a requirements.txt which results in a virtual environment being created for your model which has your requirements installed.

TShapinsky · 2025-01-30T16:50:40Z

alfalfa_worker/__main__.py


 if __name__ == '__main__':
+
+    basicConfig(level=os.environ.get("LOGLEVEL", "INFO"),


Output logs at LOGLEVEL and above to stdout.

TShapinsky · 2025-01-30T17:02:11Z

alfalfa_worker/jobs/openstudio/__init__.py

-from .create_run import CreateRun
-from .step_run import StepRun
+# from .create_run import CreateRun
+# from .step_run import StepRun


Removed to prevent source files from being automatically loaded.

The __init__.py is run when the module gets imported, and in the dispatcher unit tests this can cause it to crash as StepRun has dependencies (openstudio, pyenergyplus) which may not be present on a developer's computer and are not present in the CI runner.

This could be avoided by using a job other than CreateRun to confirm that the dispatcher is able to properly resolve job paths.

TShapinsky · 2025-01-30T17:04:52Z

alfalfa_worker/jobs/openstudio/create_run.py

@@ -33,18 +31,18 @@ def exec(self):
        # If there are requirements.txt files in the model create a python virtual environment and install packaged there
        requirements = self.run.glob("**/requirements.txt")
        if len(requirements) > 0:
-            check_output(["python3.8", "-m", "venv", "--system-site-packages", "--symlinks", str(self.dir / '.venv')])
+            self.log_subprocess(["python3.12", "-m", "venv", "--system-site-packages", "--symlinks", str(self.dir / '.venv')])


log_subprocess takes the output of the subprocess and logs it. This makes logs more useful as they include more of the output from the job.

not related to the logging, but perhaps it makes sense to not reference python by exact version. This seems like unnecessary repetition. Why not just link python or python3 to the version you want in the container setup?

TShapinsky · 2025-01-30T17:06:18Z

alfalfa_worker/jobs/openstudio/lib/variables.py

-    hour_index: int = 1
-    minute_index: int = 2
-    month_index: int = 3
-
    def __init__(self, run: Run) -> None:


Add better documentation to this class.

TShapinsky · 2025-01-30T17:09:53Z

alfalfa_worker/jobs/step_run_base.py

+
+
+@dataclass
+class Options:


Consolidated options for StepRunBase extending jobs into one place.

TODO: document each of these options better

Maybe that is an opportunity to consolidate the class hierarchy a little?

TShapinsky · 2025-01-30T17:13:35Z

alfalfa_worker/jobs/step_run_base.py

-        """Placeholder for updating the datetime in Mongo to current simulation time"""
+        self.logger.info("Advancing to start time")
+        while self.run.sim_time < self.options.start_datetime:
+            self.logger.info("Calling advance")


Make this a debug message, and increase the verbosity.

TShapinsky · 2025-01-30T17:19:50Z

alfalfa_worker/lib/job.py

 class Job(metaclass=JobMetaclass):
    """Job Base Class"""

+    @error_wrapper


This is the only place this is used. This can probably be moved into the function body.

TShapinsky · 2025-01-30T17:24:14Z

alfalfa_worker/lib/testcase.py

@@ -86,7 +86,7 @@ def __init__(self, **kwargs):
        self.u_store = copy.deepcopy(self.u)
        # Set default options
        self.options = self.fmu.simulate_options()
-        self.options['CVode_options']['rtol'] = 1e-6
+        # self.options['CVode_options']['rtol'] = 1e-6


CVode no longer included by default. FMUs must provide solver.

@TShapinsky @kbenne If we are changing expected format/requirements for FMU upload, we should update docs.

Indeed FMUs for "co-simulation" are now the standard for BOPTEST and I guess here too and indeed that means the solver is baked into the FMU.

I think this PR is key context.

ibpsa/project1-boptest#536

TShapinsky · 2025-01-30T17:24:45Z

docker-compose.dev.yml

-      - mc
-      - mongo
-      - redis
-      - worker


Removed redundant configuration.

TShapinsky · 2025-01-30T17:26:42Z

tests/api/test_point.py

@@ -72,6 +73,7 @@ def test_point_retrieval(base_url, run_id, alfalfa_client):
    for point in outputs + bidirectionals:
        response = requests.get(f"{base_url}/runs/{run_id}/points/{point['id']}")

+        response.raise_for_status()


this raises an exception if the return code isn't a 2XX.

TShapinsky · 2025-01-30T17:27:25Z

tests/api/test_point.py

@@ -179,7 +182,7 @@ def test_point_writes(base_url, run_id):
    request_body = {
        'value': "hello"
    }
-    response = requests.put(f"{base_url}/runs/{run_id}/points/{inputs[0]['id']}", json=request_body)
+    response = requests.put(f"{base_url}/runs/{run_id}/points/{bidirectionals[0]['id']}", json=request_body)


switched as default model no longer has any pure inputs. Only bidirectional points.

kbenne

Hi @TShapinsky and @anyaelena. I left a lot of comments for thought, but this seems like a good PR and I'm excited to see it merged.

Nice job @TShapinsky

kbenne · 2025-01-31T16:21:45Z

alfalfa_web/server/api.js

@@ -92,6 +92,11 @@ class AlfalfaAPI {
    return await getHashValue(this.redis, run.ref_id, "sim_time");
  };

+  getRunLog = async (run) => {
+    const log_lines = await this.redis.lRange(`run:${run.ref_id}:log`, -100, -1);


nice. I like the simple design of streaming the log into redis. is this new or is only the api endpoint new? I guess I'll find out as I go through this PR.

The only question that comes to mind is what might the load look like as the scale gets big? Redis is powerful, but I sense these logs could get verbose. Nevertheless, good stuff, and if there is a performance impact I'm sure it can be mitigated by filtering the logs or something.

Good question. LRANGE is O(S+N) where S is the start offset, and N is the number of elements. So it will currently grow linearly. However, if we are interested in getting only a few of the most recent lines we could switch from doing an RPUSH to LPUSH. That way we wouldn't need any offset to retrieve the log. This would make the writes and reads both constant complexity operations so there wouldn't be any issue with big logs, outside of storage space.

kbenne · 2025-01-31T16:24:24Z

alfalfa_web/server/api.js

@@ -197,7 +203,7 @@ class AlfalfaAPI {

    const { startDatetime, endDatetime, timescale, realtime, externalClock } = data;

-    const job = `alfalfa_worker.jobs.${sim_type === "MODELICA" ? "modelica" : "openstudio"}.StepRun`;
+    const job = `alfalfa_worker.jobs.${sim_type === "MODELICA" ? "modelica" : "openstudio"}.step_run.StepRun`;


.step_run.StepRun I'm sure there is a motivation here. Why did you add the seemingly redundant job name?

oh I see, now that I'm further along in the review. This corresponds to your module structure.

kbenne · 2025-01-31T16:27:41Z

alfalfa_worker/Dockerfile

-RUN pip3.8 install virtualenv \
-  && pip3.8 install \
-  scipy \
-  symfit


So in other words, if someone has an EnergyPlus model with Python EMS that uses third party Python modules, they're out of luck with Alfalfa. Is that right? It seems reasonable, as Alfalfa cannot anticipate every module that someone might want to use.

Can we say that Alfalfa supports any module that comes bundled with EnergyPlus?

kbenne · 2025-01-31T16:40:28Z

alfalfa_worker/jobs/modelica/create_run.py

@@ -34,99 +28,11 @@ def exec(self):
        """
        self.logger.info("add_fmu for {}".format(self.run.ref_id))

-        # Create the FMU tags (no longer external now that python2 is deprecated)
-        self.create_tags()


So the key change with all of these deleted lines of code, is that the FMU is supposed to come with the tags metadata baked into the FMU's resources directory. Is that correct? This seems to be aligned with the BOPTEST convention.

kbenne · 2025-01-31T16:49:20Z

alfalfa_worker/jobs/modelica/step_run.py

-        sim_year = self.start_datetime.year
-        self.sim_start_time = (self.start_datetime - datetime(sim_year, 1, 1)) / timedelta(seconds=1)
-        self.sim_end_time = (self.end_datetime - datetime(sim_year, 1, 1)) / timedelta(seconds=1)
+        sim_year = self.options.start_datetime.year


Just some refactoring to accommodate the options structure, correct?

kbenne · 2025-01-31T18:39:43Z

alfalfa_worker/jobs/openstudio/step_run.py

-        self.ep.outputs = outputs
-
-    def step(self):
+    def start_simulation_process(self):


and critically, this functions blocks while EnergyPlus is running so it is started in another Process. You might make a note about that.

kbenne · 2025-01-31T18:41:10Z

alfalfa_worker/jobs/step_run_base.py

+
+
+@dataclass
+class Options:


Maybe that is an opportunity to consolidate the class hierarchy a little?

kbenne · 2025-01-31T18:52:35Z

alfalfa_worker/lib/testcase.py

@@ -86,7 +86,7 @@ def __init__(self, **kwargs):
        self.u_store = copy.deepcopy(self.u)
        # Set default options
        self.options = self.fmu.simulate_options()
-        self.options['CVode_options']['rtol'] = 1e-6
+        # self.options['CVode_options']['rtol'] = 1e-6


Indeed FMUs for "co-simulation" are now the standard for BOPTEST and I guess here too and indeed that means the solver is baked into the FMU.

I think this PR is key context.

ibpsa/project1-boptest#536

kbenne · 2025-01-31T18:54:52Z

alfalfa_worker/lib/run_manager.py

@@ -150,74 +148,6 @@ def checkout_run(self, run_id: str) -> Run:
        run.dir = run_path
        return run

-    def add_site_to_mongo(self, haystack_json: dict, run: Run):


Is there any data left being pushed into a Mongo document? I don't see it. Can we remove Mongo entirely?

kbenne · 2025-01-31T18:56:20Z

tests/integration/broken_models/small_office/bad_callback.osw

@@ -0,0 +1,19 @@
+{


I admit that I'm running out of puff, but although I'm may not be catching all of the nuance on these tests, I'm really glad they are here. :)

TShapinsky added 19 commits November 5, 2024 10:21

update to openstudio 3.8, e+ 24.1 and pyfmi 2.11

0f58837

first pass at migration to pyenergyplus

2698801

first version of pyenergyplus migration passing integration tests

0f98b0b

fix formatting, bump min python to 3.11

dbc3978

fix exception import stack

9b56504

fix api tests, import changes, point id changes

67f2f57

add all the new stuff that didn't get comitted earlier

8e63da1

remove goaws from simulation ci test

ff98040

fix simulation test

6696232

fix arguments for timescale test

7c4e45d

starting fixing mock step job

96bc99a

implement initialize_simulation

978d947

correct increment of time in mock job

2129555

remove deprecated models from the scaling tests

24559ad

reduce scale test models to current set

5c792ff

fix simulation model testing

8cd08ad

fix influxdb reporting

d0bc4d2

add sleep to allow mock step run to fall behind

de7c73c

fix step duration

d5e9098

TShapinsky commented Jan 30, 2025

View reviewed changes

docker-compose.dev.yml

- mc

- mongo

- redis

- worker

Copy link

Member Author

TShapinsky Jan 30, 2025

Choose a reason for hiding this comment

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

Removed redundant configuration.

TShapinsky commented Jan 30, 2025

View reviewed changes

anyaelena requested review from anyaelena and kbenne January 30, 2025 20:44

kbenne previously approved these changes Jan 31, 2025

View reviewed changes

Remove more redundant variables in modelica step_run.py

fdbcaa5

TShapinsky dismissed kbenne’s stale review via fdbcaa5 January 31, 2025 21:04

TShapinsky added 2 commits January 31, 2025 14:21

bump bake-action to v5

52bc234

remove files which are no longer needed

8637193

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to pyenergyplus #541

Migrate to pyenergyplus #541

TShapinsky commented Jan 6, 2025 •

edited

Loading

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

kbenne Jan 31, 2025

TShapinsky Jan 31, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

kbenne Jan 31, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

kbenne Jan 31, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

anyaelena Jan 30, 2025

kbenne Jan 31, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

TShapinsky Jan 30, 2025

kbenne left a comment

kbenne Jan 31, 2025

TShapinsky Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025

kbenne Jan 31, 2025


		if __name__ == '__main__':

		basicConfig(level=os.environ.get("LOGLEVEL", "INFO"),

Migrate to pyenergyplus #541

Are you sure you want to change the base?

Migrate to pyenergyplus #541

Conversation

TShapinsky commented Jan 6, 2025 • edited Loading

Finished

In Progress

Todo

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

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

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

kbenne 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

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

TShapinsky commented Jan 6, 2025 •

edited

Loading