Closed santoshphilip closed 8 years ago
Thanks for this. A few questions and comments I was about to leave on the other thread.
Jamie,
Ignore my comment on reorganization. I was thinking about some steps needed in the far future, that I don't have a clarity on yet.
At some point in the far future, I would like to run idf.run()
asynchronously. Python3 has some libraries (asyncio i think) that help in that direction.
In any case let us get the simple stuff done first, before getting into asynchronous stuff :-)
When you talk about asynchronously are you talking about running several simulations at once? That makes perfect sense given that that's almost always the easiest way to speed a script up. I usually use a pool and multiprocessing. I've been meaning to have a look at asynciotoo though. Multiprocessing has a tendency to cause issues with logging so it would be good to get it done correctly here. I get back from my travels later today so will have a look once I'm on non-mobile internet.
Couple of possiblities here:
It seems like an natural next step on this issue Having said that, asynchronous simulation is not a priority at the moment
Jamie,
Some thoughts on organizing idf.run()
So when you write the run() function, it might look like this:
import eppy.runner.run_functions as run_functions
# since your code in the runner folder
#
run_functions.run()
#
# don't be tied to any of the names I have used
# - runner, run_functions, or run.
# if there is a more intuitive naming strategy, go for it
If you have other thoughts on this, let me know.
Ok, so what I've done is create a subclass of IDF
that has the method run
and the attributes epw
and idf
. That calls a run(idf, epw, **kwargs)
function.
# get the filenames
iddfile = os.path.join(IDD_FILES, "Energy+V8_3_0.idd")
fname1 = os.path.join(IDF_FILES, "V8_3/smallfile.idf")
epw = 'USA_CA_San.Francisco.Intl.AP.724940_TMY3.epw'
# create the IDF5 object
IDF5.setiddname(open(iddfile, 'r'), testing=True)
idf = IDF5(open(fname1, 'r'), epw)
# run the IDF
idf.run(readvars=True, output_prefix='test_')
@santoshphilip, are you happy with the directory structure? I went with what you suggested, except for the addition of a config.py file to hold the path to the EnergyPlus install directory (as a place-holder until we have a better way of finding this as discussed in issue #49).
runner
|
+--config.py
+--run_functions.py
+--runner.py
All running is done in a temp directory. This is to make it easier to add mulitprocessing
later.
The API for run()
exposes all of the functionality of the EnergyPlus CLI, apart from --help
. I've also added a verbose
option which suppresses the EnergyPlus output to std out if set to 'q'.
Tests cover all the CLI options individually and a couple of combinations for IDF5.run()
, as well as a few additional tests on the run()
function on its own including handling absolute and relative paths, and handling just the epw file name for a file in the EnergyPlus weather subdirectory (no need to specify the full path).
API
"""
Wrapper around the EnergyPlus command line interface.
Parameters
----------
idf : str
Full or relative path to the IDF file to be run.
weather : str
Full or relative path to the weather file.
output_directory : str, optional
Full or relative path to an output directory (default: 'run_outputs)
annual : bool, optional
If True then force annual simulation (default: False)
design_day : bool, optional
Force design-day-only simulation (default: False)
idd : str, optional
Input data dictionary (default: Energy+.idd in EnergyPlus directory)
epmacro : str, optional
Run EPMacro prior to simulation (default: False).
expandobjects : bool, optional
Run ExpandObjects prior to simulation (default: False)
readvars : bool, optional
Run ReadVarsESO after simulation (default: False)
output_prefix : str, optional
Prefix for output file names (default: eplus)
output_suffix : str, optional
Suffix style for output file names (default: L)
L: Legacy (e.g., eplustbl.csv)
C: Capital (e.g., eplusTable.csv)
D: Dash (e.g., eplus-table.csv)
version : bool, optional
Display version information (default: False)
verbose: str
Set verbosity of runtime messages (default: v)
v: verbose
q: quiet
Raises
------
CalledProcessError
"""
Adding multiprocessing
for IDF5 objects is a slightly harder prospect since there seems to be a problem with pickling EPBunch
objects (which is necessary since multiprocessing
relies on Queue
). I have it working for the run(idf, epw)
function but not for the IDF5.run()
method.
Traceback (most recent call last):
File "C:\Miniconda\envs\eppy\lib\multiprocessing\util.py", line 274, in _run_finalizers
finalizer()
File "C:\Miniconda\envs\eppy\lib\multiprocessing\util.py", line 207, in __call__
res = self._callback(*self._args, **self._kwargs)
File "C:\Miniconda\envs\eppy\lib\multiprocessing\pool.py", line 495, in _terminate_pool
cls._help_stuff_finish(inqueue, task_handler, len(pool))
File "C:\Miniconda\envs\eppy\lib\multiprocessing\pool.py", line 482, in _help_stuff_finish
inqueue._reader.recv()
File "C:\Users\Jamie\git\eppy\eppy\bunch_subclass.py", line 136, in __getattr__
return super(EpBunch_3, self).__getattr__(name)
File "C:\Users\Jamie\git\eppy\eppy\bunch_subclass.py", line 110, in __getattr__
return super(EpBunch_2, self).__getattr__(name)
File "C:\Users\Jamie\git\eppy\eppy\bunch_subclass.py", line 73, in __getattr__
raise BadEPFieldError(astr)
BadEPFieldError: unable to find field __setstate__
BadEPFieldError: unable to find field __setstate__
I've put in place a workaround which takes a list of IDFs and kwargs, saves out the IDF file and weather file to a temporary directory, then passes those paths and kwargs into a pool using the run(idf, epw, **kwargs)
function.
There goes my weekend. I mean that in a good way :-)
I can't wait to go thru this. Will review and respond.
I just reminded myself of another issue that you'll need to know about to run the tests (trying to get things working on Ubuntu).
The problem is that ReadVarsESO.exe
is installed in the PostProcess subdirectory, but the CLI looks for it in the EnergyPlus install directory. This means that tests which rely on the --readvars
flag (three of them) will fail.
This is a known issue with EnergyPlus 8.3. Not sure when it's expected to be fixed although the issue over there has been closed.
The temporary fix is to copy ReadVarsESO.exe
into the install directory
I was testing on a Mac. Did not succeed Will be tyring on a windows machine.
Sent from my iPad
On Sep 13, 2015, at 2:36 AM, Jamie Bull notifications@github.com wrote:
I just reminded myself of another issue that you'll need to know about to run the tests (trying to get things working on Ubuntu).
The problem is that ReadVarsESO.exe is installed in the PostProcess subdirectory, but the CLI looks for it in the EnergyPlus install directory. This means that tests which rely on the --readvars flag (three of them) will fail.
This is a known issue with EnergyPlus 8.3. Not sure when it's expected to be fixed.
The temporary fix is to copy ReadVarsESO.exe into the install directory
— Reply to this email directly or view it on GitHub.
On a mac I think the same will apply. You'll also need to change the path in runner/config.py
. Make sure you have the latest version as I've made a couple of fixes.
I'll test this over the weekend.
Jamie, I just tested this. This is awesome. I am sorry I took so long to get to this. (I may have broken your momentum in working on this)
This should be added to the tutorial. Putting it into the tutorial will ensure that the interface/API works smoothly.
No problem. I've made a few changes in the version I use to make it work cross-platform (Mac, Linux, Windows). I can't test it on those but a colleague has it working on Mac and we have it running on AWS on a Linux VM. I'll add those changes in to the Eppy version when I get minute.
For the tutorial, you just need a few example snippets and a bit of explanation?
A simple tutorial as you described would be fine. If it is reStructuredText format, I can easily slip it into the main documentation. If reStructuredText is new to you, don't bother. I can do that.
Right, back on the ball again. I've made the few changes I mentioned back in October regarding cross-platform support, and also found a sensible workaround for the help
option of the EnergyPlus CLI which avoids squashing the Python built-in function name. This is documented in a new section at the end of the newfunctions.rst
file. If that's not the right place for it let me know and I can move it elsewhere.
Thanks Jamie, I'll take a look. On another note, I think a lot of command line stuff is not working on the latest Mac (El Capitan). The new releasee of the OS has changed some of permissions, resulting in problems In any case, this is a problem for the energyplus development team. Not much we can do
I should probably post a bug report on it for the energy plus dev. team
Yes, I've had someone else ask me about this. I'll ask a question on UnmetHours and see if anyone has more info (edit: https://unmethours.com/question/13844/el-capitan-breaking-energyplus/). It was related to EnergyPlus putting files in /usr/bin and El Capitan no longer allowing that (the "rootless" / System Integrity Protection feature). Does that sound like the same issue? On Mon, 11 Jan 2016, 03:06 santoshphilip notifications@github.com wrote:
Thanks Jamie, I'll take a look. On another note, I think a lot of command line stuff is not working on the latest Mac (El Capitan). The new releasee of the OS has changed some of permissions, resulting in problems In any case, this is a problem for the energyplus development team. Not much we can do
I should probably post a bug report on it for the energy plus dev. team
— Reply to this email directly or view it on GitHub https://github.com/santoshphilip/eppy/issues/62#issuecomment-170416210.
I've just tried to run py.test test_runner.py
from the command line on Windows and noticed that it hangs on the first test to use multiprocessing
. The explanation is here. I missed it previously as the tests run fine from inside Eclipse.
Ok, here's a workaround for running multiprocessing tests on Windows from the command line:
python.exe -c "import pytest; pytest.main('test_runner.py')"
And with coverage
(reporting missed lines):
python.exe -c "import pytest; pytest.main('test_runner.py --cov-report term-missing --cov eppy.runner')"
--------------- coverage: platform win32, python 2.7.10-final-0 ---------------
Name Stmts Miss Cover Missing
------------------------------------------------------------------------------------
C:\Users\Jamie\git\eppy\eppy\runner\__init__.py 0 0 100%
C:\Users\Jamie\git\eppy\eppy\runner\run_functions.py 76 6 92% 30-35, 87
C:\Users\Jamie\git\eppy\eppy\runner\runner.py 23 0 100%
------------------------------------------------------------------------------------
TOTAL 99 6 94%
========================= 23 passed in 24.33 seconds ==========================
Missing lines are those run from inside the multiprocessing.Pool
(87) and the path-setting for platforms other than Windows (30-35).
Regarding El Capitan, it seems that the issue relates to running E+ from the .bat
files which try and write to /usr/bin
. The response to my question on UnmetHours is to use the CLI which is exactly what we're already doing. I'd like to see the tests run by someone who runs El Capitan, but this "rootless" issue shouldn't be a problem.
Also @mbadams5 has opened EnergyPlus issue #5410 on this so no need for you to do so now.
Any chance to take a look at this? I've tested on Linux (2.7 and 3.5) and Windows (2.7 only). Still needs testing on Mac and 3.5 on Windows.
I'll test on the Mac tomorrow.
Santosh
Sent from my iPad
On Jan 25, 2016, at 4:53 PM, Jamie Bull notifications@github.com wrote:
Any chance to take a look at this? I've tested on Linux (2.7 and 3.5) and Windows (2.7 only). Still needs testing on Mac and 3.5 on Windows.
— Reply to this email directly or view it on GitHub.
runs on mac - python2.7
runs on mac - python3.5 found an unrelated bug with python3.5 having to do with unicode :-( I might have to bite the bullet and deal with the whole unicode business
The code I tested looks like this:
# running from the ./docs folder
import sys
# pathnameto_eppy = 'c:/eppy'
pathnameto_eppy = '../'
sys.path.append(pathnameto_eppy)
import eppy.runner.runner
iddfile = "/Applications/EnergyPlus-8-4-0/Energy+.idd"
fname1 = "/Applications/EnergyPlus-8-4-0/ExampleFiles/1ZoneEvapCooler.idf"
epw = '/Applications/EnergyPlus-8-4-0/WeatherData/USA_CA_San.Francisco.Intl.AP.724940_TMY3.epw'
IDF5 = eppy.runner.runner.IDF5
IDF5.setiddname(iddfile)
idf = IDF5(open(fname1, 'r'), epw)
idf.run()
At this point we have proof of concept. It works and we are good. The rest is just details :-)
Some further thoughts
I would like to modify it (using IDF rather than IDF5) so that it looks like:
# running from the ./docs folder
import sys
# pathnameto_eppy = 'c:/eppy'
pathnameto_eppy = '../'
sys.path.append(pathnameto_eppy)
from eppy import modeleditor
from eppy.modeleditor import IDF
iddfile = "/Applications/EnergyPlus-8-4-0/Energy+.idd"
fname1 = "/Applications/EnergyPlus-8-4-0/ExampleFiles/1ZoneEvapCooler.idf"
epw = '/Applications/EnergyPlus-8-4-0/WeatherData/USA_CA_San.Francisco.Intl.AP.724940_TMY3.epw'
IDF.setiddname(iddfile)
idf = IDF(open(fname1, 'r'), epw)
idf.run()
Give me a few days to stare at your code and think of the best way to integrate it with the rest of the code
My other thought is on the output files EP-Launch does the following
I usually use EP-Launch, so I am accustomed to working with this file structure. Is there a rationale for doing it differently.
I would like input from @JasonGlazer on this. I'll send him an email to look at this thread
Great news. I'll install Python3 today and do the last check on Windows. I've also added one more test to demonstrate asynchronous running using concurrent.futures
(compatible with Python2 and 3) which I'll check in today.
As well as running from the console, did you also run the tests? They cover multiple concurrent runs which is one area that may be treated differently on Mac.
On structure, one option is to convert IDF5
to a mixin, say RunnableIDFMixin
, and add that to the instantiation of IDF
. This is quite nice as it simplifies the main logic of IDF
, and allows the new functionality to have a descriptive name and remain somewhat logically separate. The (simpler) alternative is just move the IDF5.run
method into the IDF
class. I'm not sure I understand why we have IDF2
, IDF3
, etc. anyway. Is there a reason their methods aren't all in IDF
?
As to the logic of outputs, what I intended was to replicate the command line interface default behaviour for the filenames (eplusout.<ext>
). This makes scripting a little easier as you are always looking for the same *.err
filename, eplusout.err
. You're right though that the default should be to output to the current directory. I've changed that.
The behaviour of EP-Launch isn't directly achievable using the CLI, so we'd have to add a little extra to the logic of runner.run
to rename the output files to <input file>.<ext>
. I think people using a Python wrapper to run EnergyPlus might be less surprised to see the default CLI behaviour rather than EP-Launch, but I'm happy to defer to others opinion on this.
On Unicode, how about a new issue to handle that and maybe another one for my other pet peeve of case-sensitivity of EnergyPlus objects, etc? I assume there's a dict or OrderedDict
somewhere hidden inside Eppy that we could convert to a CaseInsensitiveDict
/CaseInsensitiveOrderedDict
?
just tested python3 on windows7 Works fine
Great! The day ran away with me and I didn't get a chance. Let me know your thoughts on integrating it once you've had a chance to think about it.
Thinking about it a little myself, to fit with what you already have, the obvious thing to do would be to either move the IDF5
class into modeleditor
or import it there, then switch IDF = IDF4
for IDF = IDF5
.
Anyway - I'll stop thinking now. I'm supposed to be on holiday!
I also did successfully test it:
I have not yet run your tests (the concurrent runs). Will do so soon.
Thinking aloud here: This should not be tested in a unit test, since there is an external dependency (depends on E+) E+ may not be available, or may be malfunctioning, causing the test to fail, even though the the code is fine.
We might need to put together some kind of integration tests or system tests (not sure if I am using the right terminology)
Yes, quite right, they're not strictly unit tests. What I was thinking of doing was adding something to skip those tests if EnergyPlus isn't found. But also a test that EnergyPlus is installed where expected for the platform (until we implement that automatic function to find where it's installed) as that is the most likely issue someone will have when installing Eppy and running the tests.
The tests in test_runner.py
take about 30 seconds to run so well worth testing on the missing platforms, if only to make sure I haven't broken anything else!
Also, nice one on running in the notebook. I hadn't thought of that. So now I can put together a notebook demo to show off the running capability, results extraction, etc. Brilliant!
I am running py.test eppy/tests/test_runner.py
on a mac with python2.7
(after doing pip install futures
)
I get the error:
for future in futures.as_completed(to_do):
next_expected = next(expected_results)
res = future.result()
> assert res == next_expected
E assert 'OK' == 4
test_runner.py:99: AssertionError
Any quick thoughts on it ? You are doing whole bunch of stuff I am unfamiliar with. I thought I'll ask you before I spend time groking your code (I'll have to do that at some point :-)
Yes, I noticed later that that one is a bit shaky. It depends on how the interpreter calls the threads. I'll add something to make the test IDF take longer to run and then it should be fine.
The basic idea of that test is it sets up and runs three asynchronous threads, two of which are fast as they just call len('eppy') or len('spam'), and one which is slow as it's running EnergyPlus. The threads are started fast, slow, fast and the results should come back fast, fast, slow. But running the tiny test IDF is also pretty fast so sometimes they come back out of order. As I say, if I make the run period longer in the IDF it should make the test more reliable.
On Sun, 31 Jan 2016, 12:25 santoshphilip notifications@github.com wrote:
I am running py.test eppy/tests/test_runner.py on a mac with python2.7 (after doing pip install futures)
I get the error:
for future in futures.as_completed(to_do): next_expected = next(expected_results) res = future.result()
assert res == next_expected
E assert 'OK' == 4
test_runner.py:99: AssertionError
Any quick thoughts on it ? You are doing whole bunch of stuff I am unfamiliar with. I thought I'll ask you before I spend time groking your code (I'll have to do that at some point :-)
— Reply to this email directly or view it on GitHub https://github.com/santoshphilip/eppy/issues/62#issuecomment-177487421.
Jamie,
I wanted to respond to why we have IDF1, IDF2, IDF3
etc.
This also applies to EpBunch_1, EpBunch_2, EpBunch_3
from bunch_subclass.
In the best of worlds, there should be one IDF
and one EpBunch
The coding in 'IDF' and 'EpBunch' was really tricky. Especially in 'EpBunch' since I was overriding __getitem__
and __setitem__
in some tricky ways. Debugging was difficult because when __getitem__
and __setitem__
malfunction a whole bunch of other things break.
I was proceeding very slowly and incrementally. I make the first class that well tested and worked well Instead of making changes within that class, I make another class, so that any bugs were isolated within the new class.
In effect IDF0 -> IDF1 -> IDF2 ->
follows the chronological development of the class.
I am inclined to leave it that way for now. Maybe at some later point it can be refactored.
I had the code reviewed by some more experienced developers at a python meetup. They had the same question you had. It is a little awkward when you are reading the code and trying to figure out what the code is doing.
Ok, that makes sense - it's an archaeological artifact. Probably worth refactoring at some point then as it certainly confused me when I first came across it. That's kind of a key module so it would be great for new contributors to have it be as clear as possible.
On the threaded running, it seems harder than I expected to make this behave in any kind of reproducible manner - even with threads that don't actually call EnergyPlus! I'm tempted to leave that test out until I can spend some time getting to grips with asyncio
. We already have the tests for multirun
anyway so the concurrent.futures
is something of a duplication of that. That way you only need to understand the basic functions, and figure out how best to integrate it before we can add it to develop
.
Let us leave that test out for the moment. That will give me a stable point to start to integrate your code. Turn it off and let me know
I am putting some strategic thought into how to separate E+ dependancies from the pure python stuff. Mostly thinking about the API design. I'll put something together and ping you for feedback.
Ok, that's updated now. And yep, let me know your thoughts on the API.
@santoshphilip I've merged the latest develop
branch into this. Any references to IDF5
are now IDF6
but otherwise no other changes required to keep the tests passing.
I stated earlier:
In effect IDF0 -> IDF1 -> IDF2 -> follows the chronological development of the class.
This leads to the conclusion that the code would have greater clarity if it was a single class IDF
that has all the functionality.
Another approach would be to start with and base class and to create a series of child classes, each of which has greater functionality that is obvious from the class name. The classic example that is usually waved around is shown below. It comes from computer graphics
# much pseudocode here
class basicpoint(object)
x
y
class coloredpoint(point)
# rgb
red
green
blue
class drawpoint(coloredpoint)
def draw_thepoint()
# some code that draws the point on the screen in color
I think IDF0 -> IDF1 -> IDF2 has such a logic to it. Especially if I give them more meaningful names and shuffle the code around a bit. In fact that was my original thinking behind it. I did not give them meaningful names at that time, because I did not know what I was doing until I did it :-)
The proof is in the pudding. So I need to look at the code to see if such a direction is viable and will actually add clarity. If the code does not look amenable to such a modification, we can just make a single class and be done with it.
Hi Guys, that is excellent news, Can you please share me the latest code to run E+ using Eppy?
Thanks
Hi @barsa123 - the code for running models directly from Eppy is currently on the i62_eplusrunner
branch and hasn't yet been merged into develop
. You can check out that branch and use it though - let me know if you need any help with getting that working - and any feedback you have would be very welcome.
Hi Jamie,
Many thanks for quick reponse, I tried to run i62 version but couldnt run eplusrunner after the installation didnt recognize eppy.runner from python. I am not very expert on the python. Do you mind if you can send me a test unit to running and idf file with epw file? or if you like I can send you one ? Many thanks for your time and prompt reponse.
Regards
Barsa
Do you have another version of eppy installed?
edit
Ah, it looks like I was still importing futures
which isn't required any more. If you didn't have it installed then the eppy.runner
module wouldn't be imported. I've just pushed the fix so try pulling that and see if it fixes the problem.
You can run the unit tests for just the runner
functions by running py.test eppy/tests/test_runner.py
from the root eppy
directory. There's also a walkthrough of how to use IDF.run()
at the end of eppy/docs/source/newfunctions.rst
.
Hi Jamie,
I have included following code in my eppy eventhough uninstall all epy and reinstalled with your version including following lines:
from eppy.runner import IDF5
fname1 = "../eppy/resources/idffiles/V_8_3/smallfile.idf"
epw = 'USA_CA_San.Francisco.Intl.AP.724940_TMY3.epw'
idf = IDF5(open(fname1, 'r'), epw)
idf.run()
But I have fallowing error message:
import eppy.runner
ImportError: No module named runner
I am using Python 2.7 what do you suggest?
Regards Barsa
Have you tried running the unit tests?
On Tue, 15 Mar 2016, 18:44 barsa123, notifications@github.com wrote:
Hi Jamie,
I have included following code in my eppy eventhough uninstall all epy and reinstalled with your version including following lines:
from eppy.runner import IDF5
fname1 = "../eppy/resources/idffiles/V_8_3/smallfile.idf" epw = 'USA_CA_San.Francisco.Intl.AP.724940_TMY3.epw' idf = IDF5(open(fname1, 'r'), epw)
idf.run()
But I have fallowing error message:
import eppy.runner
ImportError: No module named runner
I am using Python 2.7 what do you suggest?
Regards Barsa
— You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub https://github.com/santoshphilip/eppy/issues/62#issuecomment-196943931
Can you give an example which unit taste you meant?
Try running just the the runner test with py.test eppy/tests/test_runner.py
from the eppy directory. If that doesn't work, try py.test eppy
and let me know what the outputs are.
function to run E+ from eppy
See implementation details in this link
full discussion of the issue is here
development will be done in branch
i62_eplusrunner
Jamie, do
git checkout -b i62_eplusrunner origin/i62_eplusrunner
to checkout this branch