[llvm][TableGen][Jupyter] Add configurable default reset behaviour

[DavidSpickett]

Summary

Often you are doing one of 2 things:

• Building a larger example from many small cells.
• Showing many small isolated examples.

The default so far has followed iPython, where cells are connected
by default (in its case, the interpreter state backing them sticks
around).

This change adds a new magic %config where you can change the setting
cellreset to change that behaviour (this is currently the only setting).

Also added is a %noreset magic so that along with %reset you can
override the default for one particular cell.

The default is equivalent to %config cellreset off. If you then
wanted to reset in a cell, you can just do %reset to override it.

(this is what the current notebooks do)

If you instead do %config cellreset on, cells always reset and
you can choose not to using %noreset.

The setting is named cellreset not reset to differentiate it
a bit more from the one off command reset.

The demo notebook has been updated with examples of this in action.

[DavidSpickett]

The motivation here is that notebooks like the one in https://reviews.llvm.org/D137085 are using %reset in almost every cell. With this you could start with %config cellreset on and use %noreset in the few cells you actually want it (or use %config cellreset off for that whole section).

[DavidSpickett]

Rebase.

[DavidSpickett]

Rebase.

[awarzynski]

Makes sense, just a few questions inline. Thank you :)

llvm/utils/TableGen/jupyter/LLVM_TableGen.md

120

I guess that one more example could be:

%reset
%noreset

llvm/utils/TableGen/jupyter/tablegen_kernel/kernel.py

112–135

Comments like this are super helpful, but I guess that you will need to review them every time this method is updated?

143

[nit] I would skip this. The code is self-explanatory and this comment is quite likely to get out of date soon.

229–230

Are there any benefits to allowing %reset and %noreset in one cell? IMO, it could lead to some counter-intuitive behavior.

[DavidSpickett]

llvm/utils/TableGen/jupyter/tablegen_kernel/kernel.py

112–135

These are doctests https://docs.python.org/3/library/doctest.html. So yes, someone has to remember to run them, but that person was presumably editing this method so it's not difficult to notice them.

229–230

%reset
%noreset

Should follow the last one wins precedent set by %args but as implemented it wouldn't. Also I'm not sure you'd ever intentionally do this, good reason to not allow it.

[DavidSpickett]

Disallow %reset and %noreset in the same cell.

Remove settings comment.

[awarzynski]

LGTM, thank you :)

llvm/utils/TableGen/jupyter/LLVM_TableGen.ipynb

 def other_thing : Stuff {}
 <stdin>:1:19: error: Couldn't find class 'Stuff'
def other_thing : Stuff {}
                  ^
+You can also configure the default reset behaviour using the `%config` magic.
+In [6]:%config cellreset on
+class Thing {}
+------------- Classes -----------------
class Thing {
}
------------- Defs -----------------
+In [7]:// The cache is reset here so this is an error.
+def AThing: Thing {}
+<stdin>:2:13: error: Couldn't find class 'Thing'
def AThing: Thing {}
            ^
+The default value is `off`, meaning cells are connected. If you want to override the default for one cell only, use the `%reset` or `%noreset` magic. These always override the default.
+In [8]:class Thing {}
+------------- Classes -----------------
class Thing {
}
------------- Defs -----------------
+In [9]:%noreset
+// This works because of the noreset above.
+def AThing: Thing {}
+------------- Classes -----------------
class Thing {
}
------------- Defs -----------------
def AThing {	// Thing
}
+In [10]:// This does not because we're not changing the default.
+def AnotherThing: Thing {}
+<stdin>:2:19: error: Couldn't find class 'Thing'
def AnotherThing: Thing {}
                  ^
