From 97ecd8eccafa49af298efabce10be56a4ce4fb41 Mon Sep 17 00:00:00 2001 From: Paul Ferrell Date: Mon, 4 Mar 2024 08:04:32 -0700 Subject: [PATCH 1/4] List variables can now extend config items - List variables can now be used to extend config items. - Variable lists cannot be extended in this way. --- docs/tests/format.rst | 18 +++++++ docs/tests/values.rst | 1 + lib/pavilion/parsers/expressions.py | 83 ++++++++++++++++++++--------- lib/pavilion/parsers/strings.py | 78 +++++++++++++++++++++------ lib/pavilion/resolve.py | 43 +++++++++++---- test/tests/parser_tests.py | 35 +++++++++--- test/tests/resolve_tests.py | 45 ++++++++++++++++ 7 files changed, 243 insertions(+), 60 deletions(-) create mode 100644 test/tests/resolve_tests.py diff --git a/docs/tests/format.rst b/docs/tests/format.rst index 04cc62868..398cade6e 100644 --- a/docs/tests/format.rst +++ b/docs/tests/format.rst @@ -174,6 +174,24 @@ automatically interprets that as a list of that single value. - {bar: 2} baz: {buz: "hello"} +Extending Config Lists +^^^^^^^^^^^^^^^^^^^^^^ + +Items in the config that can take a list can be extended by a list variable. + +.. code:: yaml + + mytest: + variables: + extra_modules: + - intel + - intel-mkl + + build: + modules: + - openmpi + - '{{ extra_modules.* }}' + Hidden Tests ------------ diff --git a/docs/tests/values.rst b/docs/tests/values.rst index f873f02a3..a71f6df5f 100644 --- a/docs/tests/values.rst +++ b/docs/tests/values.rst @@ -67,6 +67,7 @@ identically to Python3 (with one noted exception). This includes: - Power operations, though Pavilion uses ``^`` to denote these. ``a ^ 3`` - Logical operations ``a and b or not False``. - Parenthetical expressions ``a * (b + 1)`` +- Concatenation ``"hello " .. "world"`` and ``[1, 2 ,3] .. [4, 5, 6]`` List Operations ``````````````` diff --git a/lib/pavilion/parsers/expressions.py b/lib/pavilion/parsers/expressions.py index ce7cac593..9a624c979 100644 --- a/lib/pavilion/parsers/expressions.py +++ b/lib/pavilion/parsers/expressions.py @@ -35,7 +35,8 @@ compare_expr: add_expr ((EQ | NOT_EQ | LT | GT | LT_EQ | GT_EQ ) add_expr)* add_expr: mult_expr ((PLUS | MINUS) mult_expr)* mult_expr: pow_expr ((TIMES | DIVIDE | INT_DIV | MODULUS) pow_expr)* -pow_expr: primary ("^" primary)? +pow_expr: conc_expr ("^" conc_expr)? +conc_expr: primary (CONCAT primary)* primary: literal | var_ref | negative @@ -78,6 +79,9 @@ DIVIDE: "/" INT_DIV: "//" MODULUS: "%" +// The ignored whitespace below mucks with this, which requires us to include the whitespace in the +// token definition. +CONCAT: / *\.\./ AND: /and(?![a-zA-Z_])/ OR: /or(?![a-zA-Z_])/ NOT.2: /not(?![a-zA-Z_])/ @@ -139,38 +143,38 @@ class BaseExprTransformer(PavTransformer): def _apply_op(self, op_func: Callable[[Any, Any], Any], arg1: lark.Token, arg2: lark.Token, allow_strings=True): - """""" - - # Verify that the arg value types are something numeric, or that it's a - # string and strings are allowed. - for arg in arg1, arg2: - if isinstance(arg.value, list): - for val in arg.value: - if (isinstance(val, str) and not allow_strings and - not isinstance(val, self.NUM_TYPES)): - raise ParserValueError( - token=arg, - message="Non-numeric value '{}' in list in math " - "operation.".format(val)) - else: - if (isinstance(arg.value, str) and not allow_strings and - not isinstance(arg.value, self.NUM_TYPES)): + """Apply the given op_func to the given arguments. If strings are not allowed, then + the values are converted to numeric types if possible.""" + + if not allow_strings: + # Shouldn't throw exceptions or introduce invalid types. + val1 = auto_type_convert(arg1.value) + val2 = auto_type_convert(arg2.value) + for arg, val in (arg1, val1), (arg2, val2): + if isinstance(val, str): raise ParserValueError( - token=arg1, - message="Non-numeric value '{}' in math operation." - .format(arg.value)) + arg, + f"Math operation given string '{val}', but strings aren't valid " + "operands") + elif isinstance(val, list): + for subval in val: + if isinstance(subval, str): + raise ParserValueError( + arg, + f"Math operation given string '{subval}', but strings aren't valid " + "operands") + else: + val1 = arg1.value + val2 = arg2.value - if (isinstance(arg1.value, list) and isinstance(arg2.value, list) - and len(arg1.value) != len(arg2.value)): + if (isinstance(val1, list) and isinstance(val2, list) + and len(val1) != len(val2)): raise ParserValueError( token=arg2, message="List operations must be between two equal length " "lists. Arg1 had {} values, arg2 had {}." .format(len(arg1.value), len(arg2.value))) - val1 = arg1.value - val2 = arg2.value - if isinstance(val1, list) and not isinstance(val2, list): return [op_func(val1_part, val2) for val1_part in val1] elif not isinstance(val1, list) and isinstance(val2, list): @@ -369,6 +373,35 @@ def pow_expr(self, items) -> lark.Token: else: return items[0] + def conc_expr(self, items) -> lark.Token: + """Concatenate strings or lists. The '..' operator isn't captured.""" + + if len(items) == 1: + return items[0] + + def _concat(val1, val2): + if isinstance(val1, list): + if isinstance(val2, list): + return val1 + val2 + else: + return [item + str(val2) for item in val1] + else: + if isinstance(val2, list): + return [str(val1) + item for item in val2] + else: + return str(val1) + str(val2) + + base = items[0].value + for item in items[1:]: + if item.type == 'CONCAT': + continue + + val = item.value + + base = _concat(base, val) + + return self._merge_tokens(items, base) + def primary(self, items) -> lark.Token: """Simply pass the value up to the next layer. :param list[Token] items: Will only be a single item. diff --git a/lib/pavilion/parsers/strings.py b/lib/pavilion/parsers/strings.py index 6325bda5f..277dd9fba 100644 --- a/lib/pavilion/parsers/strings.py +++ b/lib/pavilion/parsers/strings.py @@ -12,6 +12,7 @@ import lark from .common import PavTransformer from ..errors import ParserValueError +from ..utils import auto_type_convert from .expressions import get_expr_parser, ExprTransformer, VarRefVisitor STRING_GRAMMAR = r''' @@ -152,7 +153,31 @@ def start(self, items) -> str: if len(items) > 1: parts.append('\n') - return ''.join(parts) + # If everything is a string, join the bits and return them. + is_str = lambda v: isinstance(v, str) + if all(map(is_str, parts)): + return ''.join(parts) + + # Check if all the parts are whitespace or a (single) list. + found_list = None + for part in parts: + if isinstance(part, list): + if found_list is None: + found_list = part + else: + raise ParserValueError( + token=self._merge_tokens(items, parts), + message="Value contained multiple expressions that resolved to lists.") + elif not (is_str(part) and part.isspace()): + raise ParserValueError( + token=self._merge_tokens(items, parts), + message="Value resolved to a list, but also contained none-whitespace.") + if not found_list: + raise ParserValueError( + token=self._merge_tokens(items, parts), + message="Value resolved to an invalid type (this should never happen).") + + return found_list def string(self, items) -> lark.Token: """Strings are merged into a single token whose value is all @@ -357,29 +382,48 @@ def _resolve_expr(self, err.pos_in_stream += expr.start_pos raise - if not isinstance(value, (int, float, bool, str)): + format_spec = expr.value['format_spec'] + if format_spec is not None: + spec = format_spec[1:] + def _format(val): + try: + return f'{val:{spec}}' + except ValueError as err: + try: + val = auto_type_convert(val) + return f'{val:{spec}}' + except ValueError as err: + raise ParserValueError( + expr, f"Invalid format_spec '{spec}' for value '{val}': {err}") + else: + _format = str + + if isinstance(value, list): + formatted = [] + for idx, item in enumerate(value): + if not isinstance(item, (int, float, bool, str)): + type_name = type(value).__name__ + raise ParserValueError( + expr, + "Pavilion expression resolved to a list with a bad item. Expression " + "lists can only contain basic data types (int, float, str, bool), but " + "we got type {} in position {} with value: \n{}" + .format(type_name, idx, item)) + + formatted.append(_format(item)) + + return formatted + + elif not isinstance(value, (int, float, bool, str)): type_name = type(value).__name__ raise ParserValueError( expr, "Pavilion expressions must resolve to a string, int, float, " - "or boolean. Instead, we got {} '{}'" + "or boolean (or a list of such values). Instead, we got {} '{}'" .format('an' if type_name[0] in 'aeiou' else 'a', type_name)) - format_spec = expr.value['format_spec'] - - if format_spec is not None: - try: - value = '{value:{format_spec}}'.format( - format_spec=format_spec[1:], - value=value) - except ValueError as err: - raise ParserValueError( - expr, - "Invalid format_spec '{}': {}".format(format_spec, err)) else: - value = str(value) - - return value + return _format(value) @staticmethod def _displace_token(base: lark.Token, inner: lark.Token): diff --git a/lib/pavilion/resolve.py b/lib/pavilion/resolve.py index 93b8e6265..b249e8e83 100644 --- a/lib/pavilion/resolve.py +++ b/lib/pavilion/resolve.py @@ -34,12 +34,22 @@ def test_config(config, var_man): for section in config: try: - resolved_dict[section] = section_values( + section_val = config[section] + + resolved_val = section_values( component=config[section], var_man=var_man, allow_deferred=section not in NO_DEFERRED_ALLOWED, key_parts=(section,), ) + + if isinstance(section_val, str) and isinstance(resolved_val, list): + raise TestConfigError( + "Section '{}' was set to '{}' which resolved to list '{}'. This key does " + "not accept lists.".format(section, section_val, resolved_val)) + + resolved_dict[section] = resolved_val + except (StringParserError, ParserValueError) as err: raise TestConfigError("Error parsing '{}' section".format(section), err) @@ -144,26 +154,39 @@ def section_values(component: Union[Dict, List, str], if isinstance(component, dict): resolved_dict = type(component)() - for key in component.keys(): - resolved_dict[key] = section_values( - component[key], + for key, val in component.items(): + resolved_val = section_values( + val, var_man, allow_deferred=allow_deferred, deferred_only=deferred_only, key_parts=key_parts + (key,)) + if isinstance(val, str) and isinstance(resolved_val, list): + # We probably got back a list, which is only valid when dealing with a list + full_key = '.'.join(key_parts + (key,)) + raise TestConfigError( + "Key '{}' was set to '{}' which resolved to list '{}'. This key does not " + "accept lists.".format(full_key, val, resolved_val)) + + resolved_dict[key] = resolved_val return resolved_dict elif isinstance(component, list): resolved_list = type(component)() - for i in range(len(component)): - resolved_list.append( - section_values( - component[i], var_man, + for idx, val in enumerate(component): + resolved_val = section_values( + val, var_man, allow_deferred=allow_deferred, deferred_only=deferred_only, - key_parts=key_parts + (i,) - )) + key_parts=key_parts + (idx,) + ) + # String resolution converted a string to a list - extend this list with those items. + if isinstance(resolved_val, list) and isinstance(val, str): + resolved_list.extend(resolved_val) + else: + resolved_list.append(resolved_val) + return resolved_list elif isinstance(component, str): diff --git a/test/tests/parser_tests.py b/test/tests/parser_tests.py index 2485237a5..3e6dbdd70 100644 --- a/test/tests/parser_tests.py +++ b/test/tests/parser_tests.py @@ -136,14 +136,24 @@ def test_visitors(self): '[1, 2, 3, 4] * [4, 4, 2, 1]': [4, 8, 6, 4], '[1, 2, 3, 4] < 3 < 10 < [10, 11, 12, 13]': [False, True, False, False], '[1, "foo", False, 0, ""] and 2': [True, True, False, False, False], + + # Concatenation + '"abc" .. "def"': "abcdef", + '[1, 2, 3] .. [4, 5, 6]': [1, 2, 3, 4, 5, 6], + '"a" .. ["b"] .. "c" .. ["d", "e"]': ['abc', 'd', 'e'], } def test_good_expressions(self): """Make sure good expressions work as expected.""" + import logging + from lark import Lark, logger + + logger.setLevel(logging.DEBUG) expr_parser = lark.Lark( grammar=parsers.expressions.EXPR_GRAMMAR, parser='lalr', + debug=True, ) trans = parsers.expressions.ExprTransformer(self.var_man) @@ -186,10 +196,10 @@ def test_good_expressions(self): '1 2': 'Invalid Syntax', 'a b': 'Invalid Syntax', 'True False': 'Invalid Syntax', - '"hello" + 1': "Non-numeric value 'hello' in math operation.", - '"hello" * 1': "Non-numeric value 'hello' in math operation.", - '"hello" ^ 1': "Non-numeric value 'hello' in math operation.", - '-"hello"': "Non-numeric value 'hello' in math operation.", + '"hello" + 1': "Math operation given string", + '"hello" * 1': "Math operation given string", + '"hello" ^ 1': "Math operation given string", + '-"hello"': "Math operation given string", 'var.1.2.3.4.5': "Invalid variable 'var.1.2.3.4.5': too many name " "parts.", 'var.structs.0.*': "Could not resolve reference 'var.structs.0.*': " @@ -200,10 +210,8 @@ def test_good_expressions(self): 'floor. Got 2, but expected 1', '[1, 2, 3] + [1, 2]': "List operations must be between two equal " "length lists. Arg1 had 3 values, arg2 had 2.", - '[1, "foo", 3] * 2': "Non-numeric value 'foo' in list in math " - "operation.", - '3 + [1, "foo", 3]': "Non-numeric value 'foo' in list in math " - "operation.", + '[1, "foo", 3] * 2': "Math operation given string", + '3 + [1, "foo", 3]': "Math operation given string", } @@ -266,6 +274,11 @@ def test_good_strings(self): '${hello}': '${hello}', 'also{': 'also{', '[0-9]': '[0-9]', + # Allow strings to return a list if it's just the list expression and whitespace. + ' {{ ints.* }} \n': ['0', '1', '2', '3', '4', '5'], + '{{ ints.* }}': ['0', '1', '2', '3', '4', '5'], + '{{ ints.* :02d}}': ['00', '01', '02', '03', '04', '05'], + } for expr, result in self.GOOD_EXPRESSIONS.items(): @@ -315,6 +328,12 @@ def test_bad_strings(self): # Bad expression '{{ nope }}': "Could not find a variable named 'nope' in any " "variable set.", + + # Lists are allowed, but they can't have anything other than whitespace in the string. + 'nope{{ ints.* }}': "Value resolved to a list, but also contained none-whitespace.", + # Only a single list is allowed. + '{{ ints.* }} {{ints.*}}': "Value contained multiple expressions that resolved to lists.", + } for expr, expected in self.BAD_EXPRESSIONS.items(): diff --git a/test/tests/resolve_tests.py b/test/tests/resolve_tests.py new file mode 100644 index 000000000..e217410c9 --- /dev/null +++ b/test/tests/resolve_tests.py @@ -0,0 +1,45 @@ +from pavilion.unittest import PavTestCase +from pavilion import resolve +from pavilion import variables +from pavilion.errors import TestConfigError + +class ResolveTests(PavTestCase): + + # For the most part, the config variable resolution code doesn't care about the + # format of the test. + GOOD_CONFIG = { + 'hello': 'there', + 'var1': '{{ var.single }}', + 'list': [ + 'a', + '{{ var.single }}', + '{{ var.list.* }}',] + } + + def test_resolve(self): + var_man = variables.VariableSetManager() + var_man.add_var_set('var', {'single': '1', + 'list': ['a', 'b', 'c']}) + + resolved = resolve.test_config(self.GOOD_CONFIG, var_man) + + self.assertEqual( + resolved, {'hello': 'there', 'list': ['a', '1', 'a', 'b', 'c'], 'var1': '1'}) + + BAD_CONFIGS = [ + # List expressions can only extend lists. + ({'hello': '{{ var.list.* }}'}, "Section 'hello' was set to"), + ({'a': {'b': '{{ var.list.* }}'}}, "Key 'a.b' was set to"), + ] + + def test_resolve_bad(self): + var_man = variables.VariableSetManager() + var_man.add_var_set('var', {'single': '1', + 'list': ['a', 'b', 'c']}) + + for bad_config, exp_error in self.BAD_CONFIGS: + try: + resolved = resolve.test_config(bad_config, var_man) + self.fail("Config '{}' did not raise an error as expected.".format(bad_config)) + except TestConfigError as err: + self.assertTrue(str(err).startswith(exp_error), msg="Did not get expected error.") From cde516fdeac3971cdf0bf54b7738f7a7508a9649 Mon Sep 17 00:00:00 2001 From: Paul Ferrell <51765748+Paul-Ferrell@users.noreply.github.com> Date: Mon, 4 Mar 2024 08:07:36 -0700 Subject: [PATCH 2/4] Update format.rst --- docs/tests/format.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/tests/format.rst b/docs/tests/format.rst index 398cade6e..f8999e740 100644 --- a/docs/tests/format.rst +++ b/docs/tests/format.rst @@ -190,6 +190,7 @@ Items in the config that can take a list can be extended by a list variable. build: modules: - openmpi + # All the values from the 'extra_modules' variable will be added to the list. - '{{ extra_modules.* }}' Hidden Tests From 22e4e0f6074127e7b238311c744a25c1f56468d9 Mon Sep 17 00:00:00 2001 From: tygoetsch Date: Mon, 4 Mar 2024 08:41:17 -0700 Subject: [PATCH 3/4] Update expressions.py whitespace cleanup --- lib/pavilion/parsers/expressions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pavilion/parsers/expressions.py b/lib/pavilion/parsers/expressions.py index 9a624c979..3009874a6 100644 --- a/lib/pavilion/parsers/expressions.py +++ b/lib/pavilion/parsers/expressions.py @@ -153,14 +153,14 @@ def _apply_op(self, op_func: Callable[[Any, Any], Any], for arg, val in (arg1, val1), (arg2, val2): if isinstance(val, str): raise ParserValueError( - arg, + arg, f"Math operation given string '{val}', but strings aren't valid " "operands") elif isinstance(val, list): for subval in val: if isinstance(subval, str): raise ParserValueError( - arg, + arg, f"Math operation given string '{subval}', but strings aren't valid " "operands") else: From c3ede2adbfa0f382c6abb020a74df6c19edc5cfe Mon Sep 17 00:00:00 2001 From: tygoetsch Date: Mon, 4 Mar 2024 12:38:49 -0700 Subject: [PATCH 4/4] Update expressions.py whitespace fix --- lib/pavilion/parsers/expressions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pavilion/parsers/expressions.py b/lib/pavilion/parsers/expressions.py index 3009874a6..e2a1c2e6b 100644 --- a/lib/pavilion/parsers/expressions.py +++ b/lib/pavilion/parsers/expressions.py @@ -162,7 +162,7 @@ def _apply_op(self, op_func: Callable[[Any, Any], Any], raise ParserValueError( arg, f"Math operation given string '{subval}', but strings aren't valid " - "operands") + "operands") else: val1 = arg1.value val2 = arg2.value