misc: add Toy chapter 1 python code, examples and notebook #354

superlopuh · 2023-01-18T20:51:24Z

This is the first installment of the Toy tutorial saga, and the one that least depends on xDSL. The next chapter will include xDSL IR generation

review-notebook-app · 2023-01-18T20:51:29Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

docs/Toy Tutorial/toy/location.py

superlopuh · 2023-01-18T20:54:21Z

Most of the code is as similar as makes sense to the MLIR implementation of Toy frontend

codecov · 2023-01-18T20:57:46Z

Codecov Report

Base: 88.49% // Head: 87.91% // Decreases project coverage by -0.58% ⚠️

Coverage data is based on head (0787e65) compared to base (2846e92).
Patch coverage: 76.42% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
- Coverage   88.49%   87.91%   -0.58%     
==========================================
  Files          64       70       +6     
  Lines        7847     8546     +699     
  Branches     1285     1364      +79     
==========================================
+ Hits         6944     7513     +569     
- Misses        645      756     +111     
- Partials      258      277      +19

Impacted Files	Coverage Δ
docs/Toy/toy/toy_ast.py	`64.11% <64.11%> (ø)`
docs/Toy/toy/parser.py	`79.73% <79.73%> (ø)`
docs/Toy/toy/lexer.py	`88.75% <88.75%> (ø)`
docs/Toy/toy/location.py	`100.00% <100.00%> (ø)`
docs/Toy/toy/tests/test_toy.py	`100.00% <100.00%> (ø)`
xdsl/utils/exceptions.py	`68.62% <0.00%> (-23.69%)`	⬇️
tests/test_parser_error.py	`93.02% <0.00%> (-6.98%)`	⬇️
xdsl/xdsl_opt_main.py	`87.95% <0.00%> (-0.49%)`	⬇️
xdsl/parser.py	`83.60% <0.00%> (-0.09%)`	⬇️
xdsl/ir.py	`84.69% <0.00%> (-0.06%)`	⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

webmiche

LGTM. I can't help but feel that this really is an MLIR tutorial as I feel like they could not have taken an example that needs more prior knowledge than that, but I guess it makes sense with the infrastructure as MLIR is very focused on tensors and stuff...

webmiche · 2023-01-19T07:33:47Z

docs/Toy Tutorial/examples/ast.toy

+# CHECK-NEXT:       Params: []
+# CHECK-NEXT:       Block {
+# CHECK-NEXT:         VarDecl a<> @{{.*}}ast.toy:11:3
+# CHECK-NEXT:           Literal: <2, 3>[ <3>[ 1.000000e+00, 2.000000e+00, 3.000000e+00], <3>[ 4.000000e+00, 5.000000e+00, 6.000000e+00]] @{{.*}}ast.toy:11:11


I am not 100% sure how good/consistent floating point prints work in Python. I know there can be issues with comparisons with floats generally, so maybe printing can be dangerous as well.

I think here it makes sense as this is literally a cast from 1 to a float print of 1, but this is something we have to keep in mind for the future, especially when checking for floats that had arithmetic performed on them.

webmiche · 2023-01-19T07:34:35Z

docs/Toy Tutorial/examples/codegen.toy

@@ -0,0 +1,31 @@
+# RUN: toyc-ch2 %s -emit=mlir 2>&1 | FileCheck %s


This is already chapter 2, right? Is there a reason why this is contained in this PR?

I added all the toy examples, and didn't bother removing any of the comments, it's not like we're running filecheck on them. Happy to remove the comments if you think they might be confusing

But we should right? Can we?

We technically can't right now because we don't match the MLIR syntax exactly, even in MLIR mode on the printer. I think it would be a nice goal, but a bit orthogonal to this PR

webmiche · 2023-01-19T07:38:52Z

docs/Toy Tutorial/toy/parser.py

+TokenT = TypeVar('TokenT', bound=Token)
+
+
+class Parser:


Could we maybe somehow unify this with our parser infrastructure? Or maybe even autogenerate at some point (@AntonLydike). I guess not for this PR though :)

I'm not sure how helpful that would be, this is a sort of "toy" parser, that we don't need to be particularly efficient or good to work on, more for didactic purposes. Might be a bit of a distraction for people to try to understand the parser generation infrastructure instead of xDSL itself

PapyChacal

LGTM!

docs/Toy/Toy_Ch1.ipynb

georgebisbas

We need to set up proper testing for this PR.
Also, what about placing the implementation out of docs?

