Rework python script_instrumentor.py and test

.github/workflows/api_tests.yml

@@ -60,7 +60,7 @@ jobs:
           flags: ruby-api # See codecov.yml
           token: ${{ secrets.CODECOV_TOKEN }}
 
-  script-runner-api:
+  script-runner-api-ruby:
     if: ${{ github.actor != 'dependabot[bot]' }}
     runs-on: ubuntu-latest
     strategy:
@@ -90,3 +90,36 @@ jobs:
           directory: openc3-cosmos-script-runner-api/coverage
           flags: ruby-api # See codecov.yml
           token: ${{ secrets.CODECOV_TOKEN }}
+
+  script-runner-api-python:
+    if: ${{ github.actor != 'dependabot[bot]' }}
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        python-version: ["3.10", "3.11"]
+
+    steps:
+      - uses: actions/checkout@v4
+      - name: Set up Python ${{ matrix.python-version }}
+        uses: actions/setup-python@v5
+        with:
+          python-version: ${{ matrix.python-version }}
+      - name: Install dependencies
+        run: |
+          python -m pip install --upgrade pip
+          pip install -r requirements-dev.txt
+        working-directory: openc3-cosmos-script-runner-api
+      - name: Lint with ruff
+        run: |
+          ruff --format=github scripts/*.py
+        working-directory: openc3-cosmos-script-runner-api
+      - name: Run unit tests
+        run: |
+          coverage run -m pytest ./test/
+          coverage xml -i
+        working-directory: openc3-cosmos-script-runner-api
+      - uses: codecov/codecov-action@v5
+        with:
+          working-directory: openc3/python
+          flags: python # See codecov.yml
+          token: ${{ secrets.CODECOV_TOKEN }}

openc3-cosmos-script-runner-api/scripts/running_script.py

@@ -489,6 +489,7 @@ def instrument_script(cls, text, filename):
 
         parsed = ast.parse(text)
         tree = ScriptInstrumentor(filename).visit(parsed)
+        # Normal Python code is run with mode='exec' whose root is ast.Module
         result = compile(tree, filename=filename, mode="exec")
         return result
 
@@ -1158,10 +1159,13 @@ def handle_exception(
             self.mark_error()
             self.wait_for_go_or_stop_or_retry(exc_value)
 
+        # See script_instrumentor.py for how retry is used
         if self.retry_needed:
             self.retry_needed = False
+            # Return True to the instrumented code to retry the line
             return True
         else:
+            # Return False to the instrumented code to break the while loop
             return False
 
     def load_file_into_script(self, filename):

openc3-cosmos-script-runner-api/scripts/script_instrumentor.py

@@ -17,6 +17,15 @@
 import ast
 
 
+# For details on the AST, see https://docs.python.org/3/library/ast.html
+# and https://greentreesnakes.readthedocs.io/en/latest/nodes.html
+
+# This class is used to instrument a Python script with calls to a
+# RunningScript instance. The RunningScript instance is used to
+# track the execution of the script, and can be used to pause and
+# resume the script. We inherit from ast.NodeTransformer, which
+# allows us to modify the AST of the script.  We override the visit
+# method for each type of node that we want to instrument.
 class ScriptInstrumentor(ast.NodeTransformer):
     pre_line_instrumentation = """
 RunningScript.instance.pre_line_instrumentation('{}', {}, globals(), locals())
@@ -38,93 +47,162 @@ def __init__(self, filename):
         self.filename = filename
         self.in_try = False
 
-    # These are statements which should have an enter and leave
-    # (In retrospect, this isn't always true, eg, for 'if')
-    def track_enter_leave_lineno(self, node):
+    # What we're trying to do is wrap executable statements in a while True try/except block
+    # For example if the input code is "print('HI')", we want to transform it to:
+    # while True:
+    #     try:
+    #         RunningScript.instance.pre_line_instrumentation('myfile.py', 1, globals(), locals())
+    #     --> print('HI') <-- This is the original node
+    #         break
+    #     except:
+    #         retry_needed = RunningScript.instance.exception_instrumentation('myfile.py', 1)
+    #         if retry_needed:
+    #             continue
+    #         else:
+    #             break
+    #     finally:
+    #         RunningScript.instance.post_line_instrumentation('myfile.py', 1)
+    # This allows us to retry statements that raise exceptions
+    def track_enter_leave(self, node):
+        # Determine if we're in a try block
         in_try = self.in_try
         if not in_try and type(node) in (ast.Try, ast.TryStar):
             self.in_try = True
+        # Visit the children of the node
         node = self.generic_visit(node)
         if not in_try and type(node) in (ast.Try, ast.TryStar):
             self.in_try = False
-        enter = ast.parse(
+        # ast.parse returns a module, so we need to extract
+        # the first element of the body which is the node
+        pre_line = ast.parse(
             self.pre_line_instrumentation.format(self.filename, node.lineno)
         ).body[0]
-        leave = ast.parse(
+        post_line = ast.parse(
             self.post_line_instrumentation.format(self.filename, node.lineno)
         ).body[0]
         true_node = ast.Constant(True)
         break_node = ast.Break()
-        for new_node in (enter, leave, true_node, break_node):
+        for new_node in (pre_line, post_line, true_node, break_node):
+            # Copy source location from the original node to our new nodes
             ast.copy_location(new_node, node)
 
-        # This is the code for "if 1: ..."
-        inhandler = ast.parse(
+        # Create the exception handler code node. This results in multiple nodes
+        # because we have a top level assignment and if statement
+        exception_handler = ast.parse(
             self.exception_instrumentation.format(self.filename, node.lineno)
         ).body
-        for new_node in inhandler:
+        for new_node in exception_handler:
             ast.copy_location(new_node, node)
+            # Recursively yield the children of the new_node and copy in source locations
+            # It's actually surprising how many nodes are nested in the new_node
             for new_node2 in ast.walk(new_node):
                 ast.copy_location(new_node2, node)
-        excepthandler = ast.ExceptHandler(expr=None, name=None, body=inhandler)
+        # Create an exception handler node to wrap the exception handler code
+        excepthandler = ast.ExceptHandler(type=None, name=None, body=exception_handler)
         ast.copy_location(excepthandler, node)
+        # If we're not already in a try block, we need to wrap the node in a while loop
         if not self.in_try:
             try_node = ast.Try(
-                body=[enter, node, break_node],
+                # pre_line is the pre_line_instrumentation, node is the original node
+                # and if the code is executed without an exception, we break
+                body=[pre_line, node, break_node],
+                # Pass in the handler we created above
                 handlers=[excepthandler],
+                # No else block
                 orelse=[],
-                finalbody=[leave],
+                # The try / except finally block is the post_line_instrumentation
+                finalbody=[post_line],
             )
             ast.copy_location(try_node, node)
             while_node = ast.While(test=true_node, body=[try_node], orelse=[])
             ast.copy_location(while_node, node)
             return while_node
+        # We're already in a try block, so we just need to wrap the node in a try block
         else:
             try_node = ast.Try(
-                body=[enter, node],
+                body=[pre_line, node],
                 handlers=[],
                 orelse=[],
-                finalbody=[leave],
+                finalbody=[post_line],
             )
             ast.copy_location(try_node, node)
             return try_node
 
-    visit_Assign = track_enter_leave_lineno
-    visit_AugAssign = track_enter_leave_lineno
-    visit_Delete = track_enter_leave_lineno
-    visit_Print = track_enter_leave_lineno
-    visit_Assert = track_enter_leave_lineno
-    visit_Import = track_enter_leave_lineno
-    visit_ImportFrom = track_enter_leave_lineno
-    visit_Exec = track_enter_leave_lineno
-    # Global
-    visit_Expr = track_enter_leave_lineno
-
-    # These statements can be reached, but they change
-    # control flow and are never exited.
-    def track_reached_lineno(self, node):
+    # Call the pre_line_instrumentation ONLY and then exceute the node
+    def track_reached(self, node):
+        # Determine if we're in a try block, this is used by track_enter_leave
+        in_try = self.in_try
+        if not in_try and type(node) in (ast.Try, ast.TryStar):
+            self.in_try = True
+
+        # Visit the children of the node
         node = self.generic_visit(node)
-        reach = ast.parse(
+        pre_line = ast.parse(
             self.pre_line_instrumentation.format(self.filename, node.lineno)
         ).body[0]
-        ast.copy_location(reach, node)
+        ast.copy_location(pre_line, node)
 
-        n = ast.Num(n=1)
+        # Create a simple constant node with the value 1 that we can use with our If node
+        n = ast.Constant(value=1)
         ast.copy_location(n, node)
-        if_node = ast.If(test=n, body=[reach, node], orelse=[])
+        # The if_node is effectively a noop that holds the preline & node that we need to execute
+        if_node = ast.If(test=n, body=[pre_line, node], orelse=[])
         ast.copy_location(if_node, node)
         return if_node
 
-    visit_With = track_reached_lineno
-    visit_FunctionDef = track_reached_lineno
-    visit_ClassDef = track_reached_lineno
-    visit_For = track_reached_lineno
-    visit_While = track_reached_lineno
-    visit_If = track_reached_lineno
-    visit_Try = track_reached_lineno
-    visit_TryStar = track_reached_lineno
-    visit_Pass = track_reached_lineno
-    visit_Return = track_reached_lineno
-    visit_Raise = track_enter_leave_lineno
-    visit_Break = track_reached_lineno
-    visit_Continue = track_reached_lineno
+    # Notes organized (including newlines) per https://docs.python.org/3/library/ast.html#abstract-grammar
+    # Nodes that change control flow are processed by track_reached, otherwise we track_enter_leave
+    visit_FunctionDef = track_reached
+    visit_AsyncFunctionDef = track_reached
+
+    visit_ClassDef = track_reached
+    visit_Return = track_reached
+
+    visit_Delete = track_enter_leave
+    visit_Assign = track_enter_leave
+    visit_TypeAlias = track_enter_leave
+    visit_AugAssign = track_enter_leave
+    visit_AnnAssign = track_enter_leave
+
+    visit_For = track_reached
+    visit_AsyncFor = track_reached
+    visit_While = track_reached
+    visit_If = track_reached
+    visit_With = track_reached
+    visit_AsyncWith = track_reached
+
+    # We can track the match statement but not any of the case statements
+    # because they must come unaltered after the match statement
+    visit_Match = track_reached
+
+    visit_Raise = track_enter_leave
+    visit_Try = track_reached
+    visit_TryStar = track_reached
+    visit_Assert = track_enter_leave
+
+    visit_Import = track_enter_leave
+    visit_ImportFrom = track_enter_leave
+
+    visit_Global = track_enter_leave
+    visit_Nonlocal = track_enter_leave
+    visit_Expr = track_enter_leave
+    visit_Pass = track_reached
+    visit_Break = track_reached
+    visit_Continue = track_reached
+
+    # expr nodes: mostly subnodes in assignments or return statements
+    # TODO: Should we handle the following:
+    # visit_NamedExpr = track_enter_leave
+    # visit_Lambda = track_enter_leave
+    # visit_IfExp = track_enter_leave
+    # visit_Await = track_reached
+    # visit_Yield = track_reached
+    # visit_YieldFrom = track_reached
+    # visit_Call = track_reached
+    # visit_JoinedStr = track_enter_leave
+    # visit_Constant = track_enter_leave
+
+    # All the expr_context, boolop, operator, unaryop, cmpop nodes are not modified
+    # ExceptHandler must follow try or tryStar so don't modify it
+    # Can't modify any of pattern nodes (case) because they have to come unaltered after match
+    # Ignore the type_ignore and type_param nodes