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

Silent failure in uw templater rendering when incorporating jinja2 macros in latest UFS SRW Application #422

Closed
mkavulich opened this issue Feb 29, 2024 · 13 comments
Labels
feature Added functionality

Comments

@mkavulich
Copy link

mkavulich commented Feb 29, 2024

Expected behavior
uw template render tool should be able to handle rendering jinja with macros.

Current behavior
UW tools fails silently with exit status 1, creating an empty file instead of a populated template.

Machines affected
Testing was done on Hera, but this shouldn't be platform-specific.

To Reproduce

  1. Activate UW tools conda environment from latest UFS SRW App. Commands for hera are:
git clone git@github.com:ufs-community/ufs-srweather-app
cd ufs-srweather-app/
./manage_externals/checkout_externals
./devbuild.sh -p=hera
module use modulefiles/
module load wflow_hera
conda activate srw_app
  1. Download files provided to replicate issue (you will need to remove the .txt extension before running):
    macros.jinja.txt
    met_plus_settings.yaml.txt
    GridStat_or_PointStat.conf.txt

  2. Run templater command, and observe error condition described above:

$ uw template render -i GridStat_or_PointStat.conf -o out -v --values-file met_plus_settings.yaml
[2024-02-29T15:48:40]    DEBUG Command: uw template render -i GridStat_or_PointStat.conf -o out -v --values-file met_plus_settings.yaml
[2024-02-29T15:48:40]    DEBUG Internal arguments:
[2024-02-29T15:48:40]    DEBUG ---------------------------------------------------------------------
[2024-02-29T15:48:40]    DEBUG       input_file: GridStat_or_PointStat.conf
[2024-02-29T15:48:40]    DEBUG      output_file: out
[2024-02-29T15:48:40]    DEBUG      values_file: met_plus_settings.yaml
[2024-02-29T15:48:40]    DEBUG    values_format: yaml
[2024-02-29T15:48:40]    DEBUG        overrides: {}
[2024-02-29T15:48:40]    DEBUG    values_needed: False
[2024-02-29T15:48:40]    DEBUG          dry_run: False
[2024-02-29T15:48:40]    DEBUG ---------------------------------------------------------------------
[2024-02-29T15:48:40]    DEBUG Read initial values from met_plus_settings.yaml
macros.jinja
$ echo $?
1
$ ls -al out
-rw-r--r-- 1 Michael.Kavulich fv3lam 0 Feb 29 15:48 out

Note that removing the import command results in a successful template render, so this seems to be the culprit.

Context
See also discussion in ufs-community/ufs-srweather-app#1005

@mkavulich mkavulich added the bug Something isn't working label Feb 29, 2024
@maddenp-noaa
Copy link
Contributor

Hi @mkavulich, and thank you for the report.

I see what's happening and am discussing with the uwtools team. I suspect that this is not a bug per se, in that it's not something that I'd expect to work "out of the box" the way something like {{ 2 + 2 }} should. It requires specific configuration in the Python application code to inform Jinja about where to find template files (and even that macros.jinja should be interpreted as a file), and users would need a way to specify one or more paths to search for files, which uw doesn't currently provide. But it sure looks like a valuable feature request!

I haven't used Jinja's import in the past, so the foregoing is based on reading and tests I've just done. If any of it sounds wrong to you, please let me know.

More soon...

@maddenp-noaa
Copy link
Contributor

We have a plan to implement this feature. (For readers with EPIC Jira access: UW-523.) We'll try to get it in our upcoming v2.1.0 release.

If you're using uwtools in the context of a UFS App (e.g. SRW), it may take longer for the new version to be explicitly added there, but we can help individuals with one-off updates, etc.

If you're using uwtools as a standalone tool, you'd be able to build your own version from a git clone as soon as we merge a fix to main, which we can also help with. We'll close this issue when that happens, so you'll know.

@maddenp-noaa maddenp-noaa added feature Added functionality and removed bug Something isn't working labels Feb 29, 2024
@gsketefian
Copy link

@maddenp-noaa I just wanted to provide some background info. This issue arose when I tried to use jinja2 macros with a relatively recent version of the uwtools CLI in the SRW App. Here's the PR that gives a lot more detail. The odd thing is that an older version of uwtools (not a CLI; it was just the uwtools repo cloned within the SRW App) was working with the macros. When I updated my PR to the latest develop branch of the SRW that brought with it the conda installation of various python packages and the uwtools CLI, the macros no longer worked and I got the behavior @mkavulich described above. Part (maybe all) of the problem might be a mismatch between the version of jinja2 that uwtools expects (the one in the conda installation) and the one that the SRW App uses in spack-stack-1.5.0. If interested, you can read the details of what I've tried in that PR. I'm happy to provide more info if that will help. Thanks.

@maddenp-noaa
Copy link
Contributor

@gsketefian Thanks for this, very helpful. I'll look at the older uwtools code to see what it was doing to support loadable templates.

