Refactoring template driver to dynamically create RAVEN workflows #391

j-bryan · 2024-11-18T16:26:26Z

Pull Request Description

What issue does this change request address?

#390

What are the significant changes in functionality due to this change

This pull request restructures the templating system used to generate RAVEN workflows.

The RAVEN template and the template driver are separated into different classes.
A FeatureDriver class is introduced to define changes to a RAVEN template XML to add a single feature. For example, adding a Grid sampler to the RAVEN XML is done with one FeatureDriver instance, while adding the model used by the workflow is done with a different FeatureDriver.
FeatureDriver instances can be composed and grouped in FeatureCollection objects.
The template driver uses feature drivers to edit a template object.
Edits to the template XML are intended to be strictly additive. One aspect of the previous TemplateDriver implementation that I felt made the current state of the template XML unclear at times was not knowing which nodes were present at any given point in the code due to numerous additions, deletions, and edits made throughout the TemplateDriver. Moving to a strictly additive scheme while starting from a much smaller template XML reframes the template driver's function as adding the desired features to the XML, rather than deleting all other unneeded entities from the XML, as was common practice.
Addition of a "flat" RAVEN workflow template for specific cases which can be run as flat workflows.

For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

1. Review all computer code.
2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
4. Automated Tests should pass.
5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large tes.
6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
8. If an analytic test is changed/added, the the analytic documentation must be updated/added.
9. If any test used as a basis for documentation examples have been changed, the associated documentation must be reviewed and assured the text matches the example.

PaulTalbot-INL · 2024-11-18T16:40:45Z

Wow, this is quite an effort! Do you have any diagramming or writeup for the logic flow? I think it's starting to make sense glancing over it, but some developer documentation would help get us up to speed on this new structure. Thanks for your work on this!

dylanjm

Some minor changes and discussion points. I read heavily my assigned sections and then did a quick look over the rest of the files. I will review once more when all changes are submitted.

I was thinking, it might be nice to add some linting checks to our automated testing. Nothing that stops the tests from running, but just checking to make sure we are consistent. Almost like a coverage summary but for linting. @joshua-cogliati-inl @caleb-sitton-inl what are your thoughts on this?

doc/developers/templates.md

src/Cases.py

src/DispatchManager.py

src/Economics.py

src/Placeholders.py

templates/debug_template.py

joshua-cogliati-inl

Some comments on templates/snippets

joshua-cogliati-inl · 2025-01-22T17:25:13Z

templates/snippets/base.py

+  @classmethod
+  def from_xml(cls, node: ET.Element, **kwargs) -> "RavenSnippet":
+    """
+    Alternate constructor which instantiates a new RavenSnippet objectfrom .n existing XML node


Should "objectfrom .n existing" be "object from an existing"?

joshua-cogliati-inl · 2025-01-22T17:26:34Z

templates/snippets/base.py

+
+class RavenSnippet(ET.Element):
+  """
+  RavenSnippet class objects describe one contiguous snippet of RAVEN XML, inheritingfrom .he xml.etree.ElementTree.Element


Should "inheritingfrom .he" be "inheriting from the"?

joshua-cogliati-inl · 2025-01-22T18:41:20Z

templates/snippets/distributions.py

+
+def distribution_class_from_spec(spec) -> type[Distribution]:
+  """
+  Make a new distribution classfrom .he RAVEN input spec for that class


change "classfrom .he" to "class from the"?

joshua-cogliati-inl · 2025-01-22T20:16:49Z

templates/snippets/files.py

+    @ In, value, str, the type value to set
+    @ Out, None
+    """
+    self.set("type", str(value))


If the type of value is str, why is str(value) needed? (Ditto for "path")

It's unnecessary as long as the input type is respected. I'll remove in the interest of cleaner code.

joshua-cogliati-inl · 2025-01-22T20:26:35Z

templates/snippets/models.py

