nmfs-ost / ss3-source-code

The source code for Stock Synthesis (SS3).
https://nmfs-ost.github.io/ss3-website/
Creative Commons Zero v1.0 Universal
36 stars 16 forks source link

Retry ADMB docker images #552

Closed johnoel closed 6 months ago

johnoel commented 7 months ago

Retry docker targets.

johnoel commented 7 months ago

Almost got it, just the windows build now

e-perl-NOAA commented 7 months ago

Great! The linux and mac docker images are working on my branch too and that was without these most recent changes so C++ issues might have been a merge conflict error resolution that messed the code up somewhere. For the windows admb docker image, I can't get it to run locally (on a windows machine) either. The error that I get when I do docker pull of the windows image is "image operating system "windows" cannot be used on this platform: operating system is not supported". I wonder if the windows executable can be/needs to be built off a linux image? 🤔

johnoel commented 7 months ago

Finally, windows builds are working under GitHub Actions. I will need to do a few more cleans, but it should be ready by next week.

Rick-Methot-NOAA commented 7 months ago

Congratulations. That seems a record number of commits as you diligently worked your way through the trial & error approach. Thank you.

e-perl-NOAA commented 7 months ago

Thank you @johnoel for diligently working on this for us! I'll do a bit more testing today and Tuesday just to make sure things work both on GitHub actions and locally with Docker.

johnoel commented 7 months ago

Okay, I clean up the script outputs a bit more. It's ready for user testing. The docker ADMB image files will be updated after you do the testing and provide me with feedback.

johnoel commented 7 months ago

Opps, wrong button

johnoel commented 7 months ago

Great! The linux and mac docker images are working on my branch too and that was without these most recent changes so C++ issues might have been a merge conflict error resolution that messed the code up somewhere. For the windows admb docker image, I can't get it to run locally (on a windows machine) either. The error that I get when I do docker pull of the windows image is "image operating system "windows" cannot be used on this platform: operating system is not supported". I wonder if the windows executable can be/needs to be built off a linux image? 🤔

What version of windows are you using?

Did you switch to Windows Containers?

See link...

e-perl-NOAA commented 7 months ago

Windows 10. That documentation was more helpful with the image they had there than other documentation that I found when trying to google the issue myself. Sadly, I might have to get IT involved because when I try to switch to a windows container I get an admin password prompt

johnoel commented 7 months ago

Congratulations. That seems a record number of commits as you diligently worked your way through the trial & error approach. Thank you.

Thank Rick, following the documentation did not help get it working on GitHub Actions. Trial and Error took the longest time, but was the only approach.

johnoel commented 7 months ago

Windows 10. That documentation was more helpful with the image they had there than other documentation that I found when trying to google the issue myself. Sadly, I might have to get IT involved because when I try to switch to a windows container I get an admin password prompt

Bummer, let me know when you get it switched and tested.

That’s why ADMB installation was changed. So it could be installed and used without administrator rights for modifying system environment on Windows.

e-perl-NOAA commented 7 months ago

I got IT to help with the permissions though I have to get them to enter their admin credentials every time I want to start docker using the windows image which is not ideal. However I get this message when I try to run it locally docker: a Windows version 10.0.20348-based image is incompatible with a 10.0.19045 host.

johnoel commented 7 months ago

Okay, I gotta find a windows 10 computer.

johnoel commented 7 months ago

Use johnoel/admb:windows-ltsc2019-winlibs for Windows 10 docker build.

Let me know if it works for you.

e-perl-NOAA commented 7 months ago

@johnoel that does one work locally 🎉. Unfortunate that it had to be built specifically for windows 10. I think I was hoping that a docker image would provide more of a solution to different operating systems & versions of operating systems than it does. With GitHub actions we can specify the operating system versions in the future if they stop working after a point, but locally it doesn't seem like there will be a solution. In the Make_SS_safe.bat and the _fast.bat files I added the following comment lines respectively. If you want to add them to your code for this PR then great, if not I can add them when I submit a PR to change all the rest of the workflows to use the docker image.

  @REM uncomment the commented out line to run the docker image for windows 10 locally
  @REM docker run --rm --mount source=%CD%,destination=C:\compile,type=bind --workdir C:\\compile johnoel/admb:windows-ltsc2019-winlibs ss.tpl

  @REM uncomment the commented out line to run the docker image for windows 10 locally
  @REM docker run --rm --mount source=%CD%,destination=C:\compile,type=bind --workdir C:\\compile johnoel/admb:windows-ltsc2019-winlibs ss_opt.tpl
