GTScript unstructured prototype frontend #52

tehrengruber · 2020-08-24T13:07:43Z

First prototype frontend from (unstructured) GTScript down to C++ code with working regression tests.

havogt

Some minor comments

examples/unstructured/frontend.py

setup.cfg

havogt · 2020-08-26T07:33:20Z

src/gt_frontend/frontend.py

+                "Vertex": common.LocationType.Vertex,
+                "Edge": common.LocationType.Edge,
+                "Cell": common.LocationType.Cell,
+                # "Field": Field,


src/gt_frontend/gtscript.py

src/gt_frontend/gtscript_to_gtir.py

DropD

Type annotations are missing in a lot of places (specifically return types often even when argument types are annotated), mypy can be used to discover all instances of that.

Most modules seem to lack unit tests (seeing as they are new modules I would at least expect some changes to existing tests if not new test modules).

There are many, many "todo" comments. These must be addressed either by converting them into issues (low priority), deleting them (obsolete) or fixing them right away.

I commented on some instances but not all where functions are hard to test due to deep nesting of conditionals, often in combination with loops. This will become painfully obvious while writing unit tests or if they ever need to be debugged.

examples/unstructured/frontend.py

setup.cfg

examples/unstructured/frontend.py

DropD · 2020-08-26T07:46:29Z

src/gt_frontend/ast_node_matcher.py

+    return False
+
+
+# todo: pattern node ast.Name(bla=123) matches ast.Name(id="123") since bla is not an attribute


Open TODO, please explain what the steps are to close it and whether they have been taken already.

DropD · 2020-08-26T07:49:27Z

src/gt_frontend/ast_node_matcher.py

+
+
+def match(concrete_node, pattern_node, captures=None) -> Tuple[bool, Dict[str, ast.AST]]:
+    if captures is None:


I am fairly certain this function is complex enough to warrant a docstring.

At first glance I have no Idea what this does and where it fits in.

Also, concrete_node and pattern_node should probably be annotated as Any, while captures looks like Optional[Dict]

I would also recomend thinking about splitting at least the more complex elif cases into functions for readability and also testability. Unit testing or even regression testing this function must be a nightmare.

src/gt_frontend/py_to_gtscript.py

DropD · 2020-08-26T09:10:57Z

src/gt_frontend/py_to_gtscript.py

+        ast.Div: gtc.common.BinaryOperator.DIV,
+    }
+
+    def transform(self, node, eligible_node_types=None):


In the interest of readability and testability I would think about reducing the amount of nested code paths maybe by splitting some conditional paths into smaller functions.

DropD · 2020-08-26T09:12:13Z

src/gt_frontend/py_to_gtscript.py

+                return self.leaf_map[type(node)]
+            else:
+                # visit node fields and transform
+                # todo: check if multiple nodes match and throw an error in that case


Open TODO: explain steps to resolve or steps already taken.

DropD · 2020-08-26T09:18:31Z

tests/unit_tests/test_frontend.py

+from gt_frontend import ast_node_matcher as anm
+
+
+class TestAstNodeMatcher:


I feel this test suite could be simplified while still covering more cases by simplifying the match function.

Also: is there a reason for this to be a class and not just top-level test functions? There is no setup or teardown it seems. Or is this run using something other than pytest?

havogt · 2020-08-27T08:03:17Z

About the mypy comment: in the pre-commit config there is a section about mypy which is currently disabled due to one issue I couldn't resolve. Maybe you, @DropD, have an idea how to resolve that and we can activate it.

Master has exactly one error (with python 3.8)

src/eve/typing.py:25: error: unused 'type: ignore' comment
Found 1 error in 1 file (checked 13 source files)

Regarding todo's, I have several comments:

I agree that obsolete/easy to solve should be removed/resolved
I don't think we should open issues for TODOs that are low priority here, that's too noisy and the frontend should still be considered experimental
I propose to use the format TODO(github-user-name) for todos that you want to resolve later

…s (this example is not meaningful at this point)

- Improved code quality - Added sparse and temporary fields functionality to the prototype frontend - Python ast templates are now seperate from the gtscript node definition - Improved node canonicalization

Code cleanup

…ead of creating the IR manually Minor fixes & cleanup

Co-authored-by: Rico Haeuselmann <[email protected]>