+    """
+    varname = name if not suffix else f"{name}_{suffix}"
+    if not loc:
+      loc = "Samplers|MonteCarlo@name:mc_arma_dispatch"  # where this is pointing 9/10 times


Should the doc string mention that this is the default for loc?

Probably a good idea. I'll add that.

joshua-cogliati-inl · 2025-01-22T22:55:46Z

templates/snippets/steps.py

+    if tag not in self._allowed_subs:
+      raise ValueError(f"Step type {self.tag} does not accept subelements with tag {tag}. Allowed: {self._allowed_subs}.")
+
+    # Create an Assembler nodefrom .he entity


Change "nodefrom .he" to "node from the"?

joshua-cogliati-inl · 2025-01-23T19:42:11Z

I was thinking, it might be nice to add some linting checks to our automated testing. Nothing that stops the tests from running, but just checking to make sure we are consistent. Almost like a coverage summary but for linting. @joshua-cogliati-inl @caleb-sitton-inl what are your thoughts on this?

We use pylint on RAVEN, so this seems reasonable. (pylint is available in conda and pypi)

j-bryan · 2025-01-23T20:09:54Z

I was aware of pylint but hadn't made much use of it before. I was looking through some issues it brought up in my code for this PR and fixed some of the issues it brought up. I agree it would be good to add in a way similar to how it's used by RAVEN.

GabrielSoto-INL

Couple of linting errors, some comments and questions. Fantastic work so far, big fan of the changes

GabrielSoto-INL · 2025-01-23T15:45:45Z

templates/template_driver.py

+    has_static_history = any(s.is_type("CSV") for s in sources)
+    has_synthetic_history = any(s.is_type("ARMA") for s in sources)
+    if has_static_history and has_synthetic_history:
+      raise self.raiseAnError(ValueError, "Mixing ARMA and CSV sources is not yet supported! "


the raise here should be removed, the line should just be self.raiseAnError( ...

GabrielSoto-INL · 2025-01-23T15:55:16Z

templates/template_driver.py

-          assert isinstance(dep, list)
-          new.append(xmlUtils.newNode('Index', attrib={'var':index}, text=', '.join(dep)))
-    dataobjects.append(new)
+    return not any([comp.get_capacity(None, raw=True).is_parametric() for comp in components])


from my Pylinter (https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/use-a-generator.html), you can remove the square brackets here and just have any(comp.get_capacity(...

Oh yeah, using the generator expression directly instead of collecting that generator in a list first would work the same and is a little cleaner. I'll change that.

GabrielSoto-INL · 2025-01-23T18:34:40Z

templates/raven_template.py

+from .snippets.samplers import Sampler, SampledVariable, Grid, Stratified, CustomSampler, EnsembleForward
+from .snippets.optimizers import BayesianOptimizer, GradientDescent
+from .snippets.models import GaussianProcessRegressor, PickledROM, EnsembleModel
+from .snippets.distributions import Distribution, Uniform


should Uniform here be NewDistribution? I dont see a Uniform class in the snippets

I see Uniform is used in _create_new_sampled_capacity below, but it does exist in snippets.distributions

This is because I'm doing something sneaky to create the distribution classes. Since there are so many that RAVEN can accept, I'm actually parsing the RAVEN input specs for the distribution classes and dynamically creating RavenSnippet classes for them. You can look at templates/snippets/distributions.py to see how I'm doing that. The downside is I don't think any linter will be able to find those classes I create.

GabrielSoto-INL · 2025-01-23T18:36:05Z

templates/raven_template.py

+  # PUBLIC API FUNCTIONS #
+  ########################
+
+  def loadTemplate(self) -> None:


for these three "public API functions" the input parameter list is different from the parent Template class that RavenTemplate inherits, should look at that one and consolidate/update

I can do that. I'm not a huge fan of the RAVEN class's function signatures for these methods but it is what it is, I guess

GabrielSoto-INL · 2025-01-23T18:37:23Z

templates/raven_template.py

+    @ Out, None
+    """
+    this_file_dir = Path(__file__).parent
+    template_path = this_file_dir / self.template_path


