search

GridTools / gt4py

Python library for generating high-performance implementations of stencil kernels for weather and climate modeling from a domain-specific language (DSL).
https://GridTools.github.io/gt4py
BSD 3-Clause "New" or "Revised" License
110 stars 49 forks source link

Functional: Incomplete arg handling in foast parser #997

Open BenWeber42 opened 2 years ago

BenWeber42 commented 2 years ago

See these tests: https://github.com/BenWeber42/gt4py/commit/b5d2c87be469efdb78ed8eb59358fa239a7e67a3

def test_pos_only_arg():
    def pos_only_arg(x: float, /) -> float:
        return x

    _ = FieldOperatorParser.apply_to_function(pos_only_arg)

def test_kw_only_arg():
    def kw_only_arg(*, x: float) -> float:
        return x

    _ = FieldOperatorParser.apply_to_function(kw_only_arg)

They fail with:

=================================================================================================================== FAILURES ===================================================================================================================
______________________________________________________________________________________________________________ test_pos_only_arg _______________________________________________________________________________________________________________

    def test_pos_only_arg():
        def pos_only_arg(x: float, /) -> float:
            return x

>       _ = FieldOperatorParser.apply_to_function(pos_only_arg)

tests/functional_tests/ffront_tests/test_foast_parsing.py:293: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/functional/ffront/dialect_parser.py:115: in apply_to_function
    return cls.apply(src, closure_vars, annotations)
src/functional/ffront/dialect_parser.py:106: in apply
    raise err
