Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a dockerfile to build CAM-SIMA #222

Merged
merged 17 commits into from
Aug 3, 2023
Merged

Add a dockerfile to build CAM-SIMA #222

merged 17 commits into from
Aug 3, 2023

Conversation

K20shores
Copy link
Collaborator

@K20shores K20shores commented Jun 5, 2023

  • Adds a dockerfile to build an example case that uses CAM-SIMA
  • I based much of this work off of the ESCOMP-Containers repository, just as an FYI

This will allow the fine-grain testing of newly-added components to CAM-SIMA.

@K20shores
Copy link
Collaborator Author

This is in draft because CAM-SIMA doesn't run successfully; I get a SIGILL. I am trying to debug why

@K20shores K20shores changed the title again trying to make the branch play well with the main branch after … Add a dockerfile to build CAM-SIMA Jun 5, 2023
@K20shores K20shores marked this pull request as ready for review June 13, 2023 15:50
@K20shores
Copy link
Collaborator Author

@nusbaume If you follow the instructions in the readme for building first esmf, and then cam sima, you should be able to start the container and run ./case.submit and see a run of cam-sima happen in docker.

@nusbaume
Copy link
Collaborator

@K20shores It looks like your new test-musica PR (#225) contains many of the same container files. Does that PR supersede this one (and so we can go-ahead and close this), or did you still want this to come in separately?

@K20shores
Copy link
Collaborator Author

@nusbaume I branched off of this branch to do the work in #225. I suppose that specific work in #225 really only tests a build of CAM-SIMA with Musica. If you don't care to have a base image that doesn't include Musica, we can abandon this PR.

@nusbaume
Copy link
Collaborator

I think since the vast majority of our eventual model configurations will want some type of chemistry, and because it would be good to test even seemingly independent physics changes with MICM linked in to make sure we aren't breaking that link, I would probably vote to only have one container (at least for now) that includes Musica.

@mattldawson @peverwhee @mwaxmonsky @boulderdaze Would any of you disagree with this thought process? Happy to be over-ruled here.

@mattldawson
Copy link
Collaborator

mattldawson commented Jul 13, 2023

I think since the vast majority of our eventual model configurations will want some type of chemistry, and because it would be good to test even seemingly independent physics changes with MICM linked in to make sure we aren't breaking that link, I would probably vote to only have one container (at least for now) that includes Musica.

@mattldawson @peverwhee @mwaxmonsky @boulderdaze Would any of you disagree with this thought process? Happy to be over-ruled here.

Hi @nusbaume,

I have no strong feelings here, although if forced to choose, I would probably opt to be able to build CAM-SIMA images with and without MUSICA. Even if most of the time people will use MUSICA with CAM-SIMA, it could still be useful to be able to build and test them independently to ensure that they don't become too entangled. This could make it easier when someone wants to use a different chemistry (like GEOS-Chem). But, I'm fine with either option.

@nusbaume
Copy link
Collaborator

@mattldawson That's a good point, I hadn't thought about things like GEOS-Chem. I am fine to keep both containers then. I'll still plan on getting to the Test musica PR first as I imagine it is required for any future chemistry work, but will then put this PR as second on my to-do list, unless I hear otherwise. Thanks!

better language
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks good, just have a couple Dockerfile questions and then a request to download the input data from a server instead of including it with the repo itself.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks a lot better! I just have one last question/request.

Dockerfile Outdated
Comment on lines 82 to 83
RUN mkdir -p /home/cam_sima_user/cesm_data/inputdata/atm/cam/inic/homme/
RUN cp /home/cam_sima_user/cami-mam3_0000-01_ne5np4_L30.140707.nc /home/cam_sima_user/cesm_data/inputdata/atm/cam/inic/homme/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you ever determine if you need this file? If not then it might make sense to not bother with trying to download it, and to remove these two commands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed them and it ran. I pushed changes that removed this.

@K20shores K20shores mentioned this pull request Jul 31, 2023
@@ -0,0 +1,19 @@
#!/bin/sh

FTP_HOST="cesm-inputdata-lowres1.cgd.ucar.edu"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It this a good site to have the ftp data reside? Two concerns:

  1. Typically the CGD ftp site has an expiration data available on it. I'm not sure if that policy is for this site as well.
  2. With the threat of the removal of izumi in the future, I'm not sure it that will expand to wherever this site is located as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a temporary solution until we start using snapshot files with the ne3 grid that have been processed to be as small as possible, which we will then bring in using the standard inputdata server methods.

The reason we haven't done that already is simply because there isn't an agreed-upon plan (yet) for where to put test data on the inputdata server, and because these files are larger than they need to be and so we didn't want to add them to a place where they couldn't be deleted but would stop being used in a few months.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nusbaume or @briandobbins will better know how to answer this questions.

README.md Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks great to me now. Thanks!

@K20shores
Copy link
Collaborator Author

@cacraigucar I have added some installation instructions for docker desktop to the readme. Do they seem comprehensive enough to you?

README.md Show resolved Hide resolved
Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

one small typo; otherwise looks good (though I did not try to build the docker image as Cheryl has fallen on that sword)

README.md Outdated
cd docker
docker build -f Dockerfile.esmf -t esmf .
```
2. Build the CAM-SIMA iamge
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo - image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@mwaxmonsky
Copy link
Collaborator

@K20shores Is there anyway to verify the case ran successfully in the container beyond that it generated log without error?

@K20shores
Copy link
Collaborator Author

@K20shores Is there anyway to verify the case ran successfully in the container beyond that it generated log without error?

not that I'm aware of. @nusbaume would know better

README.md Show resolved Hide resolved
@nusbaume
Copy link
Collaborator

nusbaume commented Aug 3, 2023

@K20shores Feel free to merge this PR whenever you're ready. Thanks!

@K20shores K20shores merged commit 774f71d into development Aug 3, 2023
12 checks passed
@K20shores K20shores deleted the docker branch August 7, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

6 participants