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

Conda package attempt #75

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Conda package attempt #75

wants to merge 38 commits into from

Conversation

ilaflott
Copy link
Member

@ilaflott ilaflott commented Jan 22, 2025

this PR:
- adjust success condition of some tests, particularly those testing list_files

  • adjusts how t/t001-list_paths.sh does set-up, now creates t/src/file6.f90 symbolic link on-the-fly. removed at end of each of those tests
  • adjusts how every test does setup- checks PATH and adjusts only if needed. above file6.f90 link creation done on-the-fly for t/t003-mkmf.sh as well.
  • tests and package should work in the usual way with the usual level of functioning for all desired contexts
  • adds meta.yaml to define the conda build recipe
  • adds environment.yml defining fully functional environment
  • puts all relevant code under a nested mkmf "package" directory
  • adds a github workflow file to run conda build under github's CI/CD pipelines, conda build . --no-anaconda-upload version in .github/workflows/build_conda.yml, with uploading in .github/workflows/publish_conda.yml. run conditions identical to that of fre-cli

ilaflott and others added 2 commits January 22, 2025 17:05
…define build recipe in meta.yaml and relevant fields. define conda environment.
@ilaflott
Copy link
Member Author

green check, yes. note failures in pipeline...

…for test step. add git at dependency to see if it keeps the pipeline happy.
@ilaflott ilaflott marked this pull request as ready for review January 23, 2025 19:19
@ilaflott
Copy link
Member Author

ilaflott commented Jan 23, 2025

current testing state is odd.

In the pipeline, t/t001-list_paths.sh tests fail, tests #'s 1, 3, 5, 6, and 7 (see copied pipeline output). But, locally on my workstation, only the first test, 1 list_paths prints version fails.

+ bats t/t001-list_paths.sh
+ echo 'failure guard :-('

not ok 1 list_paths prints version
# (in test file t/t001-list_paths.sh, line 19)
#   `[[ "$output" =~ ^list_paths\ [0-9]+\.[0-9]+$ ]]' failed

not ok 3 list_paths using default out file
# (in test file t/t001-list_paths.sh, line 32)
#   `[ "$(wc -l < path_names)" -eq 6 ]' failed


not ok 5 list_paths find files in t and test_* directories
# (in test file t/t001-list_paths.sh, line 44)
#   `[ "$(wc -l < path_names)" -eq 8 ]' failed

not ok 6 list_paths find specific files in t or test_* directories
# (in test file t/t001-list_paths.sh, line 51)
#   `[ "$(wc -l < path_names)" -eq 7 ]' failed

not ok 7 list_paths with specified out file
# (in test file t/t001-list_paths.sh, line 59)
#   `[ "$(wc -l < $outFileName)" = "6" ]' failed

Additionally, in the pipeline, all t/t002-git-version-string.sh tests fail, and yet locally on my workstation, all of these tests pass. I suppose i'm not entirely surprised by this because i've seen github's CI/CD has been weird about calling git commands directly in the past.

+ bats t/t002-git-version-string.sh
+ echo 'failure guard :-('

…move most unneeded failure-guarding shell commands.
…nge approach to resolving binDir in t/t001-list_paths- should possibly keep the pipeline happier. include mkmf in source files for test step of conda build. more echos, switch spot of echo output to avoid adjusting status of last-called shell command.
…he testing approach makes perfect sense locally- so why isnt the pipeline happy?
…tween string and ints in test success condition checks
…slowing this pipeline down for its own good!! (maybe...)
@ilaflott
Copy link
Member Author

AHA! the failure in githhub's pipeline arises because the command there, for some reason, resolves the symbolic link file file6.linked in t/src to file file6.f90 with one of the targeted extensions.

a little more simply: locally, the test list_paths using default out file in t/t001-list_paths.sh should ignore a link and only find 6 files. In github CI/CD, the symbolic link is resolved and it finds 7 files resulting in failure.

@ilaflott
Copy link
Member Author

relatively sure that once conda build . enters the test phase, the source_files key specifying t/ likely copies the directory with a command ala cp -L -r t/ <target_dir>, and is de-referencing the link to file6.f90, changing the answers in the list_files tests. i adjusted the answers accordingly, but this is honestly bad practice.

the git based tests in t still do not work in the pipeline but work fine for me locally.

@ceblanton
Copy link
Contributor

It looks great, thanks!

The noaa-gfdl channel is not needed for building, is it?

What do you think about throwing in the noaa-gfdl anaconda channel key and having this pipeline publish as well as build. Too soon?

It's odd that the tests needing adjusting (but then again I hadn't realized there were mkmf tests. :)

@ilaflott
Copy link
Member Author

ilaflott commented Jan 23, 2025

The noaa-gfdl channel is not needed for building, is it?

nope!

What do you think about throwing in the noaa-gfdl anaconda channel key and having this pipeline publish as well as build. Too soon?

no we can do it- i'll maybe say it should stick with an alpha version tag for now... at most, beta

It's odd that the tests needing adjusting (but then again I hadn't realized there were mkmf tests. :)

100% conda build is derefencing links found in specified source files for the testing step! idk what the best way to deal with that is and not lose true quality control.

put conda-relevant things in README- adjust markdown, update dependencies discussed. put disclaimer+license at the bottom of the readme to emphasize usage, installation, featurres etc.
update badge
@ilaflott
Copy link
Member Author

This is ready.

The only thing i can think of is that to make the tests compatible w/ conda and the past-mode of usage is to adjust PATH accordingly after checking e.g. which mkmf success.

@ilaflott
Copy link
Member Author

All tests now pass without caveat in all contexts.

@ilaflott ilaflott requested a review from singhd789 January 24, 2025 16:35
Copy link
Contributor

