Skip to content

Comments

Update workflows admb docker#568

Merged
e-perl-NOAA merged 6 commits intomainfrom
update-workflows-admb-docker
Feb 15, 2024
Merged

Update workflows admb docker#568
e-perl-NOAA merged 6 commits intomainfrom
update-workflows-admb-docker

Conversation

@e-perl-NOAA
Copy link
Collaborator

@e-perl-NOAA e-perl-NOAA commented Feb 13, 2024

Concisely describe what has been changed/addressed in the pull request.

Update all workflows to use the admb docker image.

#563

What tests have been done?

Where are the relevant files?

  • No test files are required for this pull request. -->

What tests/review still need to be done?

See PR #552

Is there an input change for users to Stock Synthesis?

  • No, there was no input change. -->

Additional information (optional).

@e-perl-NOAA e-perl-NOAA added the workflows related to workflows/gh-actions label Feb 13, 2024
@e-perl-NOAA
Copy link
Collaborator Author

@Rick-Methot-NOAA please look at the new warning_ss_ref.txt file. There are some warnings produced by the admb docker image that we haven't seen prior but look like they might be easy fixes.

@e-perl-NOAA e-perl-NOAA marked this pull request as ready for review February 14, 2024 14:16
@Rick-Methot-NOAA
Copy link
Collaborator

I saw one warning regarding an unguarded "For". I will fix that.

Note that For statements that rely on indentation for interpretation are easily broken. I try to always use explicit {}.

Copy link
Contributor

@iantaylor-NOAA iantaylor-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have enough experience with yml or Docker to go through these changes line by line, but as long as the new approach is producing the test results that we expect, that's good enough for me.

You may want to just delete the big commented-out sections of code associated with the old way of doing things. I'm told that this is the best practice where you can rely on the git history to see the old way. However, if having those comments present in the code helps understand what the new code is doing, then feel free to leave them in.

@e-perl-NOAA
Copy link
Collaborator Author

@iantaylor-NOAA my thought process for retaining them is to have them easily accessible in case 1) the docker image stops working and 2) we need to test compiling locally across all/multiple OS' (in the case that admb no longer will compile SS3 locally and we need to see if it's a compiler issue or if we need to see if it will compile locally when we finally get updated to windows 11).

@Rick-Methot-NOAA
Copy link
Collaborator

Surely someone is already compiling with Win11. Is there significant risk?

@e-perl-NOAA
Copy link
Collaborator Author

I'm not sure at this point. I can delete the commented-out portion that downloads admb and puts it in the path for most of the workflows but leave it in one (probably the build-ss3) just to have easily accessible as a just in case measure, at least until we have a better understanding of what complications could arise?

@Rick-Methot-NOAA
Copy link
Collaborator

I think it makes sense to archive it in some readily retrievable form. The source code repo has lots of commits, so if we rely on git history to retrieve this, then we at least should have a label on that commit.

@iantaylor-NOAA
Copy link
Contributor

Given this background, I think it's fine to leave the commented-out code in the files for now. If we find that we haven't needed down the road they can always be deleted later.

@e-perl-NOAA
Copy link
Collaborator Author

Okay sounds good. I'll go ahead and merge then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

workflows related to workflows/gh-actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants