-
Notifications
You must be signed in to change notification settings - Fork 23
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
Support Running AtChem2 in a docker container #518
Conversation
HI @willdrysdale @bsn502, this looks like a good idea. I confess I am not very familiar with Docker (beyond the general concepts) so I'll have to read about it a bit more before leaving my comments here. Is the idea to have the user build their own image using the docker file provided or to create pre-built images that one can simply download? |
I see it that you would create a pre-built image that others download. It would mean that the user's workflow looks something like:
This way you are using dockers benefit of preventing "it works on my machine" issues as much as possible. If the user rebuild's the image there is a chance they change something or install a different version of a dependency. (not to mention the ease of installation). With some input who knows a bit more about Linux package repos than me, I suspect the Dockerfile could specify versions of the dependencies to make this even more robust. It is possible using Github actions to have these containers generated automatically like the CI runs, (I don't have much experience setting these up however) - but it should be the case that the image could appear alongside the releases, so one could either install the traditional way from the Keen to hear your thoughts when you've had a read :) - the image I've linked about should be working if you want to give it a test. |
I understand how this works, and I can see there are several docker related apps in the github marketplace so I think it should be possible to automatically generate the image, once a new release is created (I probably wouldn't do it for the current master branch, though, as it can be in a state of flux at times). But I suppose one can also generate the image and uploaded it manually, like you have done on the wacl repo. The documentation should probably go to the manual rather than the README file, but you can leave it there for the time being as I am (very slowly) updating the manual anyway so it is easier if I do it myself. Any reason the Also, is the |
You should also add copyright notices to all the new files:
|
but could equally be:
I'll add a commit for this shortly.
|
user now provides folder containing mechanism at runtime, rather than this being hardcoded to `mcm`
Yeah all right. If you can add some more comments to the shell scripts to explain better what they do, it would be great (see the installation scripts for examples). I have started a review with some comments and questions, you should be able to reply directly under each one. |
Sounds good - where should I find these review questions? |
In the "Files Changed" tab of this PR :) |
sed -i 's,openlibm-0.8.1,/atchem-lib/openlibm-0.8.1,g' ./Makefile | ||
|
||
# Fix python command to match installed version | ||
sed -i "s/python/python3/g" ./build/build_atchem2.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be necessary. Did you have issues with the python version or is it just for extra caution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, when I was installing python via dnf install -y ... python3.11
the command to run python
was instead python3
so the build_atchem2.sh
script couldn't run the python commands. There might be a better way to go about this though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think perhaps some distros do not bind python
and require a specific reference to the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe - I don't really know enough about the various distros - could be a reason to swap out rockylinux if thats the case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it like this and when it comes to implement the github action we'll figure it out. It doesn't matter to the final user anyway, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The particular distro shouldn't affect the end user, but as it is linux if you wanted to run this container on a Windows machine you do need the Windows Subsystem for Linux installed I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can put a note about that in the manual.
Try now (my mistake!) |
I've finished changing things for the moment, comments have been added to the files and I've streamlined the installation script a fair bit. Now there are no hardcoded versions, as it copies the state of the branch into the container. The version can be included at build time, for example when I pushed to GHCR I used these:
You could then update the tag at the end with the release version |
So let me see if I get it. Once all this is done, the commands : A user would do |
That's essentially it!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #518 +/- ##
=======================================
Coverage 52.05% 52.05%
=======================================
Files 17 17
Lines 2096 2096
Branches 166 166
=======================================
Hits 1091 1091
Misses 933 933
Partials 72 72 ☔ View full report in Codecov by Sentry. |
It seems the CI action is not triggered correctly. Could you modify line 66 of |
Can you confirm - line 66 currently reads:
Do you want it changed to:
? |
Apologies, my mistake. Change to :
|
I think the issue is the version of macos11 which is no longer supported. |
Ah, probably should also uncomment line 50 :) |
Ahh yes, I just read that as a normal comment for some reason! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I don't have any other comments, but I will ask your input about this when I update the manual. I can leave the PR open, if you have other changes to make. Just let me know when you are ready to merge :)
Fantastic - more than happy to help when it comes to the manual update. @bsn502 has some models running now on the York HPC using the container - lets allow them to complete as a final test, should be done by tomorrow. So long as they work I think we are good to merge. Then do you want to look at storing the image on this repo (manually for now)? |
…ites existing outputs as expected
Hi @rs028! We just made one last change to make sure the model outputs are copied out of the the container consistently, even if the model is being re-run. I think we are happy to merge when you are :) |
Great. I think for the moment we can keep the image on the wacl page, if that is not a problem, while I figure out how to do it here. |
Sounds good to me - All the info in the readme currently points there, so everything should work for now - we can update that when the package is added here. |
Hi!
@bsn502 and I have been working on getting
AtChem2
running in a Docker (and Singularity) container to make it easier to get up and running with the model. If you are interested, this is something that we could provide to the main repo.The container comes with the
cvode
andopenlibm
dependencies installed (along with gcc-fortran, python3.11 and cmake), and should be ready to run out of the box wherever you run your containers. We have tried it out locally and on UoY's HPC successfully. I've updated the repo's README.md with some information on how to get it running.Right now the image is built off of release v1.2.2 but I imagine with GitHub actions an image per release + a latest based on the current state of the master branch could be provided.
I've used GHCR to store the image, and as such the docs all relate the the version over on wacl-york/AtChem2 so it can be tested out. We can update this down the line.
Let us know any thoughts and if you'd be happy to include this in the repo
Cheers,
Will