e-perl-NOAA commented 7 months ago

@Rick-Methot-NOAA, there are a couple more warnings in the build_warnings github action with using the docker image than when it was built with downloading ADMB and putting it in the path. See this failed GitHub action run on my testing branch. The only reason it failed is because it has more warnings than the reference file so the reference file will have to be updated, but that will be done with some other things on a different branch and PR after this PR is merged.

johnoel commented 7 months ago

Take a look at the last three changes. It checks the Windows OS and figures out which docker image to use.

e-perl-NOAA commented 7 months ago

Okay, will have to test tomorrow morning because my computer restarted for updates which means I have to get in touch with IT to enter their admin credentials again to run docker for windows.

e-perl-NOAA commented 7 months ago

Was able to get IT to do what they need to do to run docker for windows (which requires a login from them every time my computer restarts) and I was able to get the updates you made to build the ss3 exe on my computer. It also builds and runs the Github actions on my admb-docker-attempt2 branch. Thank you so much with your help on this!

e-perl-NOAA commented 7 months ago

Adding @iantaylor-NOAA and @Rick-Methot-NOAA to the review for their awareness. One last test I would like to do that I only though of just now is making sure that when docker isn't installed, it looks for admb in the PATH. @iantaylor-NOAA or @Rick-Methot-NOAA, could one of you try downloading this branch and running the ./Make_SS_safe.bat file to make sure that it does this? I'm hesitant to close down docker since I have to get IT involved every time I need it opened/running and I think with docker running, it prefers to use that rather than admb in my path.

johnoel commented 6 months ago

All good?

e-perl-NOAA commented 6 months ago

@johnoel Yes, both Ian and Rick were out all last week.

@Rick-Methot-NOAA and @iantaylor-NOAA could one of you try downloading this branch and running the ./Make_SS_safe.bat file to make sure that it does this? I'm hesitant to close down docker since I have to get IT involved every time I need it opened/running and I think with docker running, it prefers to use that rather than admb in my path.

iantaylor-NOAA commented 6 months ago

@johnoel, I was not able to get your revised Make_SS_safe.bat file work on my computer without Docker installed. It looks like the line if not defined ADMB_HOME ( https://github.com/admb-project/ss3-source-code/blob/main/Compile/Make_SS_safe.bat#L28 is not getting interpreted correctly on my Windows 10 computer. It finds the admb.cmd, and defines an ADMB_HOME variable (which I already have in my environmental variables anyway), but still enters that section and looks for docker (which I don't yet have installed).

There are lots of other options, such as having a different .bat file for the docker vs plain ADMB installations, but if you know of a fix to make the single file work for both situations, that would be even better.

johnoel commented 6 months ago

Redid the batch files (see changes in dd1ddaf).

The scripts will attempt to build the tpl using the steps below:

  1. Use ADMB_HOME if defined, else goto next step
  2. Use admb.cmd in PATH if found, else goto next step
  3. Use docker if installed, else exit with error

Please test and let me know.

iantaylor-NOAA commented 6 months ago

@johnoel, your Friday fix to the batch file works for me now (without Docker). Thank you!

e-perl-NOAA commented 6 months ago

@johnoel are you still making tweaks are is this ready to be merged now?

johnoel commented 6 months ago

Should be okay to merge, just finding workarounds with GitHub Action.

e-perl-NOAA commented 6 months ago

Okay, thank you @johnoel. I will merge soon. We are working on an issue with a test model that we have that will disrupt me updating the other github actions/workflows to use the admb docker image. I might go ahead and merge your branch and make the update to some of the workflows or, depending on how long it will take us to get that test model fixed, I will wait to merge this until it's fixed and I can update all the workflows immediately following the merge.

e-perl-NOAA commented 5 months ago

@msupernaw