@gsketefian
Copy link

Hi @maddenp-noaa FYI the SRW App code manager Michael Leuken ran a test of my PR #1005 using the version of jinja2 that the uwtools CLI expects, and the failure is still there. The PR has more details of what he did. So apparently it's not (just?) a version issue.

@maddenp-noaa
Copy link
Contributor

I believe I see what needs to be done in uwtools to support loading Jinja2 macros. It should be straightforward to implement, and I can probably do so on Monday. I'll see if there's an easy way to update the reproducer environment @mkavulich detailed above so that you can make use of the fix without waiting for an updated uwtools version to be incorporated into SRW.

@maddenp-noaa
Copy link
Contributor

I have a potential update to uwtools to address this issue, and have installed it into the SRW I built on hera in /scratch2/BMC/wrfruc/Paul.Madden/gh422/ufs-srweather-app. After doing the prescribed module load and conda activate commands, and omitting debug and stacktrace info, I get

% uw template render -i GridStat_or_PointStat.conf -o out -v --values-file met_plus_settings.yaml
...
The list of valid field groups for forecasts (fgs_fcst) must be identical
to that for observations (fgs_obs) but isn't:
  fgs_fcst = ['ASNOW', 'REFC', 'RETOP']
  fgs_obs = ['APCP', 'ASNOW', 'REFC', 'RETOP']

Is this the expected behavior given these files? It seems intentional. Please let me know if not, but otherwise please feel free to test with my SRW install. I've tried to open up permissions sufficiently. If things look ok, I could give you a recipe for updating your own SRW install with this uwtools version. This version is based on the latest release, though, so it may be that other uwtools uses in SRW may need to be updated to continue to work correctly.

The update works as follows: By default, the directory containing the template file (identified by -i in the above command) will be searched for template files named in e.g. Jinja2 import statements. Alternatively, an optional --search-path CLI option can be used to suppress the default path and to provide a colon-separated series of paths to search for templates.

@mkavulich
Copy link
Author

@maddenp-noaa Thanks for getting to this quickly! I believe that error is expected given the minimal example I provided. Unfortunately I have the "Hera is down" blues today and that's where my work is staged so I will have to wait until tomorrow to check that it works in the context of the SRW branch I'm working on.

@gsketefian
Copy link

gsketefian commented Mar 5, 2024

@maddenp-noaa Yes, thank you for coming up with a fix so soon! I'm assuming you didn't run one of the workflow-end-to-end (WE2E) tests in the SRW App to get that message; you just ran that command on the command line. If so, that would explain the error message, which (if I remember right) is coming from the jinja2 macros file.

I would like to test your fix on my PR (#1005) when Hera is back up. Will it work if I simply replace the uwtools directory in the conda build that the SRW requires with your version? Or are there other packages to update as well?

If the update requires changes in other calls to uwtools in the SRW, it may be that we'll have to first open a new PR into the app to update uwtools (along with appropriate tests), and, once that's merged, re-test my PR. Still, I'd like to test my PR with your fix to see whether it at least works for the WE2E tests that I'm running, so it would be great if you could send that recipe for updating uwtools. Thanks!

@maddenp-noaa
Copy link
Contributor

@gsketefian You should be able to test by replacing any conda activate srw_app command with conda activate /scratch2/BMC/wrfruc/Paul.Madden/gh422/ufs-srweather-app/conda/envs/srw_app/ to activate my environment directly. If that's done in one place (or a few easily updatable places) in your automation, that might be sufficient for testing.

Alternatively, I think you could rename your ufs-srweather-app/conda (following the naming conventions in the initial issue report) directory and then ln -s /scratch2/BMC/wrfruc/Paul.Madden/gh422/ufs-srweather-app/conda ufs-srweather-app/conda to use my entire conda installation instead. (conda is sensitive to paths due to the way it encodes absolute paths into binaries, and you can't e.g. copy one conda installation to a new path and expect it to work, but I think a symllink should work.)

I think one of those would be easiest to try first, but I can give you an recipe for updating your own installation with the test uwtools version if we're initially unsuccessful. Please let me know how it goes.

@maddenp-noaa
Copy link
Contributor

We've incorporated this fix into today's v2.1.0 release. You can install this version into an existing, activated conda environment with

conda install -c ufs-community -c conda-forge uwtools=2.1.0

As before, it's possible that other CLI invocations of or API calls into uwtools from SRW will need to be (hopefully lightly) updated for compatibility with the new release, and someone on our team will be working on an SRW PR to that effect.

@maddenp-noaa
Copy link
Contributor

From following ticket traffic elsewhere, it sounds like this is working as expected. If I don't hear objections in the next couple days, I'll close this ticket. Thanks again for the report.

@mkavulich
Copy link
Author

@maddenp-noaa FYI Gerard was on vacation this week so I assume he hasn't had a chance to test it for his PR yet.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Added functionality
Projects
None yet
Development

No branches or pull requests

3 participants