Recipe parametrisation #337
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
jfrost-mo
force-pushed
the
296_recipe_parameterisation
branch
from
699a8f7
to
469ab4d
Compare
|
|
jfrost-mo
force-pushed
the
296_recipe_parameterisation
branch
3 times, most recently
from
586ff31
to
7b1bc09
Compare
This is needed to fix the coverage failures detailed at nedbat/coveragepy#512 We may instead need to have this listed somewhere else however, as if pytest can't pick up the pyproject.toml it can't read the line telling it to use it.
This also fixes cset cookbook, that has been unknowingly broken when not installed from source. This was due to it not including the recipes in the package.
Also rearranged the cookbook CLI arguments. Specifically test for None Previously any falsey value could have unexpectedly worked.
This makes order irrelevant, and means they match the other arguments. Make input and output dirs required for bake Also improve a docstring Exist non-zero when unhandled arguments are given Add required options to bake test
These are variables that can be put into a recipe and are replaced at runtime with values specified as command line arguments. In a recipe file they are any string value of the form `$VARIABLE`, where the variable name can consist of A-Z and _. The runtime value is given to cset bake in the form `--VARIABLE=value`. Add fix variable handling and add tests Test for missing variable value Add documentation for recipe variables
This comment was marked as outdated.
This comment was marked as outdated.
As we use conda the pip update doesn't do anything. We use our own autoupdate system for our conda lock files.
jfrost-mo
changed the title
jfrost-mo
changed the title
jfrost-mo
force-pushed
the
296_recipe_parameterisation
branch
from
9985745
to
6160974
Compare
I'd fixed the same thing in two places independently, causing conflicts.
|
@JorgeBornemann I am holding off with the merge request until you had a chance to do the technical/portability review. |
jfrost-mo
added
the
full_review
There was a problem hiding this comment.
Sorry for the delay. The changes look good, but I have not been able to test it fully, as I have been having troubles with my conda environments.
I have not seen in the changes anything that can prevent portability, and as this review is blocking other developments I am happy to approve and will create a separate issue to address portability problems that may surface.
| subprocess.run(("cset", "-v", "cookbook", cset_recipe), check=True) | ||
| else: | ||
| cset_recipe = Path("recipe.yaml") | ||
| cset_recipe.write_bytes(os.getenvb("CSET_RECIPE")) |
There was a problem hiding this comment.
In our installation getenvb causes a type error, at the end of the traceback, in _check_bytes (line 781 of os.py) bails with TypeError("bytes expected, not %s" % type(value).__name__)
There was a problem hiding this comment.
Probably related to the problems with my conda environments, so feel free to ignore.
There was a problem hiding this comment.
And here I was hoping that not having to deal with any encoding would be better for portability.
Re: conda environment issues, have you tried doing a fresh install? conda update does some funny things, and as we resolve from lock files it is more reliable to remove the conda environment and recreate it.
# Remove existing conda env
conda env remove -n cset-dev
# Create new one (from inside CSET directory)
conda create -n cset-dev --file requirements/locks/py312-lock-linux-64.txt
|
Thanks for the review. I'll go ahead and merge it. Hopefully we can resolve your conda troubles and get it working again soon. |
Fixes #296
I hope you both had a good break, and I apologies for the rather large review request on your return. This PR adds variables into the recipe, which allows recipes to be make generic across times, locations, model fields, and more.