@ceblanton ceblanton left a comment

Choose a reason for hiding this comment

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

Nice work, Ian. Bonus for the the documentation updates.

The templates still have to be installed somehow, right?

.github/workflows/build_conda.yml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
meta.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ilaflott
Copy link
Member Author

ilaflott commented Jan 24, 2025

Nice work, Ian. Bonus for the the documentation updates.

The templates still have to be installed somehow, right?

RE: these templates- mkmf needs you to specify a template target as an argument- i think. are these templates just supposed to be "found"? are they expected to be "nearby" ?

EDIT:
https://github.com/NOAA-GFDL/fre-cli/blob/a9730e039274739e5dc8710d82dcc09578735d18/fre/make/gfdlfremake/buildDocker.py#L31

@ceblanton
Copy link
Contributor

It's odd that mkmf includes the code and the templates for compiling FMS models on selected platforms, but they're been used together like this for 20 years.

I think it's a little more consistent to only conda install the mkmf and list-paths tools, but by tradition, we should probably include the templates in the conda package also.

CatalogBuilder does this.. I'm not sure how it works, but e.g. the templates are installed alongside the bin and lib files. @Ciheim could explain it. For proof though, the GFDL default template (gfdl_template.json) is installed in fre-cli:

c2b:/nbhome/fms/conda/envs/fre-test%>find . -name gfdl_template.json 

./lib/python3.9/site-packages/catalogbuilder/cats/gfdl_template.json

@ilaflott
Copy link
Member Author

It's odd that mkmf includes the code and the templates for compiling FMS models on selected platforms, but they're been used together like this for 20 years.

using shell to drive workflows for decades will incentivize this kind of approach. unfortunately, given the way conda packages function... do we really want to be doing things like this? targeting the templates/ dir within a conda environment i guess like:

mkmf -t /home/Ian.Laflotte/conda/envs/mkmf_test/......mkmf/templates

maybe we should consider de-coupling the templates and the code?

I think it's a little more consistent to only conda install the mkmf and list-paths tools, but by tradition, we should probably include the templates in the conda package also.

i'm sympathetic, i wanna figure it out.

CatalogBuilder does this.. I'm not sure how it works, but e.g. the templates are installed alongside the bin and lib files. @Ciheim could explain it. For proof though, the GFDL default template (gfdl_template.json) is installed in fre-cli:

c2b:/nbhome/fms/conda/envs/fre-test%>find . -name gfdl_template.json 

./lib/python3.9/site-packages/catalogbuilder/cats/gfdl_template.json

It's not quite 1-1: mkmf is not a python package and there is no pip install procedure here.

@ilaflott
Copy link
Member Author

catalogbuilder is using it's own pkg_dir property: https://github.com/search?q=repo%3ANOAA-GFDL%2FCatalogBuilder%20gfdl_template&type=code

i get the executables in the environment to $PREFIX/bin, because conda adds that to one's PATH for you. I could add a similar step where i cp mkmf/templates/* $PREFIX/lib or something. but fre-cli might have to do that pkg_dir path wrangling on it's own, and adjust the -t option, making the directory resolving fre make's problem to solve

@ilaflott
Copy link
Member Author

i think the ideal solution would be that
mkmf -t /absolute/or/relative/path/to/template.mk works as it does now, but e.g. specifying mkmf -t ncrc-gcc without the .mk extension says to mkmf "hey, get this template from your include dir".

@ceblanton
Copy link
Contributor

Yes!! I agree that would be a natural solution. I was thinking of a dummy list-templates tool to be added that reports the template directory but your idea is better.

@singhd789
Copy link

singhd789 commented Jan 24, 2025

Nice work, Ian. Bonus for the the documentation updates.
The templates still have to be installed somehow, right?

RE: these templates- mkmf needs you to specify a template target as an argument- i think. are these templates just supposed to be "found"? are they expected to be "nearby" ?

EDIT: https://github.com/NOAA-GFDL/fre-cli/blob/a9730e039274739e5dc8710d82dcc09578735d18/fre/make/gfdlfremake/buildDocker.py#L31

Pertaining to the templates for fre make functions, currently we have the path specified in the platforms.yaml:
https://github.com/NOAA-GFDL/fre-cli/blob/a9730e039274739e5dc8710d82dcc09578735d18/fre/make/tests/null_example/platforms.yaml#L6

Ryan mentioned this in another issue as well (in relation to mkmf being a submodule but still applies): NOAA-GFDL/fre-cli#303

If the template is in a set location in the conda package, that would pretty nice to refer to that location and just have the name of the template in the platforms.yaml (for fre make things at least)

@ilaflott
Copy link
Member Author

My main problem with me implementing that in this PR is that i do not know perl and am unsure if i'm the best person to implement that functionality on a short time-scale.

@ilaflott
Copy link
Member Author

Let's chat for a minute in-person on this when we can

@ilaflott
Copy link
Member Author

The templates should be a separate repository, as they're not really used by this repository out of necessity- they are a convenience for prospective users.

We're informing this view point with the following:

  1. all mkmf does with the value given to -t is stick the value as-is into the output Makefile, and the template is not read nor it's contents used programmatically by mkmf.
> mkmf -t FOO
. Makefile is ready.

> cat Makefile | grep TEMPLATE
MK_TEMPLATE = FOO
include $(MK_TEMPLATE)
  1. no one to our knowledge is relying on the templates in this repo as being in a particular spot directory-structure wise.

ilaflott and others added 4 commits January 28, 2025 11:42
slightly adjust current README instructions
test-run publish_conda workflow by asking it to run on push to `conda_package` branch, see what happens!
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