+In [11]:%config cellreset off
+%reset
+// Here we have an empty cache and default reset behaviour.
+------------- Classes -----------------
------------- Defs -----------------
+It is not valid to have `%reset` and `%noreset` in the same cell.
+In [12]:%reset
+%noreset
+%reset and %noreset in the same cell is not allowed. Use only one, or neither.
+Consider setting `cellreset` to the majority usecase for your notebook. For example a tutorial building a large example across many cells will likely want it `off`. One with many standalone examples, `on`.
 There is a "magic" directive `%args` that you can use to send command line arguments to `llvm-tblgen`.
 For example, here we have some code that shows a warning.

llvm/utils/TableGen/jupyter/LLVM_TableGen.md

 def other_thing : Stuff {}
 ```
 
     <stdin>:1:19: error: Couldn't find class 'Stuff'
     def other_thing : Stuff {}
                       ^
 
 
+You can also configure the default reset behaviour using the `%config` magic.
+
+
+```tablegen
+%config cellreset on
+class Thing {}
+```
+
+    ------------- Classes -----------------
+    class Thing {
+    }
+    ------------- Defs -----------------
+
+
+
+```tablegen
+// The cache is reset here so this is an error.
+def AThing: Thing {}
+```
+
+    <stdin>:2:13: error: Couldn't find class 'Thing'
+    def AThing: Thing {}
+                ^
+
+
+The default value is `off`, meaning cells are connected. If you want to override the default for one cell only, use the `%reset` or `%noreset` magic. These always override the default.
+
+
+```tablegen
+class Thing {}
+```
+
+    ------------- Classes -----------------
+    class Thing {
+    }
+    ------------- Defs -----------------
+
+
+
+```tablegen
+%noreset
+// This works because of the noreset above.
+def AThing: Thing {}
+```
+
+    ------------- Classes -----------------
+    class Thing {
+    }
+    ------------- Defs -----------------
+    def AThing {  // Thing
+    }
+
+
+
+```tablegen
+// This does not because we're not changing the default.
+def AnotherThing: Thing {}
+```
+
+    <stdin>:2:19: error: Couldn't find class 'Thing'
+    def AnotherThing: Thing {}
+                      ^
+
+
+
+```tablegen
+%config cellreset off
+%reset
+// Here we have an empty cache and default reset behaviour.
+```
+
+    ------------- Classes -----------------
+    ------------- Defs -----------------
+
+
+It is not valid to have `%reset` and `%noreset` in the same cell.
+
+
+```tablegen
+%reset
+%noreset
+```
+
+    %reset and %noreset in the same cell is not allowed. Use only one, or neither.
+
+Consider setting `cellreset` to the majority usecase for your notebook. For example a tutorial building a large example across many cells will likely want it `off`. One with many standalone examples, `on`.
+
 There is a "magic" directive `%args` that you can use to send command line arguments to `llvm-tblgen`.
 
 For example, here we have some code that shows a warning.
 
 
 ```tablegen
 %reset
 class Thing <int A, int B> {

llvm/utils/TableGen/jupyter/tablegen_kernel/kernel.py

 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 import os
 import shutil
 import subprocess
 import tempfile
 from ipykernel.kernelbase import Kernel
 
 __version__ = "0.0.1"
 
 
+class TableGenKernelException(Exception):
+    pass
+
+
 class TableGenKernel(Kernel):
     """Kernel using llvm-tblgen inside jupyter.
 
     All input is treated as TableGen unless the first non whitespace character
     is "%" in which case it is a "magic" line.
 
-    The supported magic is:
-    * %args - to set the arguments passed to llvm-tblgen.
-    * %reset - to reset the cached code and magic state.
+    The supported cell magic is:
+    * %args    - to set the arguments passed to llvm-tblgen.
+    * %reset   - to reset the cached code and magic state.
+    * %noreset - to not reset the cached code and magic state
+                 (useful when you have changed the default to always
+                  reset the cache).
 
     These are "cell magic" meaning it applies to the whole cell. Therefore
     it must be the first line, or part of a run of magic lines starting
     from the first line.
 
+    The following are global magic (that applies to all cells going
+    forward):
+    * %config  - to change the behaviour of the kernel overall, including
+                 changing defaults for things like resets.
+
+    Global magic must be written in the same way as cell magic.
+
     ```tablgen
     %args
     %reset
     %args --print-records --print-detailed-records
     class Stuff {
       string Name;
     }
 
def __init__(self, **kwargs):
         Kernel.__init__(self, **kwargs)
         self._executable = None
         # A list of (code, magic) tuples.
         # All the previous cell's code we have run since the last reset.
         # This emulates a persistent state like a Python interpreter would have.
         self._previous_code = ""
         # The most recent set of magic since the last reset.
         self._previous_magic = {}
+        # The default cache reset behaviour. True means do not cache anything
+        # between cells.
+        self._cell_reset = False
 
     @property
     def banner(self):
         return "llvm-tblgen kernel %s" % __version__
 
     def get_executable(self):
         """If this is the first run, search for llvm-tblgen.
         Otherwise return the cached path to it."""
         if self._executable is None:
             path = os.environ.get("LLVM_TBLGEN_EXECUTABLE")
             if path is not None and os.path.isfile(path) and os.access(path, os.X_OK):
                 self._executable = path
             else:
                 path = shutil.which("llvm-tblgen")
                 if path is None:
                     raise OSError(
                         "llvm-tblgen not found. Put it on your PATH or set the"
                         " environment variable LLVM_TBLGEN_EXECUTABLE to point to it."
                     )
                 self._executable = path
 
         return self._executable
 
+    def parse_config_magic(self, config):
+        """Config should be a list of parameters given to the %config command.
+        We allow only one setting per %config line and that setting can only
+        have one value.
+
+        Assuming the parameters are valid, update the kernel's setting with
+        the new value.
+
+        If there is an error, raise a TableGenKernelException.
+
+        >>> k.parse_config_magic([])
+        Traceback (most recent call last):
+         ...
+        TableGenKernelException: Incorrect number of parameters to %config. Expected %config <setting> <value>.
+        >>> k._cell_reset
+        False
+        >>> k.parse_config_magic(["a", "b", "c"])
+        Traceback (most recent call last):
+         ...
+        TableGenKernelException: Incorrect number of parameters to %config. Expected %config <setting> <value>.
+        >>> k.parse_config_magic(["notasetting", "..."])
+        Traceback (most recent call last):
+         ...
+        TableGenKernelException: Unknown kernel setting "notasetting". Possible settings are: "cellreset".
+        >>> k.parse_config_magic(["cellreset", "food"])
+        Traceback (most recent call last):
+         ...
+        TableGenKernelException: Invalid value for setting "cellreset", expected "on" or "off".
+        >>> k.parse_config_magic(["cellreset", "on"])
+        >>> k._cell_reset
+        True
+        >>> k.parse_config_magic(["cellreset", "off"])
+        >>> k._cell_reset
+        False
+        """
+        if len(config) != 2:
+            raise TableGenKernelException(
+                "Incorrect number of parameters to %config. Expected %config <setting> <value>."
+            )
+
+        name, value = config
+        if name != "cellreset":
+            raise TableGenKernelException(
+                'Unknown kernel setting "{}". '
+                'Possible settings are: "cellreset".'.format(name)
+            )
+
+        try:
+            self._cell_reset = {"on": True, "off": False}[value.lower()]
+        except KeyError:
+            raise TableGenKernelException(
+                'Invalid value for setting "{}", '
+                'expected "on" or "off".'.format(name)
+            )
+
     def get_magic(self, code):
         """Given a block of code remove the magic lines from it.
         Returns a tuple of newline joined code lines and a dictionary of magic.
         Where the key is the magic name (minus the %) and the values are lists
         of the arguments to the magic.
 
         Currently we only look for "cell magic" which must be at the start of
         the cell. Meaning the first line, or a set of lines beginning with %
def get_magic(self, code):
                 # Key has the % removed
                 magic[magic_parts[0][1:]] = magic_parts[1:]
             else:
                 code_lines = [line, *lines]
                 break
 
         return "\n".join(code_lines), magic
 
+    def should_reset(self, magic):
+        """Return true if we should reset the cache, based on the default
+        setting and the current cell's magic %reset and/or %noreset.
+
+        >>> k._cell_reset = False
+        >>> k.should_reset({})
+        False
+        >>> k.should_reset({'reset': [], 'noreset': []})
+        Traceback (most recent call last):
+        ...
+        TableGenKernelException: %reset and %noreset in the same cell is not allowed. Use only one, or neither.
+        >>> k.should_reset({'reset': []})
+        True
+        >>> k.should_reset({'noreset': []})
+        False
+        >>> k._cell_reset = True
+        >>> k.should_reset({})
+        True
+        >>> k.should_reset({'reset': [], 'noreset': []})
+        Traceback (most recent call last):
+        ...
+        TableGenKernelException: %reset and %noreset in the same cell is not allowed. Use only one, or neither.
+        >>> k.should_reset({'reset': []})
+        True
+        >>> k.should_reset({'noreset': []})
+        False
+        """
+        # Cell reset is the default unless told otherwise.
+        should_reset = self._cell_reset
+        # Magic reset commands always win if present.
+        reset = magic.get("reset") is not None
+        noreset = magic.get("noreset") is not None
+
+        if reset and not noreset:
+            should_reset = True
+        elif noreset and not reset:
+            should_reset = False
+        elif noreset and reset:
+            raise TableGenKernelException(
+                "%reset and %noreset in the same cell is not allowed. Use only one, or neither."
+            )
+        # else neither are set so use the default.
+
+        return should_reset
+
     def get_code_and_args(self, new_code):
         """Get the code that do_execute should use, taking into account
         the code from any cached cells.
 
         Returns the code to compile and the arguments to use to do so.
 
         >>> k._previous_code = ""
         >>> k._previous_magic = {}
         >>> k.get_code_and_args("")
         ('', [])
         >>> k.get_code_and_args("%args 1\\nSome code")
         ('Some code', ['1'])
         >>> k.get_code_and_args("%args 2\\nSome more code")
         ('Some code\\nSome more code', ['2'])
         >>> k.get_code_and_args("%reset\\n%args 3 4\\nSome new code")
         ('Some new code', ['3', '4'])
         >>> k.get_code_and_args("%reset\\nSome new code")
         ('Some new code', [])
         """
         new_code, new_magic = self.get_magic(new_code)
 
-        # Only a reset in the newest cell actually does a reset.
-        if new_magic.get("reset") is not None:
+        # Update kernel configuration first, if needed.
+        config_magic = new_magic.get("config")
+        if config_magic is not None:
+            self.parse_config_magic(config_magic)
+
+        if self.should_reset(new_magic):
             self._previous_code = new_code
             self._previous_magic = new_magic
         else:
             self._previous_code += ("\n" if self._previous_code else "") + new_code
             self._previous_magic.update(new_magic)
 
         return self._previous_code, self._previous_magic.get("args", [])
 
class TableGenKernel(Kernel):
 
     def send_stdout(self, stdout):
         return self.send_stream("stdout", stdout)
 
     def do_execute(
         self, code, silent, store_history=True, user_expressions=None, allow_stdin=False
     ):
         """Execute user code using llvm-tblgen binary."""
+        try:
             all_code, args = self.get_code_and_args(code)
+        except TableGenKernelException as e:
+            return self.send_stderr(str(e))
 
         # If we cannot find llvm-tblgen, propogate the error to the notebook.
         # (in case the user is not able to see the output from the Jupyter server)
         try:
             executable = self.get_executable()
         except Exception as e:
             return self.send_stderr(str(e))