src/functional/ffront/dialect_parser.py:89: in apply
    output_ast = cls._postprocess_dialect_ast(
src/functional/ffront/func_to_foast.py:98: in _postprocess_dialect_ast
    typed_foast_node = FieldOperatorTypeDeduction.apply(foast_node)
src/functional/ffront/foast_passes/type_deduction.py:161: in apply
    typed_foast_node = cls().visit(node)
src/eve/traits.py:162: in visit
    result = super().visit(node, **kwargs)
src/eve/visitors.py:123: in visit
    return visitor(node, **kwargs)
src/functional/ffront/foast_passes/type_deduction.py:169: in visit_FunctionDefinition
    new_body = self.visit(node.body, **kwargs)
src/eve/traits.py:162: in visit
    result = super().visit(node, **kwargs)
src/eve/visitors.py:123: in visit
    return visitor(node, **kwargs)
src/eve/visitors.py:175: in generic_visit
    return node.__class__(  # type: ignore
src/eve/visitors.py:178: in <genexpr>
    if (new_child := self.visit(child, **kwargs)) is not NOTHING
src/eve/traits.py:162: in visit
    result = super().visit(node, **kwargs)
src/eve/visitors.py:123: in visit
    return visitor(node, **kwargs)
src/eve/visitors.py:162: in generic_visit
    **{
src/eve/visitors.py:165: in <dictcomp>
    if (new_child := self.visit(child, **kwargs)) is not NOTHING
src/eve/traits.py:162: in visit
    result = super().visit(node, **kwargs)
src/eve/visitors.py:123: in visit
    return visitor(node, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <functional.ffront.foast_passes.type_deduction.FieldOperatorTypeDeduction object at 0x2b50e1960bb0>
node = Name(location=SourceLocation(line=291, column=12, source='/scratch-shared/meteoswiss/scratch/wbenjami/gt4py-bugs/tests...ests/test_foast_parsing.py', end_line=291, end_column=13), type=DeferredSymbolType(constraint=None), id=SymbolRef('x'))
kwargs = {'symtable': ChainMap({SymbolName('int'): Symbol(location=SourceLocation(line=290, column=1, source='/scratch-shared/m...={}, returns=ScalarType(kind=<ScalarKind.FLOAT64: 1064>, shape=None)), namespace=<Namespace.CLOSURE: 'closure'>)}, {})}
symtable = ChainMap({SymbolName('int'): Symbol(location=SourceLocation(line=290, column=1, source='/scratch-shared/meteoswiss/scr...s={}, returns=ScalarType(kind=<ScalarKind.FLOAT64: 1064>, shape=None)), namespace=<Namespace.CLOSURE: 'closure'>)}, {})

    def visit_Name(self, node: foast.Name, **kwargs) -> foast.Name:
        symtable = kwargs["symtable"]
        if node.id not in symtable or symtable[node.id].type is None:
>           raise FieldOperatorTypeDeductionError.from_foast_node(
                node, msg=f"Undeclared symbol {node.id}"
            )
E             File "/scratch-shared/meteoswiss/scratch/wbenjami/gt4py-bugs/tests/functional_tests/ffront_tests/test_foast_parsing.py", line 291
E               def pos_only_arg(x: float, /) -> float:
E               return x
E                          ^
E           functional.ffront.foast_passes.type_deduction.FieldOperatorTypeDeductionError: Could not deduce type: Undeclared symbol x

src/functional/ffront/foast_passes/type_deduction.py:240: FieldOperatorTypeDeductionError
_______________________________________________________________________________________________________________ test_kw_only_arg _______________________________________________________________________________________________________________

    def test_kw_only_arg():
        def kw_only_arg(*, x: float) -> float:
            return x

>       _ = FieldOperatorParser.apply_to_function(kw_only_arg)

tests/functional_tests/ffront_tests/test_foast_parsing.py:300: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/functional/ffront/dialect_parser.py:115: in apply_to_function
    return cls.apply(src, closure_vars, annotations)
src/functional/ffront/dialect_parser.py:94: in apply
    ).visit(cls._preprocess_definition_ast(definition_ast)),
src/functional/ffront/func_to_foast.py:89: in _preprocess_definition_ast
    sat = SingleAssignTargetPass.apply(ssa)
src/functional/ffront/ast_passes/simple_assign.py:23: in apply
    result = list(cls().visit(node))
src/functional/ffront/ast_passes/simple_assign.py:33: in visit
    yield from result
src/functional/ffront/ast_passes/simple_assign.py:42: in generic_visit
    new_node, *_ = list(self.visit(old_value)) or (None,)
src/functional/ffront/ast_passes/simple_assign.py:33: in visit
    yield from result
src/functional/ffront/ast_passes/simple_assign.py:39: in generic_visit
    new_values = [i for j in old_value for i in self.visit(j)]
src/functional/ffront/ast_passes/simple_assign.py:39: in <listcomp>
    new_values = [i for j in old_value for i in self.visit(j)]
src/functional/ffront/ast_passes/simple_assign.py:33: in visit
    yield from result
src/functional/ffront/ast_passes/simple_assign.py:37: in generic_visit
    for field, old_value in ast.iter_fields(node):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

node = None

    def iter_fields(node):
        """
        Yield a tuple of ``(fieldname, value)`` for each field in ``node._fields``
        that is present on *node*.
        """
>       for field in node._fields:
E       AttributeError: 'NoneType' object has no attribute '_fields'

../ben-stack/epoche-003/spack-instance/opt/spack/linux-rhel7-skylake_avx512/gcc-12.1.0/python-3.10.6-pe6hg5gby4tsmwdb2nwqbosacmpcmp4h/lib/python3.10/ast.py:252: AttributeError
=========================================================================================================== short test summary info ============================================================================================================

The first failure is because ast.argument isn't handled exhaustively here: https://github.com/GridTools/gt4py/blob/c0388b6757a363d51361132b5206e6fac6a5f609/src/functional/ffront/func_to_foast.py#L174-L175

The error message may be correct, because we only support normal arguments in field operators. It's still missleading to the user, because there is no message that positional only arguments aren't supported (this is silently ignored by the foast parser).

The second failure seems to trigger an unrelated bug earlier, but would most likely also not be handled well.

tehrengruber commented 2 years ago

We are aware of the issue and will gradually improve it (work to support keyword arguments in calls to programs has already started: https://github.com/GridTools/gt4py/pull/982).

BenWeber42 commented 1 year ago

There's still:

The second failure seems to trigger an unrelated bug earlier, but would most likely also not be handled well.

I guess I should open a separate issue for that, but I don't have time to look into it currently and see if the test case could be reduced. I can try to do this later.

We are aware of the issue and will gradually improve it (work to support keyword arguments in calls to programs has already started: https://github.com/GridTools/gt4py/pull/982).

Yes, but ast.arguments has up to 7 attributes, and we are currently ignoring all but one. It's not clear to me whether that PR will make sure that all cases are handled properly (if something isn't supported yet, the user should get an appropriate error message). Leaving all the other cases completely unhandled is against the principle of defensive coding. You are only risking forgetting about it (we don't have an official backlog anyway) and then leave it up to the user to discover it, which will lead to a poor user experience. If we don't support something (because we plan to add support later, deliberately chose not to support it or any other reason), all cases should be handled properly, and the user should get an appropriate error message for all those cases.