Skip to content

Add YAML linting and improve setup script#80

Merged
maddenp-cu merged 14 commits intoNOAA-EPIC:mainfrom
maddenp-cu:yamllint
Mar 5, 2026
Merged

Add YAML linting and improve setup script#80
maddenp-cu merged 14 commits intoNOAA-EPIC:mainfrom
maddenp-cu:yamllint

Conversation

@maddenp-cu
Copy link
Contributor

Description:

Fixes #78 by adding yamllint to the base conda environment, adding a make yamllint target, incorporating that target into the test target, and documenting this in the README.md.

Also, improve the setup script by installing both run and dev packages in a single command to reduce the amount of time spent by conda in resolving package sets; and make it easy to install dev packages on top of a non-dev set of environments, or to change package versions, without removing conda or its environments.

Type of change:

  • New feature
  • Refactor / cleanup
  • Documentation

Area(s) affected

  • uwtools-based software environment and Makefile targets under src/

Commit Requirements:

  • This PR addresses a relevant NOAA-EPIC/EAGLE issue (if not, create an issue); a person responsible for submitting the update has been assigned to the issue (link issue)
  • Fill out all sections of this template.
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have made corresponding changes to the system documentation if necessary

Testing / Verification:

  • I ran and/or verified the changes (or provided a test plan)

I ran the Quickstart routine from the README.md.

Runtime Environment:

As specified in the README.md Quickstart, on Ursa.

@@ -1,5 +1,7 @@
# This is the base EAGLE config. It currently configures the Nested EAGLE case.

# yamllint disable rule:anchors rule:line-length
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When final configs are created by composing multiple YAML files together, it may be that one config contains a YAML alias for which no corresponding anchor is apparently available -- but it will be available after composition.

EAGLE YAML configs may also have long lines.

@(set -x && shellcheck --format=gcc --severity=info --shell=bash $(BASHSRCS))

test: lint shellcheck typecheck
test: lint shellcheck typecheck yamllint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make test will now also run make yamllint, though the latter can also be requested independently.

src/setup Outdated
# invoking make targets requires make itself; and driver execution and config management require
# uwtools, so install those.
conda create -y -q -c ufs-community -n $* jq $MAKE $UWTOOLS
conda create -y -q -c ufs-community -n $* $UWTOOLS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the base environment needs jq, and make needs to be available outside the conda installation, as it is used to create the conda installation. A note has been added to the README.md about the make requirement. This should be present, or easily made available, on just about any system.

$UWTOOLS
yamllint=1.38.*
)
test -v EAGLE_DEV && pkgs+=( $(devpkgs) )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By conditionally appending the development packages to the list of packages to install, a single conda create or conda install command can be performed instead of two.

Comment on lines +89 to +92
if conda_env_exists $name; then
conda_install ${args[@]}
else
conda_create ${args[@]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this logic, if e.g. make env ... is run once to create a non-dev environment, make devenv ... can later be run and conda (and pip) will find almost all required packages already installed, and will only need to install the missing dev packages. Or, a user could change version numbers here for experimental reasons, re-run make env, and update the existing conda environments without deleting them or the conda installation itself. This could be a big time saver.

@maddenp-cu maddenp-cu marked this pull request as ready for review March 5, 2026 16:09
@maddenp-cu maddenp-cu requested a review from a team as a code owner March 5, 2026 16:09
Comment on lines +149 to +153
yamllint:
$(call activate,base)
@echo "=> Linting YAML configs"
@(set -x && yamllint --no-warnings config/)

Copy link
Contributor

@AnilKumar-NOAA AnilKumar-NOAA Mar 5, 2026

Choose a reason for hiding this comment

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

each recipe line runs in a separate shell. conda activate base line won’t affect the later yamllint. meaning yamllint will run in whatever environment Make happens to have.
possible suggestion
yamllint:
@echo "=> Linting YAML configs"
$(call activate,base) && (set -x && yamllint --no-warnings config/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .ONESHELL: directive higher in the Makefile ensures that all recipe lines run in the same shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. Thats great!

Comment on lines +142 to +143
python=3.13
xesmf=0.8.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: may be fine on Ursa if you’ve validated solver availability, but python 3.13 + compiled geo stack (ESMF / esmpy / xesmf) is a common point of failure on HPC channels.

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 haven't seen issues yet, but thanks for the heads-up.

src/setup Outdated
# invoking make targets requires make itself; and driver execution and config management require
# uwtools, so install those.
conda create -y -q -c ufs-community -n $* jq $MAKE $UWTOOLS
conda create -y -q -c ufs-community -n $* $UWTOOLS
Copy link
Contributor

Choose a reason for hiding this comment

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

conda_create to use -n "$name" with shift ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I think that would be clearer. I'll update and test. Thanks!

src/setup Outdated
# shellcheck disable=1090,1091,2046,2048,2068,2086,2206,2155
# shellcheck disable=1090,1091,2046,2048,2068,2086,2206,2207

set -aeu
Copy link
Contributor

@AnilKumar-NOAA AnilKumar-NOAA Mar 5, 2026

Choose a reason for hiding this comment

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

do we really need -a?
prefer set -euo pipefail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, -a ensures that the functions defined at the top level are available in subshells.

I'll test with -o pipefail and either add it or address any issues. I remember having problems with it in ternary expressions like stmt && then || else, but maybe it doesn't matter here.

Copy link
Contributor

@AnilKumar-NOAA AnilKumar-NOAA left a comment

Choose a reason for hiding this comment

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

looks good to me!

)
}

parse_kvargs $@
Copy link
Contributor

Choose a reason for hiding this comment

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

parse_kvargs should use "$@"?? (both in the call and in the loop).

Copy link
Contributor Author

@maddenp-cu maddenp-cu Mar 5, 2026

Choose a reason for hiding this comment

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

In the general case I agree, because the individual arguments in $@ could in theory contain spaces. But the only arguments allowed here are of the form key=value, with no spaces. In my opinion, having "$@" suggests to readers that we think that there might be spaces, which isn't true. I think using syntax choices to communicate intent is useful and so am reluctant to add more syntax where it isn't needed.

But I might be missing something -- please let me know if so!

Copy link
Contributor

Choose a reason for hiding this comment

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

Its fine - will see if any issues later.

@maddenp-cu maddenp-cu merged commit 879adc5 into NOAA-EPIC:main Mar 5, 2026
27 of 29 checks passed
@maddenp-cu maddenp-cu deleted the yamllint branch March 5, 2026 19:49
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.

Lint YAML src/config/*.yaml

3 participants