where is self.template_path being set? should it be in the init?

self.template_path is set by the subclasses, but I agree that this could've been done better. I'll see if I can revise that while changing these public API methods to better match the RAVEN super class definitions.

GabrielSoto-INL · 2025-01-23T20:19:35Z

tests/unit_tests/test_listproperty.py

+class MockListPropertyUser:
+  """ A minimal class implementing an attribute as a listproperty """
+  def __init__(self):
+    self._list = []


docstrings for the methods

GabrielSoto-INL · 2025-01-23T20:23:12Z

tests/integration_tests/mechanics/csv_and_arma_fail/tests

Great addition!

GabrielSoto-INL · 2025-01-23T20:26:21Z

tests/integration_tests/mechanics/debug_mode/gold/dispatch_print.csv

columns ordered differently on purpose?

The column reordering isn't intentional, just how things happened to shake out with how the refactored code orders things. The diff GitHub calculates here is a little misleading though. The real difference is in the way the prefix column is expressed. Since there is no inner/outer workflow for this case (prefixes values expressed as {i_outer}_{i_inner} or something like that), the prefix values are just ints for the flat workflow.

GabrielSoto-INL · 2025-01-23T20:30:21Z

tests/integration_tests/mechanics/pyomo_options/gold/Sweep_Runs_o/sweep.csv

I see, it's a static history so no need for statistics on the results.

GabrielSoto-INL · 2025-01-23T20:34:32Z

tests/integration_tests/mechanics/test_cashplot/gold/Runs_o/dispatch.nc

any idea why these files have changed?

There was a typo in template_driver.py which failed to add some values to the dispatch.nc database. The line in question is here:

HERON/templates/template_driver.py

Line 447 in bb70736

db.append(xmlUtils.newNode('variables', text='GRO_outer_debug_dispath,GRO_outer_debug_synthetics'))

The variable group GRO_outer_debug_dispath should have been GRO_outer_debug_dispatch. The dispatch.nc file here has changed to reflect the addition of these values.

joshua-cogliati-inl

I have reviewed HERON/tests/unit_tests/snippets and HERON/templates/xml.

joshua-cogliati-inl · 2025-01-23T20:40:06Z

tests/unit_tests/snippets/test_optimizers.py

+import sys
+import os
+import unittest
+


Do we have a policy (or precedent) that allows skipping doc strings in tests? Otherwise, they probably need to be added.

j-bryan added 3 commits November 6, 2024 13:12

flat RAVEN for static history

31d4dc7

feature driver approach to template driver refactor

949da7f

some feature drivers

f5c8d54

j-bryan added 13 commits November 21, 2024 09:51

move feature logic to template from template driver

a848112

build workflow for bilevel outer

b6c3749

bilevel inner template

816f5d4

remove redundant code

b40ed92

dynamic creation of distributions from RAVEN input spec

f5ad122

change getters and setters to dynamic properties

aa3209b

naming conventions, housekeeping, other fixes

2cf21ce

WIP debug mode template

26d51bf

a few debug template fixes

078d788

bilevel xml files

d69ae7c

fixes inner file pathing

2abc8a2

outer parallel batch size

4d91ea0

unit testing for snippet classes

61572de

j-bryan marked this pull request as draft December 9, 2024 16:04

j-bryan added 12 commits December 10, 2024 22:24

better snippet property implementation

e6874bd

regolds two XML check tests to remove extraneous nodes

a118b66

fix missing econ UQ vargroup

69662fb

fix ROM file path

43e4356

unit testing for snippets

67d6924

file assembler node type fix

517f029

add case labels to sweep output

d2fc750

final bilevel fixes

8f26fdf

debug mode for synthetic history models

cf0ff44

flattened debug mode writes prefixes differently

f99aa4c

fixes MonteCarlo default init, debug mode sampled variables