…ed code) (#54) * Allow injection of static code blocks at the beginning of the generated code in UsidCodeGenerator * Remove check in `UsidCodeGenerator` for conformance with sid concept as it triggers a compilation error in nvcc (workaround with templated function call instead of decltype possible, but withdrawn) * Fix hardcoded name for temporary field in UsidCodeGenerator * Added gtc regression test for FVM nabla operator (gtscript -> generated code) Note: driver code borrowed from cpputils regression tests via symlink for now * Fix cmake not rebuilding on changes in cpputils and gtc regression tests * Update src/gtc/unstructured/usid_codegen.py Co-authored-by: Hannes Vogt <[email protected]> * Update src/gtc/unstructured/usid_codegen.py Co-authored-by: Hannes Vogt <[email protected]> * Fix `ModuleNotFoundError: No module named 'eve'` in CI * Added __init__.py to gt_frontent * Remove obsolete hash_path in gtc regression tests cmake * Revert "Fix `ModuleNotFoundError: No module named 'eve'` in CI" This reverts commit cd7531d. * CI: pip install eve (#57) Co-authored-by: Hannes Vogt <[email protected]>

Co-authored-by: Rico Haeuselmann <[email protected]>

…ght now and lead to errors with mypy

…_node_matcher

Reformat

egparedes

I add some comments but I would say that it looks good enough to be merged right now in order to avoid merge conflicts later.

General Python issues:

missing type annotations
all the instance attributes should be initialized in the __init__ method (an empty initialization with None is also ok)
use f-strings instead of str.format() whenever possible, since it usually enhances readibility (and it's slightly faster)
it should be discussed if Protocols or AbstractBaseClasses could be enough to describe the "parser type system" instead of the current metaclass-based hierarchy

Format issues:

docstrings should end with a period .
several wrongly capitalized names like UsidGpuCodeGenerator or GtirToNir (according to PEP8: "when using acronyms in CapWords, capitalize all the letters of the acronym. Thus HTTPServerError is better than HttpServerError")
"To Do" comments should use Hannes suggestion: TODO(github-handle)
I would try to split the code in more logical blocks using empty lines. Sources sometimes look a bit too crammed

Naming suggestions:

py_to_gtscript -> python_to_gtscript (and also change the class names inside)

setup.cfg

src/gt_frontend/__init__.py

egparedes · 2020-09-21T10:11:47Z

src/gt_frontend/frontend.py

+from gtc.unstructured.usid_codegen import UsidGpuCodeGenerator
+
+
+class GTScriptCompilationTask:


Some points about this class:

I don't see the point of storing the intermediate results .gtir, .gtscript_ast as instance state if it is not used at all. If there is a point to do it, the you they should be already initialized in the __init__ method with an empty initialization to None

Is this class aimed to be instantiated and kept around after use because the state is meaningful or it is meant to be a cheap "use and thow-away" object? I would provide a class method like apply in visitors for the second use-case, and maybe change the generate method name to __call__ in the first one

I find a bit confusing the definition of the default value for the code_generator in two different methods. Again, if the class is meant to be instantiated and used like a regular object, I think I would add the code_generator parameter to the constructor, if it is a single use instance, I would add the default value only to the public method and make the argument compulsory in the private one, to simplify the understanding of the code and avoid setting different default values for the same object in different places

The intermediate results are stored since they are used in the unit tests. I spent absolutely no time in making this clean since the complete class will disappear with the PassManager to be introduced in Eve and the build stages approach @DropD introduced in GT4Py. I've added a todo comment in the meantime.

src/gt_frontend/py_to_gtscript.py

src/gt_frontend/gtscript_ast.py

egparedes · 2020-09-21T11:31:58Z

src/gt_frontend/gtscript_to_gtir.py

+        self.types[item] = val
+        return self.types[item]
+
+    def materialize_constant(self, name, expected_type=None):


A bit weird name for just a getter method with validation. I would use something simpler:

Suggested change

def materialize_constant(self, name, expected_type=None):

def get_constant(self, name, expected_type=None):

Also the docstrings seems to be out of date

The naming was intentional here to remind myself that this would currently be the place were constant folding would take place (for which I don't know yet what the right place is). It's just that there is nothing to "fold" since the values are externally supplied. I extended the docstring to emphasize this and added a todo.

src/gt_frontend/gtscript_to_gtir.py

src/gt_frontend/ast_node_matcher.py

Co-authored-by: Enrique G. Paredes <[email protected]>

…ings, docstrings should end in period, capitalization, todo comment format)

tehrengruber · 2020-09-21T22:12:27Z

After countless remarks and commits this PR has reached a level of maturity I believe it to be ready to be merged into master. It is still a prototype with many (but clearly documented) todos, but avoiding merge conflicts and keeping the review of further changes reviewable has priority. The remaining remarks will then be addressed in a seperate PR.

tehrengruber marked this pull request as draft August 24, 2020 13:15

tehrengruber force-pushed the gt4pyast_unstructured branch 4 times, most recently from 73bf1e6 to bb39143 Compare August 25, 2020 10:46

tehrengruber requested review from havogt and DropD August 25, 2020 10:55

tehrengruber marked this pull request as ready for review August 25, 2020 11:18

havogt reviewed Aug 26, 2020

View reviewed changes

DropD suggested changes Aug 26, 2020

View reviewed changes

tehrengruber and others added 19 commits September 18, 2020 21:31

Code cleanup

1fc171d

GTC regression test temporary fields

7e437b1

Code cleanup

d4fe955

Code cleanup

c47ce2b

Enable stencil definition inside others functions

6b949cc

Add additional stencils for the code generation unit tests

32c3166

Code cleanup

33fa635

Added cell to cell reduction example in preperation of GTIR extension…

90dbf9c

…s (this example is not meaningful at this point)

GT4PyAst to GTIR working (rest unfinished and chaotic)

979d433

fix minor bugs

7843e71

Some cleanup

73653e5

Allow sparse fields

e39451d

FVM Nabla stencil code generation working (c++ code not tested yet)

975c8b1

- Improved code quality - Added sparse and temporary fields functionality to the prototype frontend - Python ast templates are now seperate from the gtscript node definition - Improved node canonicalization

Renamed GT4PyAst into GTScriptAst

b51bf06

Code cleanup

Moved unstructured prototype frontend examples to examples folder

f2c0ee4

Tests and fixes (default captures) for ast_node_matcher

cb2998c

Added file headers

67c3efa

Unstructured cpp regression tests now use the prototype frontend inst…

3d5a01d

…ead of creating the IR manually Minor fixes & cleanup

Allow subscript in assignment

c55faa1

tehrengruber and others added 13 commits September 20, 2020 23:41

Extended documentation & code cleanup

086df9d

Extended documentation & code cleanup

57918b2

Update examples/unstructured/frontend.py

89a5713

Co-authored-by: Rico Haeuselmann <[email protected]>

Cleanup in src/gt_frontend/ast_node_matcher.py

2351c30

Co-authored-by: Rico Haeuselmann <[email protected]>

Update src/gt_frontend/py_to_gtscript.py

079dafd

Co-authored-by: Rico Haeuselmann <[email protected]>

Comment out __metadata__ and select in BaseNode as they are unused ri…

1b1c511

…ght now and lead to errors with mypy

Fix errors reported by mypy

70294f7

Reformat

2b7bb04

Fix flake8 errors

94d2e63

Add code generation unit tests

0c201b3

Distinguish between optional fields and fields with None value in ast…

809e865

…_node_matcher

Simplified ast_node_matcher again

0e3ff58

Reformat

tehrengruber force-pushed the gt4pyast_unstructured branch from 4273fc5 to 0e3ff58 Compare September 20, 2020 21:59

Minor fix

45eb869

egparedes reviewed Sep 21, 2020

View reviewed changes

tehrengruber and others added 10 commits September 21, 2020 14:01

Addressed code quality remarks

f00b424

Reformat

26e7720

Update src/gt_frontend/py_to_gtscript.py

8b626ec

Co-authored-by: Enrique G. Paredes <[email protected]>

Update src/gt_frontend/py_to_gtscript.py

515916d

Co-authored-by: Enrique G. Paredes <[email protected]>

Update src/gt_frontend/py_to_gtscript.py

e7a87c5

Co-authored-by: Enrique G. Paredes <[email protected]>

Update src/gt_frontend/py_to_gtscript.py

53a44af

Co-authored-by: Enrique G. Paredes <[email protected]>

Addressed code quality remarks (instance attribute in __init__, f-str…

d9b7004

…ings, docstrings should end in period, capitalization, todo comment format)

Merge

6e01a4a

Reformat & Minor fix

ab27422

Addressed code quality remarks

16a1a43

Reformat

f7c8b03

tehrengruber merged commit 8c58566 into master Sep 21, 2020

havogt deleted the gt4pyast_unstructured branch November 3, 2020 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTScript unstructured prototype frontend #52

GTScript unstructured prototype frontend #52

tehrengruber commented Aug 24, 2020

havogt left a comment

havogt Aug 26, 2020

DropD left a comment

DropD Aug 26, 2020

DropD Aug 26, 2020

DropD Aug 26, 2020

DropD Aug 26, 2020

DropD Aug 26, 2020

DropD Aug 26, 2020

DropD Aug 26, 2020

havogt commented Aug 27, 2020

egparedes left a comment •

edited

Loading

egparedes Sep 21, 2020

tehrengruber Sep 21, 2020

egparedes Sep 21, 2020

tehrengruber Sep 21, 2020

tehrengruber commented Sep 21, 2020

		return False


		# todo: pattern node ast.Name(bla=123) matches ast.Name(id="123") since bla is not an attribute



		def match(concrete_node, pattern_node, captures=None) -> Tuple[bool, Dict[str, ast.AST]]:
		if captures is None:

		from gt_frontend import ast_node_matcher as anm


		class TestAstNodeMatcher:

		from gtc.unstructured.usid_codegen import UsidGpuCodeGenerator


		class GTScriptCompilationTask:

	def materialize_constant(self, name, expected_type=None):
	def get_constant(self, name, expected_type=None):

GTScript unstructured prototype frontend #52

GTScript unstructured prototype frontend #52

Conversation

tehrengruber commented Aug 24, 2020

havogt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DropD 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

havogt commented Aug 27, 2020

egparedes left a comment • edited Loading

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

tehrengruber commented Sep 21, 2020

egparedes left a comment •

edited

Loading