-
Notifications
You must be signed in to change notification settings - Fork 11
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
update CI image and add mkmf for fre make testing #245
Conversation
I'm not reviewing this yet, but i did want to bring your attention to my attempt to package TBH, i don't think i exhausted all possibilities/workarounds, but from my googling at the time, the situation doesn't look fun. I would like to see you succeed with this, because then making |
I haven't gotten that error in the CI testing or when I ran it in the container manually. Did you try starting off that CI with the recursive checkout option like i did for this one? I think pulling in the submodule after the |
RE git submodules: consider my anxieties salved! |
@ilaflott @singhd789 @kiihne-noaa This is ready now but wanted to bring up a potential side-effect of adding the run-fremake test. I didn't know this initially but it looks like I'm sure there's some way to exclude a specific test from pytest, so maybe it would be good to exclude compilation related tests during Any thoughts appreciated, I'm still a conda newb so lmk if i got something wrong. |
@rem1776 Ooooh right, fre make is kind of specific to gaea for bare-metal builds. That's interesting. I didn't think about that, thank you for bringing it up. There is this: to skip certain tests in a test script, you can add |
@singhd789 Hmmm I'm not sure if we should add that cause then it'd be skipped every time and there'd be no point to having it. It looks like there's an Line 66 in 20e943a
|
we have a few situations where tests aren't going to run everywhere right away. IMO i'd rather have a skip on a test that works somewhere specific, than no test at all. there's a lot of iteration on these things happening across the board... |
this is actually a choice i made- if the tests can't run as part of
I think ideally we only publish the conda package from here as a
i think this is fine for now
yer doing great work |
@@ -7,12 +7,15 @@ jobs: | |||
build: | |||
runs-on: ubuntu-latest | |||
container: | |||
image: continuumio/miniconda3:latest | |||
image: ghcr.io/noaa-gfdl/fre-cli:miniconda24.7.1_gcc14.2.0 |
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.
Clarifying question for myself, is this a version/image we'll have to update every once in a while?
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 don't think anything will break anytime soon if we used this image for a while, but it'd be good to get a new image with newer software every year or so, that's what I've typically done with other CI images. Anytime you rebuild the image it'll pull in the latest versions, so you wouldn't have to make any changes to the dockerfile.
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.
out of curiosity and my own notekeeping, where can one find the dockerfile for the uploaded container image in question?
edit: this isn't a condition for approval/merge/etc, i just like knowing stufff cause its fun
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.
It's in the hpc-me repo:
https://gitlab.gfdl.noaa.gov/fre/HPC-ME/-/blob/main/ci/fre-cli-gnu/Dockerfile?ref_type=heads
# add spack installed binaries to front of path so that | ||
# conda's netcdf/hdf5 installs don't break compilation tests | ||
export path_save=$PATH | ||
export PATH="/opt/views/view/bin:$PATH" |
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.
Tom just walked me through views the other day, cool to see!
The only thing we might have to discuss is the output location for fre make tests. The bare-metal runs all have the same output location of |
is the output location hard coded? it should, in theory, be something that can be set elsewhere and fed through as an e.g. an argument |
checkout files, the makefile, compile script, etc, use
So, it is set elsewhere, using |
Describe your changes
This PR updates the existing CI workflow that runs pytest to support compilation of models using fre make, as well as container creation.
run-fremake
subcommand compiles the null model successfully using an added CI platformIssue ticket number and link (if applicable)
#226
Fixes #229
Checklist before requesting a review