urbanopt / geojson-modelica-translator

GeoJSON to Modelica Translator that is focused on district energy system design and analysis.
https://www.nrel.gov/buildings/urbanopt.html
Other
18 stars 11 forks source link

fix(modelica_runner): Replace shell script with call directly to docker #606

Closed chriswmackey closed 8 months ago

chriswmackey commented 8 months ago

Resolves https://github.com/urbanopt/geojson-modelica-translator/issues/598

This PR essentially replaces the om_docker.sh script with a direct call to docker run, thereby enabling the docker simulation to work on Windows (where shell scripts are not executable) just as it does on unix-based systems.

Using poetry I have verified that the implementation in this PR succeeds for practically all tests on my machine. 119 tests passed on my machine and the 9 that seem to have failed all appear to be Dymola, which would make sense since I do not have Dymola installed.

image

The only thing that needs to be done for someone to be able to run all of the tests on their end is that a new docker image generated from the updated Dockerfile has to be pushed to nrel/gmt-om-runner on dockerhub. This will ensure that there's an image on dockerhub that has the MBL installed.

Alternatively, if you want to run some tests before the the new image is pushed to Dockerhub, just build the image yourself by running docker build on the Dockerfile in this PR. Then replace this line of code in the GMT with the ID of your locally-built image. This is how I verified that all of the tests are passing for me.

CC: @vtnate

vtnate commented 8 months ago

Holy macaroons @chriswmackey this is glorious! 💯

This is very exciting!

vtnate commented 8 months ago

I merged a PR into develop since you branched, which makes a bunch of tests better. Can you merge the latest gmt-develop into your branch too?

chriswmackey commented 8 months ago

Hey @vtnate . I think I successfully rebased everything such that there are no merge conflicts. The only thing I was not sure about is that one of the other commits that had been merged in the last few days added this cmake dependency into the docker image:

https://github.com/urbanopt/geojson-modelica-translator/pull/606/files#diff-6b42c56f0c9822944bb23e2929be1c8e09fb50f6079fec8171e9915944957a06R8

I have removed it in my PR since it wasn't necessary for all of the tests I was running but just let you know if you need me to add it back.

vtnate commented 8 months ago

Fascinating! Yeah, our runs were breaking without cmake, but if this new way of building it doesn't need it, great!

One final request: can you update the docker base image to pull from v1.22.1 instead of v1.22.0?

I wonder why our CI isn't picking this up and running? Maybe we should merge it into a branch first, and then I can try merging that branch into develop?

chriswmackey commented 8 months ago

Actually, I realized that all of those tests I had run were using the old OM docker image as a base. So I just put back the cmake since I think you may be right that it's needed to work with this version of OM. I also changed the base image to pull from v1.22.1.

I'm re-running the tests locally on my machine now.

Just let me know if you want me to send this PR elsewhere into a different branch.

vtnate commented 8 months ago

I just made a new-docker branch, can you set that as the base branch for this PR?

vtnate commented 8 months ago

Never mind, I can change the base branch