58f2408

adds missing items to database due to typo

0c8d39f

j-bryan added 7 commits December 23, 2024 13:16

Working on docstrings

d5d4f26

completes docstrings

e3e14cd

removing unused files

600e9a1

fix: missed some docstrings and type hints

26bd9d2

WIP Merge branch 'devel' into additive_templating

4897a04

integrate implementation of parallel settings files

3983392

developer guide for the template driver

9b28c87

j-bryan marked this pull request as ready for review January 3, 2025 23:27

j-bryan added 7 commits January 13, 2025 14:32

file pathing

a9118da

fix: modifies RampFreq test to make solution unique

6fa9d35

fix: adjust rel_err threshold for new gold file

6c48352

dbg: archive CapacityFactors test results with Github action

fb59267

dbg: revert change to github action

2720cba

fix(tests): adjust zero threshold for capacity factors test

bfd45c6

fix: parallel settings for clusters are correctly identified

3a29161

dylanjm requested changes Jan 21, 2025

View reviewed changes

fix: typos, calculation reuse, source combination assertion

6a22714

joshua-cogliati-inl reviewed Jan 22, 2025

View reviewed changes

GabrielSoto-INL self-requested a review January 23, 2025 15:02

fix: typos and linting warnings

3ad53df

GabrielSoto-INL requested changes Jan 23, 2025

View reviewed changes

joshua-cogliati-inl reviewed Jan 23, 2025

View reviewed changes

j-bryan added 2 commits January 27, 2025 10:10

fix: some fixes for Gabe's review comments

b785ed3

fix(tests): adds docstrings to unit tests

55aea34

j-bryan requested review from dylanjm, joshua-cogliati-inl and GabrielSoto-INL January 28, 2025 15:38

j-bryan changed the title ~~[WIP] Refactoring template driver to dynamically create RAVEN workflows~~ Refactoring template driver to dynamically create RAVEN workflows Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring template driver to dynamically create RAVEN workflows #391

Refactoring template driver to dynamically create RAVEN workflows #391

j-bryan commented Nov 18, 2024

PaulTalbot-INL commented Nov 18, 2024

dylanjm left a comment

joshua-cogliati-inl left a comment

joshua-cogliati-inl Jan 22, 2025

joshua-cogliati-inl Jan 22, 2025

joshua-cogliati-inl Jan 22, 2025

joshua-cogliati-inl Jan 22, 2025

j-bryan Jan 23, 2025

joshua-cogliati-inl Jan 22, 2025

j-bryan Jan 23, 2025

joshua-cogliati-inl Jan 22, 2025

joshua-cogliati-inl commented Jan 23, 2025

j-bryan commented Jan 23, 2025

GabrielSoto-INL left a comment

GabrielSoto-INL Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

j-bryan Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

j-bryan Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

j-bryan Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

j-bryan Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

j-bryan Jan 27, 2025

GabrielSoto-INL Jan 23, 2025

GabrielSoto-INL Jan 23, 2025

j-bryan Jan 27, 2025

joshua-cogliati-inl left a comment

joshua-cogliati-inl Jan 23, 2025 •

edited

Loading

Refactoring template driver to dynamically create RAVEN workflows #391

Are you sure you want to change the base?

Refactoring template driver to dynamically create RAVEN workflows #391

Conversation

j-bryan commented Nov 18, 2024

Pull Request Description

What issue does this change request address?

What are the significant changes in functionality due to this change

For Change Control Board: Change Request Review

PaulTalbot-INL commented Nov 18, 2024

dylanjm left a comment

Choose a reason for hiding this comment

joshua-cogliati-inl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-cogliati-inl commented Jan 23, 2025

j-bryan commented Jan 23, 2025

GabrielSoto-INL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-cogliati-inl left a comment

Choose a reason for hiding this comment

joshua-cogliati-inl Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

joshua-cogliati-inl Jan 23, 2025 •

edited

Loading