docs/Toy/examples/ast.toy

This reverts commit b75fcd0.

georgebisbas

My understanding is that not all files of this PR are necessary to run this notebook.
Is that right? If so can we incrementally add only the necessary things, in every one of the forthcoming PRs?

georgebisbas

Also, a gentle reminder that docs/ folder is not under codecov!

superlopuh · 2023-01-22T14:46:43Z

Which files aren't necessary? Can't codecov what we don't test ;)

georgebisbas · 2023-01-23T11:21:49Z

Which files aren't necessary? Can't codecov what we don't test ;)

This line here: https://github.com/xdslproject/xdsl/blob/main/.coveragerc#L8
contains what is currently under codecov.

Are all the files in docs/Toy used/covered in this PR?

superlopuh · 2023-01-23T11:53:40Z

I'm not sure what the benefit would be of adding the folder to codecov, if it's not covered by any unit tests, which we probably don't want, at least in the short term.

math-fehr

Nice! I added minor comments.
You are often using toy-ch1 or toy-ch2, where are these defined?

math-fehr · 2023-01-23T15:46:21Z

.github/workflows/ci-notebooks.yml

@@ -39,3 +39,4 @@ jobs:
      run: |
        pytest --nbval-lax docs/irdl.ipynb --maxfail 1 -vv
        pytest --nbval docs/tutorial.ipynb --maxfail 1 -vv
+        pytest --nbval docs/Toy/Toy_Ch1.ipynb -vv


If we don't have --maxfail 1, does that mean that we could potentially break toy without seeing it on the CI?

Is that how maxfail works? I thought it was that we wouldn't see it on the CI if there were 0 or 1 failures

Not at all. maxfail just stops testing at the first failure. By default testing stops at 5 failures. It can be dropped

math-fehr · 2023-01-23T15:47:20Z

docs/Toy/Toy_Ch1.ipynb

+    "## The Language\n",
+    "\n",
+    "This tutorial will be illustrated with a toy language that we’ll call “Toy”\n",
+    "(naming is hard...). Toy is a tensor-based language that allows you to define\n",


While I do agree with the joke, I'm not sure about keeping it here :/

The text is almost entirely lifted word-for-word from the page. We could also write our own tutorial, which would probably be a bit safer from the copyright perspective.

Maybe you can add a citation at the last cell to refer/cite to the page

math-fehr · 2023-01-23T15:48:42Z

docs/Toy/Toy_Ch1.ipynb

+    "to the LLVM Kaleidoscope equivalent that are detailed in the first two chapters of the\n",
+    "[Kaleidoscope Tutorial](https://llvm.org/docs/tutorial/MyFirstLanguageFrontend/LangImpl02.html).\n",
+    "\n",
+    "The next chapter will demonstrate how to convert this AST into MLIR."


Suggested change

"The next chapter will demonstrate how to convert this AST into MLIR."

"The next chapter will demonstrate how to convert this AST into xDSL."

hmm I'm not sure which is correct, since it's going to MLIR text representation, through xDSL the framework.

convert this AST, first to xDSL and next to MLIR ?.

docs/Toy/Toy_Ch1.ipynb

docs/Toy/toy/ast.py

math-fehr · 2023-01-23T15:52:43Z

docs/Toy/toy/ast.py

+        self.loc = loc
+        print(self.dump())
+
+    @property


Should we make this an abstract class? Or is this class instantiated at some point without being a child class?

How do you make it abstract?

You inherit from ABC :)

I thought that was for isinstance dark magic. Does it do things like prevent instantiation? I guess we could make this class an abstract class, not sure if it's worth doing

math-fehr · 2023-01-23T15:54:55Z

docs/Toy/toy/ast.py

+
+@dataclass
+class VarType:
+    'A variable type with shape information.'


I think we should use """ everywhere:
https://peps.python.org/pep-0257/

Convert to using """

math-fehr · 2023-01-23T15:56:32Z

docs/Toy/toy/ast.py

