USEPA / SWMM-EPANET_User_Interface

User interface for the Stormwater-Management-Model
111 stars 67 forks source link

SWMM output access failure post simulation in python 3 environment #282

Closed TongZhai closed 6 years ago

TongZhai commented 6 years ago

SwmmOutputObject class instantiation for opening output file after a simulation always fail, it seems the swmm5_x64.dll swmm_close routine can't close all linkage to the output files after a simulation (in the python 3 environment), hence, it brings down the whole program when another code tries to open the .out file

michaeltryby commented 6 years ago

@TongZhai so it sounds like the simulation runs and generates a binary file, you call swmm_close() but the binary file is still busy so when you try to open it using the output API the program crashes. This seems strange, cause the output API should issue "File Error 434: unable to open the binary file" and quit nicely. I will write a test and make sure things are working properly.

The version of the output API you are using is out of date. Here is a newer one. swmm_output-0.1.0a0-cp36-cp36m-win_amd64.zip

You can find the source code here.

TongZhai commented 6 years ago

@michaeltryby the output API did give an error message (Error 436 opening C:/Data/SWMM/Example1_trial_21_39_33.out retrying...). I will apply your latest output API. But quitting nicely if output file is busy still poses an issue, in that I can see that the simulation run to completion and the ro, flow error terms are all correct, so we should be able to access the output... I am thinking perhaps we need to examine the SWMMlibobj.swmm_close() routine to ensure output file be ready for access immediately after a successful simulation.

TongZhai commented 6 years ago

@michaeltryby the pip library in the osgeo4w install has problem running, there is a ticket for it on osgeo git repo, so for the time being, can you post the swmm output api in a zip file, I mean not the whl file.

TongZhai commented 6 years ago

@michaeltryby I was able to update my pip to install the whl package you posted. It looks like it involves a 'swmm_output.py' file that is installed into the osgeo's site package directory and it has very different class structure from the current SMOutputWrapper.py, I think we will need to have a pow-wow together on how to use this new version because it looks like this might need more work than a simple update of the existing code...

michaeltryby commented 6 years ago

@TongZhai its essentially the same library wrapped in Python two different ways. We were using ctypes now we are using SWIG. Only the names of the functions have changed. It's more pythonic now and should actually be easier to use and faster.

TongZhai commented 6 years ago

@michaeltryby Nonetheless, this is quite different from the current codes for accessing various swmm output items, will take a bit time to upgrade to this new api... I will work on that and ask a lot of questions. Still, we are facing the bigger issue of swmm5_x64.dll can't close/release output file after a good simulation, would it be more advantageous to deal with that so as to get the process flowing?

michaeltryby commented 6 years ago

@TongZhai I think you will be pleasantly surprised. When I switched from ctypes to SWIG I was able to eliminate a lot of code. I am going to do the same thing for the swmm dll API.

TongZhai commented 6 years ago

@michaeltryby that sounds great. one thought, if possible, if you can retain the original set of functions (their signatures) in the current swmm5_x64.dll, it will make the upgrade effort much easier.

TongZhai commented 6 years ago

@michaeltryby I have been looking and probing the new SWIG output api and I think a document about the api would be needed for the upgrade. Also some small examples for the call sequence when opening/accessing the output would be great. For example, the ctypes need to initiate pointers and call an open output routine before accessing, how are these done or even needed in the new api is not clear. The installed swig api has all of the functions with real signature unknown due to pythons variable arguments. Can I assume they work the same way if their names are close to their ctypes equivalent (if there is a close named function)? again, I think a document would be great for this.

TongZhai commented 6 years ago

@michaeltryby @barrc the new SWIG swmm output api doesn't have nodeCount, subcatchCount, linkCount, and pollutantCount state variables, so does the smo_get_project_size function return all sizes at once? right now its signature is rejecting the last int argument for holding the counts of different type of model object.

michaeltryby commented 6 years ago

@TongZhai get_project_size() returns a list with all the count state variables.

TongZhai commented 6 years ago

@michaeltryby what is the order of these model elements' counts? does it return all model object types?

michaeltryby commented 6 years ago

@TongZhai It returns a list with 4 elements Nsubcatch, Nnode, Nlink, and Npollut.

The best place to learn this is by referring to the source code for the SWMM Output library.

TongZhai commented 6 years ago

@michaeltryby do the 'get_xxxx_series', 'get_xxxx_result', and 'get_xxxx_attribute' in the new output api returns a list of values? the ctype version has to create an array and save its address, which gets pass into other routine to save the returned values, so do the new routines forgo such a step? The C code seems to still need to take in a 2D array and returns error code for these routines... so please help confirm.

michaeltryby commented 6 years ago

@TongZhai Yep they all return lists. You don't need to worry about the arrays its all handled for you in the wrapper code and errors are thrown as exceptions.

TongZhai commented 6 years ago

@michaeltryby ok, finally got the new output api retrofitted and hooked up to the process, now the model runs, but it seems the binary output file is not getting written correctly, as it is only just 1KB in size, also, now the new output api simply throw an Error 440 (unspecified error) and except out of the workflow. So, at this point, we need to deal with the swmm engine's output handling to have this fully functional again.

michaeltryby commented 6 years ago

@TongZhai thanks for your work on this. What has changed since this code worked? We have upgraded Python from 2 to 3. Anything else? Why would this cause the binary file to change? The binary file is written by the dll so why would it be any different? So this doesn’t exactly add up. I suppose that’s why they call them bugs.

TongZhai commented 6 years ago

@michaeltryby Ok, I will start debugging the SWMM engine then, will probably need to look at the C source code for swmm5_x64.dll (is it on a git repo somewhere?)

michaeltryby commented 6 years ago

