feature: adding demo container and automated builds for mfem #2981
Conversation
as discussed, this is a simple contain 8000 er build to provide mfem in spack, and then show example of running examples and miniapps. This is paired with an automated build so we always have an updated container base. This container setup can be extended to include other kinds of builds, or other use cases Signed-off-by: vsoch <vsoch@users.noreply.github.com>
|
Thanks so much for doing this @vsoch! Please mark with the |
|
okay ready for review (and labeled). Note that the container build appears to have successfully built, and I'm not sure why it's lingering in the yellow "in progress" state - hopefully not a GitHub bug! But it's no worries because we have some items for discussion: Container build timeThe container build is not quick, and I went off to have dinner when it was building locally and didn't realize it took so long! I don't think it should be triggered on PR, but rather just on a nightly basis. So my suggestion would be to remove the PR trigger (and we can confirm the build was successful here) and then have it build and deploy just on a scheduled nightly task and releases. The drawback is that you don't have a test for the container build on every PR, but I think I have an approach (discussed next) that can account for that. A dependency base container?We could actually have our cake and it it too by having a base container (nightly built) done that does this long build, and then a quicker testing container that runs on PRs that adds the new code and builds on top of it, so it should just need the time at the end to build mfem (but not the dependencies). If you are interested in this approach I can follow up with a second PR - I'll reorganize to have a docker/ directory with the Dockerfiles there, and a second Dockerfile.test that uses the base container. A Production Container?Since we keep all the source code around, this container is bigger rather than smaller. I can also suggest we follow up with a multistage build that removes everything but the spack view (which some app developer would want to develop) to create a much smaller base. If you like this idea I can follow up with another PR for it, with the testing idea if you like that too :) This means in summary we will have:
Changelog Update?I didn't update the CHANGELOG because the change isn't related to anything in mfem proper, but let me know if this is appropriate to add and you would like a note added. And just a comment, your CONTRIBUTING guide is impressive! I've never seen one so detailed, it almost made me nervous to mark this ready and request review. 😆 I apologize in advance if I missed a detail! Let me know your thoughts and I'll update the PR! |
|
This PR is now under review (see the table in the PR description). To help with the review process, please do not force push to the branch. |
Dockerfile
Outdated
| spack install sw4lite && \ | ||
| apt-get install -y libcurl4-openssl-dev libssl-dev && \ | ||
| spack install liblas lapackpp && \ | ||
| git clone --depth 1 https://github.com/geodynamics/sw4 /code |
There was a problem hiding this comment.
This looks like an artifact. sw4 is not a dependency or the like.
There was a problem hiding this comment.
Yep an oversight (and I will remove) - I started from another Dockerfile I was working on.
Let me know about the things I proposed above and when I am allowed to push changes.
There was a problem hiding this comment.
Adding commits is fine at any time, we just prefer not to have force-pushes, e.g. from rebase, hence the automated message: #2981 (comment).
There was a problem hiding this comment.
Gotcha! I'll push changes (without force) as they are requested.
There was a problem hiding this comment.
okay that makes it much faster (53 minutes!) I'm still confused why the action doesn't end at the end - it's either cleaning something up or a legit GitHub bug. I can open a ticket if it doesn't seem to ever end, I've never seen this before.
There was a problem hiding this comment.
@jandrej your emoji game is on point 👉 ✏️
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
|
Why are the tests so slow? Just lack of GitHub runners? Are there runners somewhere else? 🤔 |
Are you referring to the tests like Those don't run for PRs from forks |
|
oh gotcha! Yes I was looking at these 4 where it says "expected waiting for status to be reported" and wondering what's up. If you are doing these in GH-actions, you can do something like: if (github.repository_owner == 'mfem')to only run the check given it's not a fork, that way it won't output the confusing checks above! |
Hi @vsoch, Here is why this is happening. I agree it is not optimal... maybe you can help us fix it 🤔 Most of our GitHub CI tests are run on changes to PRs. They take a while and perform relatively extensive "smoke tests": see builds-and-tests (which runs 10 serial/parallel, debug/optimized, mac/linux builds) and build-analysis (which checks for compliance with our However, there are quick tests that we will like to run as soon as possible for things like code styling or accidentally committing big files. We'd like this to be run these on each push, to each development branch in the repo (with or without PR). This is done in the repo-check script, which is setup in GH-actions as follows: This all works fine for branches inside the main mfem repo (the workflow we encourage), but what happens for PRs from forked branches is that Does this make sense? Any suggestions? |
|
hey @tzanio ! So I took a look, and it looks like the workflow runs because the trigger is a general "push" which of course will hit the forked repo before here. For the contributor, this means the only way it would run here is if they disable actions on their repository, which might be reasonable for long term contributors, but for a first-time / one-time contributor, it's going to be confusing (as it was for me!). I looked into creative options - like can we have a step in the workflow that detects it's being run on the fork, and then sends a comment here to disable? Or better, to be able to disable the action from running on the fork? These API endpoints don't exist yet I don't think, and also that seems like an overly complex solution for a simple problem. So I looked at your workflows, and I think this is my suggested solution: https://github.com/mfem/mfem/pull/2987/files#diff-b34025e1f15d85c58cfbcffcdf592e3f07c69ba75065de8c2ee1b6059cd6aec1. The underlying motivation for this check is to make sure that the repository adheres to good practices (docs, code style) and I would argue this should not run on every push, but rather be scoped to pushes to the master / next branches, and for pull requests. This means that it will reliably be run here (in the PR) and not on the user's fork, where nobody is likely to see it. |
Co-authored-by: Tzanio Kolev <tzanio@llnl.gov>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
|
Merged in |
|
🤞 |
| We provide a [Dockerfile](Dockerfile) to build an ubuntu base image. You can use | ||
| this image for a demo of using mfem! 🎉️ | ||
|
|
||
| Updated containers are built and deployed on merges to the main branch and releases. |
There was a problem hiding this comment.
Mention here also how to trigger a manual build in CI?
README.md
Outdated
| MFEM is a modular parallel C++ library for finite element methods. Its goal is | ||
| to enable high-performance scalable finite element discretization research and | ||
| application development on a wide variety of platforms, ranging from laptops to | ||
| supercomputers. | ||
|
|
||
| We welcome contributions and feedback from the community. Please see the file | ||
| CONTRIBUTING.md for additional details about our development process. | ||
| [CONTRIBUTING.md](CONTRIBUTING.md) for additional details about our development process. | ||
|
|
||
| * For building instructions, see the file INSTALL, or type "make help". |
There was a problem hiding this comment.
Link INSTALL, LICENSE and NOTICE, e.g.
| * For building instructions, see the file INSTALL, or type "make help". | |
| * For building instructions, see the file [INSTALL](INSTALL), or type "make help". |
README.md
Outdated
| @@ -22,6 +25,8 @@ CONTRIBUTING.md for additional details about our development process. | |||
| * The best starting point for new users interested in MFEM's features is to | |||
| review the examples and miniapps at https://mfem.org/examples. | |||
|
|
|||
| * Instructions for learning with docker are [here](config/docker) | |||
There was a problem hiding this comment.
| * Instructions for learning with docker are [here](config/docker) | |
| * Instructions for learning with Docker are in [config/docker](config/docker). |
|
Can you give merge Alternatively, can you give me permission to push to the branch? |
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
|
okay updated! @tzanio I would have given you permission too but I don't see that little checkbox anywhere - I'm not totally keen on how that works because sometimes I see it and other times I do not. |
|
Thanks @vsoch |
|
Re-merged in |
According to the documentation you may be seeing |
Co-authored-by: Tzanio Kolev <tzanio@llnl.gov>
|
Thank you @vsoch 🙏 🚀 |
|
@tzanio do you plan to make the package public? https://github.com/mfem/mfem/pkgs/container/mfem-ubuntu-base. And if not already, it should be associated with the repo here so it shows up in the sidebar (and can be seen to be associated!) |
|
Hi @vsoch, I made the package visible and enabled the option for packages to show up in the sidebar. Does everything look okay now? |
|
And by "visible" I meant public. |
|
yes, looks perfect, thank you! I'll start on the other containers we discussed. |
As discussed in LLNL/radiuss#21 (comment), this is a simple container build to provide mfem in spack, and then show example of running examples and miniapps. I've added a new README-DOCKER.md with full instructions for this! This is paired with an automated build so we always have an updated container base. This container setup can be extended to include other kinds of builds, or other use cases. In terms of the more advanced use cases, if we have a base container that matches in rse-ops we could easily just extend the base container to the current matrix and build fairly easily (but I haven't tested yet - it's much better to start out simpler and grow based on request!)
I haven't tested this CI workflow yet, so expect that I'll tweak it until it's working.
Signed-off-by: vsoch vsoch@users.noreply.github.com
PR Checklist
make style.CHANGELOG?.githubif necessary