Draft for OWA_v5.1.14 release

[abhiramm7]

This PR is for us to review the changes and additions since the previous release and prep the code for release.

Closes #344

[abhiramm7]

@bemason, for the pollutant getters in pyswmm, can you confirm that all tests are passing and we are good to go? I want to make sure we are good on all downstream things before the release.

CC: @karosc

[bemason]

@abhiramm7 I ran all the tests and a few failed but none of the pollutant tests.

Looks like for all five functions that failed were because of the swmm_getVersion() function?

[jennwuu]

@bemason

It looks like the SWMM version getter functions have changed in the api: https://github.com/OpenWaterAnalytics/Stormwater-Management-Model/blob/6d6b5272250acaa3939db9e265f0e593dc96e43e/src/solver/toolkit.c#L46-L62

[jennwuu]

@bemason, for the pollutant getters in pyswmm, can you confirm that all tests are passing and we are good to go? I want to make sure we are good on all downstream things before the release.

CC: @karosc

I also see some merge conflicts that needs to be addressed before merge.

[jennwuu]

The SWMM version getter used to look like this: https://github.com/OpenWaterAnalytics/Stormwater-Management-Model/blob/93891ff1e3775073895a3b96f38fa6999e961ed9/src/solver/toolkit.c#L52-L63

referenced by: https://github.com/OpenWaterAnalytics/swmm-python/blob/dev/swmm-toolkit/src/swmm/toolkit/solver_rename.i#L102

[bemason]

Good catch @jennwuu. @abhiramm7 is there something else you need from me? I'm not sure how to rectify the above. I presume this is something you'll handle on your end?

[michaeltryby]

Hey @jennwuu and @abhiramm7,

The version API’s were kind of a mess. I refactored them when I added the version header feature.

Have you checked out the version header? It’s generated automatically! The version APIs are a lot more rational now and provide useful information like an actual build ID!

The new version function also eliminates a bunch of unnecessary copying and argument passing. It should be a piece of cake to update the wrappers and tests for the new function calls.

[jennwuu]

It looks like the new version functions return string instead of tuple of integers.

[karosc]

@abhiramm7 do you need any help addressing the version getter? I could put together a PR, but I'm not sure which branch you'd want it on. If I submit a PR to release_version, then master would be ahead of develop after merging.

[abhiramm7]

@karosc, that would be awesome! I like the addition of auto-generating the version number by @michaeltryby. I suggest we start a branch in swmm-python to address the downstream patches. I assume that is what you meant by creating PR?

I think we should have a parallel release in swmm-python, pyswmm, and OWA-swmm. That way, we can fix any bugs in the right place and keep things clean. I would love to know what you all think about this :)

[karosc]

@michaeltryby, I am having trouble debugging get_version_legacy. I can see that the semantic version is defined in the CMakeLists.txt, which I think is how swmm_getSemVersion works, but I cannot find where VERSION_MAJOR, VERSION_MINOR, and VERSION_PATCH are defined in CMake. I think this might be why get_version_legacy is broken. Are we deprecating that function entirely? Should we just remove it, or patch it for legacy compatibility?

If we leave SWMM as is, I think we can just patch pyswmm's call to swmm_version_info.

[michaeltryby]

We can merge master back into develop after the release

[michaeltryby]

Here is a link to cmake docs on project defined VERSION variables:

https://cmake.org/cmake/help/latest/variable/PROJECT-NAME_VERSION.html

[karosc]

Thanks @michaeltryby , I was able to find in the generated header that has the integer variables are defined correctly. However, I am still getting an exception when I try to call solver.swmm_get_version() from swmm-python, which should call get_version_legacy and return an int.

I am not sure if it's necessary to have this function working since it is legacy...but if it is broken and not intended for future use, should we remove it from swmm-python in this release?

I also realize I might be going down a rabbit hole since we really just need to patch pyswmm's calls to solver.swmm_version_info() and that solver.swmm_get_version() issues are unrelated , I just like to button up loose ends when I see them.

[abhiramm7]

@karosc I agree, we should remove legacy function. Unless @jennwuu and @michaeltryby think otherwise

[michaeltryby]

@karosc I too have confirmed that the C library is working properly. The bug is in the wrapper code. I would like to preserve the behavior of swmm_getVersion() for the sake of backward compatibility.

[karosc]

@michaeltryby, do you think we need to fix the wrapper code before release? Or can we remove it from the wrapper while leaving it in SWMM? Or would you prefer we leave the broken swmm-python function be and just patch pyswmm?

[michaeltryby]

I suspect this bug has always been there and we are only noticing it now. All the SWMM API functions return error codes. The wrapper ignores the int code returned and queries the swmm_getAPIError() function instead. The solution is to write a custom typemap for swmm_getVersion() so the int return value gets returned instead of ignored.

[michaeltryby]

My philosophy is that when you release you should close known bugs that you have workable solutions for

[karosc]

Thanks for the help @michaeltryby! I dug into the swig docs and found that adding the following to the solver.i file fixes the solver.swmm_get_version() bug for me. Are you good with me putting a PR together like this for swmm-python then patching pyswmm?

%exception swmm_getVersion
{
    $function
}

%typemap(out) int swmm_getVersion {
  $result = PyInt_FromLong($1);
}

[michaeltryby]

@karosc Fantastic! Go for it!

[karosc]

Thanks!

I added the swmm-python legacy version fixes to my current swmm-python PR and put together a one-line patch for pyswmm's version getter

[bemcdonnell]

@jennwuu and @abhiramm7 can you please run through the list of open issues and add them to @abhiramm7's initial comment with the issues that this PR intends to close? I don't have a sense for all the new features beyond the WQ api.

[abhiramm7]

@jennwuu I can work on the rest of the things if you want to take point on the flow direction thing. I don't image this fix needs anything updated downstream in pyswmm or swmm-python

[jennwuu]

@abhiramm7 Yeah - it just needs to unit tests as described by Bryant to confirm the bug fix has been addressed. Thanks!

[abhiramm7]

@bemcdonnell, not sure if you saw this already. Was there something you wanted to include on the checklist https://github.com/OpenWaterAnalytics/Stormwater-Management-Model/wiki/Release-Checklist

[karosc]

@abhiramm7 or @jennwuu, I have time to put together a unit test for the flow direction fix if that will help keep this release moving forward.

[jennwuu]

Hey @karosc That would be great if you have time and want to add an unit test. I can help to review. Thanks!! 🦥

[bemcdonnell]

@karosc that would be awesome. I think that might be the only test holding up the release. I understand there is a bigger checklist, however.

[abhiramm7]

@karosc that would be awesome. I think that might be the only test holding up the release. I understand there is a bigger checklist, however.

@bemcdonnell , yeah. I think we are good on most of the checklist items 🤞🏼 . It is a matter of verifying them :)

[abhiramm7]

Logs look good; we just seem to have a move issue. I think we should be ok for now.

mv: rename receipt.json to /Users/runner/work/Stormwater-Management-Model/Stormwater-Management-Model/upload/receipt.json: No such file or directory

[michaeltryby]

@abhiramm7 all runners are in the green. Where are you seeing this error?

[abhiramm7]

@abhiramm7 all runners are in the green. Where are you seeing this error?

its it in GitHub Actions. If you go reg testing in Ubuntu or Mac and scroll to the end, you should see it there.

[michaeltryby]

@abhiramm7 good catch. Linux and Mac aren’t configured to save receipts

[bemcdonnell]

🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