+@dataclass
+class PrototypeAST:
+    '''
+    This class represents the "prototype" for a function, which captures its


We could probably say in the comments that this is a function declaration.
I guess the name prototype is only used in the C/C++ world? (though I may be wrong)

The comments are also lifted word-for-word from the MLIR tutorial. In my mind keeping it almost identical was a win, but you're right that as a standalone tutorial it's not made stronger by using some of the cpp concepts

math-fehr · 2023-01-23T16:02:20Z

docs/Toy/examples/ast.toy

@@ -0,0 +1,76 @@
+# RUN: toyc-ch1 %s -emit=ast 2>&1 | FileCheck %s


These tests are using toyc-ch1 and toyc-ch2, however no such file exist?

They don't really use them, I just copied the files including this comment

Can you add a note that they are not used for future reference?

math-fehr · 2023-01-23T16:14:50Z

docs/Toy/toy/parser.py

+        self.tokens = tokenize(file, program)
+        self.pos = 0
+
+    def getToken(self):


Can we document the few next functions?

add documentation for getToken and token precedence functions

Co-authored-by: Fehr Mathieu <mathieu.fehr@gmail.com>

georgebisbas

I think we are one step before merging. Great work. Left a few minor comments, I am fine if they addressed at some other PR..

georgebisbas · 2023-01-24T11:01:05Z

docs/Toy/Toy_Ch1.ipynb

+    "## The Language\n",
+    "\n",
+    "This tutorial will be illustrated with a toy language that we’ll call “Toy”\n",
+    "(naming is hard...). Toy is a tensor-based language that allows you to define\n",


Maybe you can add a citation at the last cell to refer/cite to the page

georgebisbas · 2023-01-24T11:02:04Z

docs/Toy/Toy_Ch1.ipynb

+    "to the LLVM Kaleidoscope equivalent that are detailed in the first two chapters of the\n",
+    "[Kaleidoscope Tutorial](https://llvm.org/docs/tutorial/MyFirstLanguageFrontend/LangImpl02.html).\n",
+    "\n",
+    "The next chapter will demonstrate how to convert this AST into MLIR."


convert this AST, first to xDSL and next to MLIR ?.

georgebisbas · 2023-01-24T11:02:41Z

docs/Toy/examples/ast.toy

@@ -0,0 +1,76 @@
+# RUN: toyc-ch1 %s -emit=ast 2>&1 | FileCheck %s


Can you add a note that they are not used for future reference?

georgebisbas · 2023-01-24T11:03:33Z

docs/Toy/toy/lexer.py

@@ -0,0 +1,120 @@
+from dataclasses import dataclass


import re

from .location import Location

from dataclasses import dataclass
from pathlib import Path
from typing import List

georgebisbas · 2023-01-24T11:03:46Z

docs/Toy/toy/lexer.py

+            # 1-indexed
+            col += 1
+            if char == '#':
+                # Comment


Nope, although I see where you're coming from :) Comments start with a # in Toy like in Python, which makes this comment look a bit weird

georgebisbas · 2023-01-24T11:04:36Z

docs/Toy/toy/parser.py

+        return self.tokens[self.pos]
+
+    def getTokenPrecedence(self) -> int:
+        '''Returns precedence if the current token is a binary operation, -1 otherwise'''


Don;t we want """ quotes?

Do we? I guess I'm used to single quotes, but consistency is best. It looks like I'm the only one who's added triple single quotes in the whole repo, I'll undo that in this one, and a followup for the code that I've already checked in

georgebisbas · 2023-01-24T11:05:33Z

docs/Toy/toy/parser.py

+
+    def pop(self) -> Token:
+        self.peek()
+        self.pos += 1


why do not we just do return self.tokens[self.pos] ? Am I missing something?

Because I need to change pos before returning it. I could do something like res = self.tokens[self.pos]; self.pos += 1; return res but I prefer how I wrote it in the code

docs/Toy/toy/parser.py

georgebisbas · 2023-01-24T11:09:41Z

docs/Toy/toy/tests/test_toy.py

+
+def test_parse_ast():
+    ast_toy = Path() / 'docs' / 'Toy' / 'examples' / 'ast.toy'
+


Can;t we do:

dir_to_scan = "......xxx.x.x.x.x.x.x..examples/ast_toy" ast_toy = Path(dir_to_scan)

as in: https://pbpython.com/pathlib-intro.html

from pathlib import Path dir_to_scan = "/media/chris/KINGSTON/data_analysis" p = Path(dir_to_scan)

I guess we could if you prefer it

superlopuh added 4 commits January 18, 2023 20:39

add toy AST and first chapter

532b9f5

wip remove things beyond ch1

c2a0fda

remove unnecessary config changes

0ff8950

fix file names

18dd32f

superlopuh added documentation Improvements or additions to documentation convolve labels Jan 18, 2023

superlopuh requested review from tobiasgrosser, AntonLydike, martin-luecke, georgebisbas, math-fehr, PapyChacal and webmiche January 18, 2023 20:51

superlopuh self-assigned this Jan 18, 2023

superlopuh commented Jan 18, 2023

View reviewed changes

docs/Toy Tutorial/toy/location.py Outdated Show resolved Hide resolved

add newline at end of location.py

698c77c

superlopuh added convolve and removed convolve-0 labels Jan 18, 2023

webmiche approved these changes Jan 19, 2023

View reviewed changes

remove space in folder name

b1d9bc8

PapyChacal approved these changes Jan 19, 2023

View reviewed changes

superlopuh added this to the Convolve 2023-02 milestone Jan 19, 2023

superlopuh removed the convolve label Jan 19, 2023

superlopuh mentioned this pull request Jan 19, 2023

Add Toy as a dialect. #214

Closed

georgebisbas reviewed Jan 19, 2023

View reviewed changes

docs/Toy/Toy_Ch1.ipynb Show resolved Hide resolved

georgebisbas requested changes Jan 19, 2023

View reviewed changes

docs/Toy/examples/ast.toy Show resolved Hide resolved

add some text about the content of the tutorial

633c9fa

superlopuh added 2 commits January 21, 2023 09:14

Revert "test chapter 1 notebook with nbval"

dd78ee4

This reverts commit b75fcd0.

add back notebook without formatting

ef7fcf7

georgebisbas requested changes Jan 22, 2023

View reviewed changes

georgebisbas reviewed Jan 22, 2023

View reviewed changes

superlopuh added 5 commits January 23, 2023 14:25

add docs to codecov

7a2b706

make toy imports relative

78598e3

add tests and __init__.py files

db1b24a

move toy tests into toy folder

bbfbab7

remove unused import pytest

2c3cbde

math-fehr approved these changes Jan 23, 2023

View reviewed changes

superlopuh and others added 6 commits January 23, 2023 16:48

rename ast.py to toy_ast.py

ceef200

format whitespace

119c3ae

rename test

6bec4b3

run all tests for codecov

8914108

remove init=False

9f93e29

Update docs/Toy/Toy_Ch1.ipynb

b740957

Co-authored-by: Fehr Mathieu <mathieu.fehr@gmail.com>

superlopuh requested review from georgebisbas and math-fehr January 23, 2023 18:54

superlopuh added 2 commits January 23, 2023 22:29

use triple quotes for doc comments

aa346d1

add doc comments to parser

bf7bacc

georgebisbas approved these changes Jan 24, 2023

View reviewed changes

superlopuh added 4 commits January 24, 2023 11:38

reorder imports

27e273c

replace triple single quotes with triple double quotes

23371c7

compress paths

49e3c49

split out notebook for chapter 1

0787e65

superlopuh merged commit 2d69772 into main Jan 24, 2023

superlopuh deleted the sasha/toy-ch1 branch January 24, 2023 12:47

		@@ -0,0 +1,31 @@
		# RUN: toyc-ch2 %s -emit=mlir 2>&1 \| FileCheck %s

	"The next chapter will demonstrate how to convert this AST into MLIR."
	"The next chapter will demonstrate how to convert this AST into xDSL."

		@@ -0,0 +1,76 @@
		# RUN: toyc-ch1 %s -emit=ast 2>&1 \| FileCheck %s


		def test_parse_ast():
		ast_toy = Path() / 'docs' / 'Toy' / 'examples' / 'ast.toy'

misc: add Toy chapter 1 python code, examples and notebook #354

misc: add Toy chapter 1 python code, examples and notebook #354

Conversation

superlopuh commented Jan 18, 2023

review-notebook-app bot commented Jan 18, 2023

superlopuh commented Jan 18, 2023

codecov bot commented Jan 18, 2023 • edited

Codecov Report

webmiche 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

PapyChacal left a comment

Choose a reason for hiding this comment

georgebisbas left a comment

Choose a reason for hiding this comment

georgebisbas left a comment

Choose a reason for hiding this comment

georgebisbas left a comment

Choose a reason for hiding this comment

superlopuh commented Jan 22, 2023

georgebisbas commented Jan 23, 2023

superlopuh commented Jan 23, 2023

math-fehr 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

superlopuh Jan 23, 2023 • edited

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

superlopuh Jan 23, 2023 • edited

Choose a reason for hiding this comment

georgebisbas 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

codecov bot commented Jan 18, 2023 •

edited

superlopuh Jan 23, 2023 •

edited

superlopuh Jan 23, 2023 •

edited