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

Temsim container #9

Merged
merged 5 commits into from
Nov 9, 2021
Merged

Temsim container #9

merged 5 commits into from
Nov 9, 2021

Conversation

fredericpoitevin
Copy link
Member

Attempt at setting up CI for containerization, specifically a TEM-simulator container. See #7 (comment) for more details.

@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #9 (e7ab82e) into master (9f392bd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #9   +/-   ##
=======================================
  Coverage   95.84%   95.84%           
=======================================
  Files           3        3           
  Lines          72       72           
=======================================
  Hits           69       69           
  Misses          3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f392bd...e7ab82e. Read the comment docs.

Copy link
Contributor

@geoffwoollard geoffwoollard left a comment

Choose a reason for hiding this comment

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

I took a look. Not sure how this all works, but it seems you know what you're doing.

One of the tests failed, but that's just a numerical rounding / tolerance issue from a test I wrote. It passed in the other Python versions. I've seen this before and there is nothing to worry about.

@fredericpoitevin
Copy link
Member Author

fredericpoitevin commented Oct 29, 2021

I took a look. Not sure how this all works, but it seems you know what you're doing.

What parts are confusing? We could use this as an opportunity for me to explain?

One of the tests failed, but that's just a numerical rounding / tolerance issue from a test I wrote. It passed in the other Python versions. I've seen this before and there is nothing to worry about.

Hmmmm OK maybe we could improve the test?

@ninamiolane
Copy link
Contributor

Yes, in this case the test should be adapted/improved, we cannot merge the PR if one of the tests fails :/ The tolerance for the test could be lowered?

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Nice!! So this runs a docker image by pulling the already built docker image from the cloud, and creating a container from this image?

It will be interesting to see how we can adapt our CI github workflow to run tests in this container. With this mindset, maybe this command lines should be done directly in the CI github workflow actually, otherwise the container will be created here and independently from the CIs, and the CI/unit-tests will not be able to see it? What did you have in mind for this?

@fredericpoitevin
Copy link
Member Author

fredericpoitevin commented Oct 29, 2021

Nice!! So this runs a docker image by pulling the already built docker image from the cloud, and creating a container from this image?

So this only builds a container and pushes it to DockerHub. The resulting image contains FFTW3 and TEM-simulator compiled. At this point, it is ready to pull from DockerHub and run wherever we see fit (as long as it's Linux).

It will be interesting to see how we can adapt our CI github workflow to run tests in this container. With this mindset, maybe this command lines should be done directly in the CI github workflow actually, otherwise the container will be created here and independently from the CIs, and the CI/unit-tests will not be able to see it? What did you have in mind for this?

But yes definitely! The next thing to figure out is how to run tests in the newly built container directly. If things run there, they are guaranteed to run anywhere the image will be. I'll try to see if I can set it up tomorrow

@ninamiolane
Copy link
Contributor

@fredericpoitevin thank you for the details, I see!

Do we need to build and push to dockerhub everytime someone creates a PR? Maybe we can change the on field from the github workflow, so that a new image is built and push only if the dockerfile has been modified, so that it does not slow down everybody's PRs?

NIT: maybe the workflow could be called tem_dockerbuild.yml since it is not simulating anything but rather building the image?

@fredericpoitevin
Copy link
Member Author

Do we need to build and push to dockerhub everytime someone creates a PR? Maybe we can change the on field from the github workflow, so that a new image is built and push only if the dockerfile has been modified, so that it does not slow down everybody's PRs?

Good idea! I'll see how to do that.

NIT: maybe the workflow could be called tem_dockerbuild.yml since it is not simulating anything but rather building the image?

Good point! Let's not merge until this is perfect :D

@ninamiolane
Copy link
Contributor

👮‍♀️ 😄 hehehe

@geoffwoollard
Copy link
Contributor

Ok I think I see from Nina's comments that this is doing some "installation stuff" on the cloud, and then making executable code available on the computer its running on.

Rather than installing tem simulator from scratch on whatever computer the repo is cloned to.

On what machines do the paths in docker/TEM-simulator/Dockerfile refer to? Like /work?

…triggered when the corresponding Dockerfile changes.
@fredericpoitevin
Copy link
Member Author

Do we need to build and push to dockerhub everytime someone creates a PR? Maybe we can change the on field from the github workflow, so that a new image is built and push only if the dockerfile has been modified, so that it does not slow down everybody's PRs?

Good idea! I'll see how to do that.

NIT: maybe the workflow could be called tem_dockerbuild.yml since it is not simulating anything but rather building the image?

Good point! Let's not merge until this is perfect :D

@ninamiolane - I believe this is better now.

@fredericpoitevin
Copy link
Member Author

Ok I think I see from Nina's comments that this is doing some "installation stuff" on the cloud, and then making executable code available on the computer its running on.

Rather than installing tem simulator from scratch on whatever computer the repo is cloned to.

On what machines do the paths in docker/TEM-simulator/Dockerfile refer to? Like /work?

So /work is a path "in the container". It sounds very abstract right now, but with a little bit of practice things get much clearer.

At this point, we have "built" and "pushed" an "image" to the "cloud". For you to use it, the next steps are to "pull" it and "run" it as a container. If you have a Linux machine handy, you could install Singularity and do the following:

singularity pull docker://fpoitevi/simspi-temsimulator:latest

This would "pull" the Docker image from the cloud and convert it to a Singularity image: a simspi-temsimulator_latest.sif file. You are now equipped with an image that you can run as a container - what does this mean? It means you can "execute" the container and work in it as if you were accessing a different computer.

Say you do this in a Terminal window on your Linux machine:

singularity exec simspi-temsimulator_latest.sif /bin/bash

This will just appear as a regular Terminal bash line (starting with Singularity>) but now you can run TEM simulator since you are in the container where it is installed.

@geoffwoollard
Copy link
Contributor

I did some work on the tests, and merged into main #12

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Looking good, thank you and sorry for the delay in reviewing this!

As a side note, @fredericpoitevin , could you maybe add something in the README on how to use this image?

@ninamiolane ninamiolane merged commit b2abb41 into master Nov 9, 2021
@fredericpoitevin fredericpoitevin deleted the temsim-container branch November 10, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants