-
Notifications
You must be signed in to change notification settings - Fork 119
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
[develop] Add jinja templating to all config file formats. #701
[develop] Add jinja templating to all config file formats. #701
Conversation
ff841b8
to
e1d3c73
Compare
e1d3c73
to
c21ae6b
Compare
c21ae6b
to
6208b5e
Compare
I tested on Hera. It has following running error messages: |
@panll You can parse the xml workflow file under
If you try an xml file in an EXPTDIR instead, it should work with the commands you used.
You may encounter issue #691 with an old experiment directory, but it should work with experiment generated from latest develop or this PR. For example this is the output with the community test case:
|
It's important to note that the YAML created from this tool will not be sufficient to create an XML given the tool we recently have. |
@christinaholtNOAA It can generate the workflow xml file from a rendered yaml file, because we can convert from yaml to xml too. However, it would need a jinja-templated equivalent of the original
|
@panll If you can re-test and approve this PR, assuming there are no issues, it would be great since I need this for my next PR. |
@danielabdi-noaa @christinaholtNOAA While working through issues in PR #707, it has become clear that using
leads to the following:
Is this something that could be addressed in this PR, or should a separate issue be created for this problem? Thanks! |
@MichaelLueken I think ignoring validation of the new |
@danielabdi-noaa I should also note that this behavior was noted on Orion. Testing this on Gaea, this wasn't an issue. However, your strategy sounds like it will work for any machine. Thanks! |
@MichaelLueken Since hard-coding
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies that I did not fully review this PR on my first pass.
This addition is, IMO, not a desired feature. This will absolutely convolute how we do configuration in the App. We should really have one way to do a task, and this one circumvents the path forward that I had been trying to apply for a fairly clean way to create a workflow without prolific numbers of control switches.
I'd propose that we do not go this route at all.
It works now, thanks! @danielabdi-noaa @MichaelLueken |
@MichaelLueken I'd suggest a "Do Not Merge" flag until we have a full discussion around the Google Doc that @danielabdi-noaa introduced in the meeting this morning. |
Per the discussion during today's SRW CM meeting, I have added the DO_NOT_MERGE label to this PR. |
…nity#718) Added the capability for the SRW App to use GDAS nemsio data type as initial conditions. --------- Co-authored-by: Edward Snyder <Edward.Snyder@noaa.com>
@danielabdi-noaa The reviewers voted to close this PR until demonstrably necessary to ensure that the workflow works correctly. Thank you very much for discussing the issues that you see with the current SRW workflow. It is clear that a lot more work is required to get the existing workflow into a spot where we will all be happy. |
DESCRIPTION OF CHANGES:
Mainly addresses issue #700
extend_yaml
does, namely, fill template values of dictionary entries. Given a second file containing the context (dictionary) for rendering the jinja template, you can now do this:instead of
If these variables are defined only for realtime runs, the first option without the
{% else %}
will not define them, while the second option still has to define the variables to null string.I think this feature is required for the upcoming rrfs workflow with yaml template files.
Other changes include:
With this change you should be able to parse a rendered xml file (not the one in parm) and convert it to any of the five formats.
If the context for rendering is available in file
context.yaml
, we can render and convert the templated file instead like thisEdit: Work done after this PR's approval
yaml/json
workflow file work as intended. Tested with:The test is conducted by converting the generated
FV3LAM_wflow.xml
file say to yaml using this tool, and then back to xml.The resulting xml file replaces the one in EXPTDIR and the test re-run again. The above tests succeeded using this verification method.
Type of change
TESTS CONDUCTED:
DEPENDENCIES:
None
DOCUMENTATION:
ISSUE:
CHECKLIST
LABELS (optional):
A Code Manager needs to add the following labels to this PR:
CONTRIBUTORS (optional):