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

update CI image and add mkmf for fre make testing #245

Merged
merged 13 commits into from
Nov 8, 2024

Conversation

rem1776
Copy link
Contributor

@rem1776 rem1776 commented Nov 1, 2024

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.

  • Changes the CI image to a newly created one with gcc, mpich and anything else needed to test fre make capabilities more thoroughly. This replaces the 'stock' image we were using from github and has miniconda 24.7.1 and gcc 14
  • Adds mkmf as a git submodule
  • Adds a new test file that checks the run-fremake subcommand compiles the null model successfully using an added CI platform
  • Made changes to the workflow to run on the new image and cleaned up a few commands that don't seem necessary

Issue ticket number and link (if applicable)

#226
Fixes #229

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback

@ilaflott
Copy link
Member

ilaflott commented Nov 1, 2024

I'm not reviewing this yet, but i did want to bring your attention to my attempt to package fms_yaml_tools with gfdl_msd_schemas as a git submodule. If i'm not overlooking something, your approach will run into this

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 fms_yaml_tools a proper conda package for fre-cli will be MUCH easier. I.e. PLEASE prove me wrong about conda build / git submodule, i would really prefer to be wrong.

@rem1776
Copy link
Contributor Author

rem1776 commented Nov 4, 2024

I'm not reviewing this yet, but i did want to bring your attention to my attempt to package fms_yaml_tools with gfdl_msd_schemas as a git submodule. If i'm not overlooking something, your approach will run into this

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 fms_yaml_tools a proper conda package for fre-cli will be MUCH easier. I.e. PLEASE prove me wrong about conda build / git submodule, i would really prefer to be wrong.

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 conda env create might be part of the issue.

@ilaflott
Copy link
Member

ilaflott commented Nov 5, 2024

I'm not reviewing this yet, but i did want to bring your attention to my attempt to package fms_yaml_tools with gfdl_msd_schemas as a git submodule. If i'm not overlooking something, your approach will run into this
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 fms_yaml_tools a proper conda package for fre-cli will be MUCH easier. I.e. PLEASE prove me wrong about conda build / git submodule, i would really prefer to be wrong.

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 conda env create might be part of the issue.

RE git submodules: consider my anxieties salved!

@rem1776
Copy link
Contributor Author

rem1776 commented Nov 6, 2024

@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 conda build actually runs all the pytests as part of building the package. I guess thats standard for conda, but in this case it could prevent you from doing a conda build on other systems since the platform used in the test is specific to the CI image. Without editing the ci.gnu platform (and sometimes the mkmf template) for whichever system you are on the compile will probably fail during the test and ruin the build.

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 conda build for the sake of portability?

Any thoughts appreciated, I'm still a conda newb so lmk if i got something wrong.

@rem1776 rem1776 marked this pull request as ready for review November 6, 2024 16:46
@singhd789
Copy link
Collaborator

@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 conda build actually runs all the pytests as part of building the package. I guess thats standard for conda, but in this case it could prevent you from doing a conda build on other systems since the platform used in the test is specific to the CI image. Without editing the ci.gnu platform (and sometimes the mkmf template) for whichever system you are on the compile will probably fail during the test and ruin the build.

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 conda build for the sake of portability?

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 @pytest.mark.skip(reason='why we're skipping') that you can add right before a test

@rem1776
Copy link
Contributor Author

rem1776 commented Nov 6, 2024

@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 --ignore argument to the pytest command that can skip a directory, I think if we add it here it should skip it during the conda build but still include the test if you run pytest outside of the conda build command:

- pip install GitPython && pytest --config-file=fre/pytest.ini --cov-config=fre/coveragerc --cov=fre fre/

@ilaflott
Copy link
Member

ilaflott commented Nov 6, 2024

@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 --ignore argument to the pytest command that can skip a directory, I think if we add it here it should skip it during the conda build but still include the test if you run pytest outside of the conda build command:

- pip install GitPython && pytest --config-file=fre/pytest.ini --cov-config=fre/coveragerc --cov=fre fre/

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...

@ilaflott
Copy link
Member

ilaflott commented Nov 6, 2024

@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 conda build actually runs all the pytests as part of building the package.

this is actually a choice i made- if the tests can't run as part of conda build, i think we DO want the packaging step to fail.

I guess thats standard for conda, but in this case it could prevent you from doing a conda build on other systems since the platform used in the test is specific to the CI image.

I think ideally we only publish the conda package from here as a noplatform build. the presence of compilers etc. is something i think the code should check for and handle/

Without editing the ci.gnu platform (and sometimes the mkmf template) for whichever system you are on the compile will probably fail during the test and ruin the build.
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 conda build for the sake of portability?

i think this is fine for now

Any thoughts appreciated, I'm still a conda newb so lmk if i got something wrong.

yer doing great work

meta.yaml Show resolved Hide resolved
@@ -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
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Member

@ilaflott ilaflott Nov 8, 2024

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# 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"
Copy link
Collaborator

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!

.github/workflows/create_test_conda_env.yml Show resolved Hide resolved
.github/workflows/publish_conda.yml Show resolved Hide resolved
@singhd789
Copy link
Collaborator

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 HOME/fremake_canopy/test and I have a feeling tests might conflict or fail if files are getting overwritten on removed as the tests run. Something we should explore

@ilaflott
Copy link
Member

ilaflott commented Nov 8, 2024

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 HOME/fremake_canopy/test and I have a feeling tests might conflict or fail if files are getting overwritten on removed as the tests run. Something we should explore

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

@singhd789
Copy link
Collaborator

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 HOME/fremake_canopy/test and I have a feeling tests might conflict or fail if files are getting overwritten on removed as the tests run. Something we should explore

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 modelRoot that's defined in the platforms.yaml:

modelRoot: ${HOME}/fremake_canopy/test

So, it is set elsewhere, using HOME - similar to how cylc-src does I guess. In other test scripts, I reset HOME to an output location I wanted like os.environ["HOME"]=str(Path(f"{test_dir}/configure_yaml_out")), but Ryan brought up a good point of having to reset it at the end (or else the next test uses that HOME location) - similar to the chdir issues.

@singhd789 singhd789 merged commit 26e2093 into NOAA-GFDL:main Nov 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

mkmf not included in installation
3 participants