Do you have this code checked in somewhere? I’m not certain the swmm code is the place to start.

TongZhai commented 6 years ago

@michaeltryby the engine wrapper is the swmm5.py file

michaeltryby commented 6 years ago

Where is the code you updated with the new outputapi package?

TongZhai commented 6 years ago

@michaeltryby the new output api wrapper is the SMOutputSWIG.py file

https://github.com/USEPA/SWMM-EPANET_User_Interface/blob/dev-ui-py3qt5/src/Externals/swmm/outputapi/SMOutputSWIG.py

TongZhai commented 6 years ago

@michaeltryby just ran some more test on the output api using an output file from a successful run from an external simulation, the 'smo_init()' call was originally (ctypes) getting back a LP_struct_SMOutputAPI object, but the SWIG call gets back <Swig Object of type 'void **' at 0x0000000003D097E0>, then it failed from there.

TongZhai commented 6 years ago

@michaeltryby ran another round of test of the swmm engine api, again, the simulation is complete, but the output file is not closed properly, which is reflected by the small file size as it looks like it did not get the flushed buffer of outputs into that file somehow. We could first examine 'swmm_end' and 'swmm_close' routines of the engine api as a start. It looks like this is not a new issue, because I found that there is a loop for 5 tries to open the swmm output binary files in the swmm output wrapper (from the python 2 version), which is indicative of the original effort to forestall the "access too early" issue.

michaeltryby commented 6 years ago

SWIG call gets back <Swig Object of type 'void **' at 0x0000000003D097E0>, then it failed from there.

@TongZhai that’s the correct return value for the swig wrapped version of smo_init(). Honestly, I use the swig output api in Python 2 for the testing framework and @bemcdonnell uses it extensively in Python 3 for his work flow. Therefore I’m pretty confident this package is working properly.

michaeltryby commented 6 years ago

@TongZhai are you absolutely certain that swmm_close() gets called prior to smo_open()? It’s not going to work otherwise.

TongZhai commented 6 years ago

@michaeltryby yes, actually, it is 'swmm_end', then 'swmm_close', then open the output in that order. then, it gives the 440 error due to incomplete binary output file. I also see that the 'smo_open' returns 'None', is that right?

michaeltryby commented 6 years ago

@TongZhai That’s correct smo_open() returns none.

michaeltryby commented 6 years ago

@TongZhai I think I may have discovered something. The function swmm_start() takes a 'save results' flag as an argument, but this flag appears to be omitted in the swmm5.py interface file you are using.

In my testing when the start flag is set to 0 the test throws an exception when is opens the binary output file after the simulation. When the start flag is set to 1 the test works as expected. I think this may be the source of the problem.

TongZhai commented 6 years ago

@michaeltryby I added the 1 flag to the swmm_start(1) call and SWMM simulation is reliably generating the .out file now. This resolves the first half of issue! So, the remaining problem is the SWIG output api, starting with the swmm_open(ptrapi, output_filename) call, it returns 'None' as expected, very quickly, but subsequent calls are failing, e.g. SMO_pollutant_conc_node() call, it excepted out with 'int is not callable' error message. About the return value from the swmm_open call, I still think some type of integer error code would be good for proper response in case of failure, with None, does that mean that anything other than None is problematic?

TongZhai commented 6 years ago

@michaeltryby I am not quite sure about the SWIG SWMM output api, I guessed pretty much from looking at their python stubs and the source code, so if you have an independent testing script for the output api, that would be great to be included in the repo so as to minimize the guess work when it comes to properly calling the various functions.

michaeltryby commented 6 years ago

@TongZhai here you go. I created new base packages called swmm-python. swmm-toolkit and swmm-output have been organized into implicit namespace packages. Using SWIG I added documentation to module files. Unit tests are currently passing.

This pre-release contains binary wheels for python 3.6.5 amd64. Tests can be found in the repository.
https://github.com/OpenWaterAnalytics/swmm-python/releases/tag/v0.2.0-dev

I used these newly created packages on my machine to debug your issue. So I am absolutely certain they work properly.

TongZhai commented 6 years ago

@michaeltryby I just uninstalled the previous version of swmm output api and installed the 0.2 version whl, and I can see it is in the list of installed package, but I was unable to find it in the import, the previous version, I can see _swmm_output namespace right away, but now, there is only a 'swmm' namespace, but it seems empty, so is the couple levels down its 'output' folder... so how do you access it?

michaeltryby commented 6 years ago

@TongZhai for unit testing I imported it like this ... from swmm.output import output as smo

TongZhai commented 6 years ago

@michaeltryby looks like some of the apis are changed in this version, anyway, I will update them in the program.

TongZhai commented 6 years ago

@michaeltryby getsubcatchattribute, getnodeattribute, getlinkattribute, getsystemattribute are not in the testing script, they seem to be in the output api namespace, they failed when called with output handle, time index, and an attribute as argument. The attribute's index is set to the proper enum value for output item, e.g. SubcatchAttribute.INFIL_LOSS etc. Will need to look at the source code to see what's wrong.

michaeltryby commented 6 years ago

@TongZhai What error information did you receive if any?

michaeltryby commented 6 years ago

@TongZhai I just wrote a test for getsubcatchattribute() and it is passing with no changes to the library. You can find the test here.

TongZhai commented 6 years ago

@michaeltryby the new test helped pinpoint to the 3rd argument to these functions, instead of the usual SMO output attribute object, this time it asks for its index., i.e. its output's enum value. So, these are updated and now its working.

michaeltryby commented 6 years ago

@TongZhai if you look at the output module file it contains the enum classes needed to use the api. And the value property gets called automatically.

TongZhai commented 6 years ago

@michaeltryby At this point, the SWMM engine and output SWIG API are working correctly in the new Python 3 environment.