[WIP] Added region aware testing by kc611 · Pull Request #50 · numba/numba-scfg

kc611 · 2023-04-24T20:20:40Z

As titled, this PR adds region-aware testing by modifying the from_yaml and to_yaml methods of SCFG.

Following is the task list for this PR:

Refactor from_yaml and to_yaml to new format.
Make the methods block-type aware
Make the methods region aware
Adapt the test suite to the new methods
Add the capability to build graphs using the YAML format
Add a comparator for testing the graph accurately.
Test the logic by adding automated tests for fig3 and fig4

…rmat

…to_dict

esc · 2023-06-30T09:59:39Z

Add capability to build graphs using the YAML format

@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to?

esc

It looks mostly fine. I left some comments about code that needs to be cleaned up.

esc · 2023-06-30T10:02:38Z

@kc611 also, The synthetic assignments need to save, which variable is being assigned. Also the synthetic branches and such need to store their table of variable -> destination. This is part of the graph and semantics so needs to be stores inside the YAML.

kc611 · 2023-06-30T10:05:19Z

Add capability to build graphs using the YAML format

@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to?

Yes, it refers to updating the from_yaml and from_dict methods for constructing SCFGs using a given YAML.

esc · 2023-06-30T10:06:16Z

Add capability to build graphs using the YAML format

@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to?

Yes, it refers to updating the from_yaml and from_dict methods for constructing SCFGs using a given YAML.

OK, can you tick that checkbox, or is there still stuff missing?

kc611 · 2023-06-30T10:09:42Z

It is still missing support for building region blocks.

I have updated the description to include the final goal of this PR i.e. to get an automated test for transformed SCFGs which in our case is fig3 and fig4.

esc · 2023-06-30T10:13:12Z

It is still missing support for building region blocks.

Ah, so you can't get a graph back from YAML that contains region blocks?

I have updated the description to include the final goal of this PR i.e. to get an automated test for transformed SCFGs which in our case is fig3 and fig4.

Yes, and test_scc.py too.

esc

One additional question, is the backedges is empty anyway, can we add code to omit that in input YAML?

esc · 2023-07-05T12:43:08Z

+        edges = {}
+        backedges = {}
+
+        def reverse_lookup(value):


this needs a type signature

The values are class constructors, not sure what type that is.

I think the correct signature for this would be type[BasicBlock]

esc · 2023-07-05T12:49:05Z

+                    "0": ["1", "2"],
+                    "1": ["5"],
+                    "2": ["1", "5"],
+                    "3": ["1"],


Can you explain this case, it goes from 0 to 1 in the diff?

Same as #50 (comment) except in this case since graphs are random it doesn't matter where they point.

esc · 2023-07-05T12:52:20Z

+            '2': ['1', '4']
+            '3': ['0']
+            '4': []
+        backedges:


I think this is wrong. The original should never have any backedges. They are just edges to begin with. Part of the SCFG loop restructure is to go through and make the distinction between edges and backedges.

The issue is, if there's an edge (that is not a back edge) pointing towards the head of the graph, then we have no way of automatically detecting which node is the head of the graph from YAML.

Since our code logic behaves as follows:
The head of the graph is the node towards which no edge is pointing towards.

To resolve this issue for this example we can simply have a placeholder head with no edges pointing towards it that in turn points towards the actual graph we need, that'll remove the need for back edges.

This is also an offshoot variant of the issue faced by @sklam downstream in Numba, wherein he circumvented it by having a separate placeholder head and tail for every RegionBlock. Seems like that could be upstreamed here to fix it in the long run.

I am not convinced that this is a good implementation. I think it should be possible to specify a cyclic graph in YAML and read it in. I think I can see a way to eliminate the need to detect a head. In general, I think cyclic, but reducible graphs should be supported.

esc

Two mores suggestions.

esc · 2023-07-05T12:58:48Z

@kc611 thank you for submitting this. I made two reviews with some sugggestions. Also, please don't forget #50 (comment)

esc · 2023-07-05T13:02:12Z

Also, I am not sure that the values of the control variables are being tested. Using the following diff, I was expecting the tests to fail:

diff --git i/numba_rvsdg/tests/test_figures.py w/numba_rvsdg/tests/test_figures.py
index 82419f1d69..4e86d088d8 100644
--- i/numba_rvsdg/tests/test_figures.py
+++ w/numba_rvsdg/tests/test_figures.py
@@ -144,7 +144,7 @@ blocks:
         end: 22
     synth_asign_block_0:
         type: synth_asign
-        variable_assignment: {'control_var_0': 0}
+        variable_assignment: {'control_var_0': 1}
     synth_asign_block_1:
         type: synth_asign
         variable_assignment: {'control_var_0': 1}

But they do seem to pass?

As title

Co-authored-by: esc <esc@users.noreply.github.com>

…nching properties

kc611 · 2023-07-06T14:32:42Z

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

esc · 2023-07-11T16:02:42Z

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

I think there are some fixes for this exact issue in: #57 -- I will investigate porting those fixes to this PR.

esc · 2023-07-12T05:55:53Z

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

I think there are some fixes for this exact issue in: #57 -- I will investigate porting those fixes to this PR.

This patch "seems" to fix things. I've run it several times now and I was unable to reproduce the error:

diff --git i/numba_rvsdg/core/datastructures/scfg.py w/numba_rvsdg/core/datastructures/scfg.py
index 2d29025b45..d80b99ae07 100644
--- i/numba_rvsdg/core/datastructures/scfg.py
+++ w/numba_rvsdg/core/datastructures/scfg.py
@@ -617,7 +617,7 @@ class SCFG:
             # Need to create synthetic assignments for each arc from a
             # predecessors to a successor and insert it between the predecessor
             # and the newly created block
-            for s in set(jt).intersection(successors):
+            for s in sorted(set(jt).intersection(successors)):
                 synth_assign = self.name_gen.new_block_name(SYNTH_ASSIGN)
                 variable_assignment = {}
                 variable_assignment[branch_variable] = branch_variable_value

Looking again at the patch in #57 -- there are only three changes that are not related to rendering. The above is one of them. Perhaps this is sufficient to fix the instability of the current loop restructure implementation.

This removes non-determinism from the insertion order of the SYNTH_ASSIGN blocks such that the resulting SCFG output is stable and can be tested appropriately.

esc · 2023-07-13T10:51:26Z

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

I think there are some fixes for this exact issue in: #57 -- I will investigate porting those fixes to this PR.

This patch "seems" to fix things. I've run it several times now and I was unable to reproduce the error:
diff --git i/numba_rvsdg/core/datastructures/scfg.py w/numba_rvsdg/core/datastructures/scfg.py
index 2d29025b45..d80b99ae07 100644
--- i/numba_rvsdg/core/datastructures/scfg.py
+++ w/numba_rvsdg/core/datastructures/scfg.py
@@ -617,7 +617,7 @@ class SCFG:
             # Need to create synthetic assignments for each arc from a
             # predecessors to a successor and insert it between the predecessor
             # and the newly created block
-            for s in set(jt).intersection(successors):
+            for s in sorted(set(jt).intersection(successors)):
                 synth_assign = self.name_gen.new_block_name(SYNTH_ASSIGN)
                 variable_assignment = {}
                 variable_assignment[branch_variable] = branch_variable_value
Looking again at the patch in #57 -- there are only three changes that are not related to rendering. The above is one of them. Perhaps this is sufficient to fix the instability of the current loop restructure implementation.

I pushed this change with 42434fd

esc · 2023-07-13T11:20:53Z

+
+        seen = set()
+
+        def make_scfg(curr_heads: set, exiting: str = None):


I think this should not be a closure, unless it really needs to be.

I managed to write this such that it isn't a closure.

I investigated this a bit more and I think this function will need some significant changes so as to make it more readable and that it becomes clearer what goes on. I think the make_scfg shouldn't be a closure and I also think that some stuff can be decomposed into smaller functions too.

esc

I left a few comments, but I think there is some more work to do on this:

Refactor make_scfg so that it complies more closely with general software engineering best practices (low-coupling, high cohesion)
Refactor the need for having a graph with a pre-determined "head-node" to support cyclic graphs as YAML input.

esc

I would suggest to add some docstrings to the auxilliary methods and also place those into a class SCFGIO alongside make_scfg. Then, forward the methods from_yaml/to_yaml and from_dict/to_dictto the static methods of SCFGIO class. Make sure you also update the documentation to reflect this. The methods remaining in the original SCFG class can simply have a see also: in the NumPy style.

esc · 2023-07-19T12:17:06Z

        SCFGRenderer(self).view(name)


+def find_outer_graph(graph_dict: dict):


docstring please.

esc · 2023-07-19T12:17:20Z

+    return outer_blocks
+
+
+def extract_block_info(blocks, current_name, block_ref_dict, edges, backedges):


docstring too please.

As title

esc · 2023-07-31T08:06:36Z

@kc611 looks good, thank you, I fixed two minor typos in cadd031 and it's now ready to merge.

kc611 marked this pull request as ready for review April 25, 2023 09:04

kc611 added 4 commits June 29, 2023 17:09

Added from_yaml, to_yaml, from_dict and to_dict conversion for new fo…

63e1194

…rmat

Refactored test suite to match the new functions

1d9689a

Added region identification and printing capabilities to to_yaml and …

dae773c

…to_dict

Adapted tests to the changes

0ea0e6c

kc611 force-pushed the region_testing branch from 992fad5 to 0ea0e6c Compare June 29, 2023 12:48

Fixed no head detected test failure

4213e58

esc requested changes Jun 30, 2023

View reviewed changes

Comment thread numba_rvsdg/tests/test_fig3.py Outdated

Comment thread numba_rvsdg/core/datastructures/scfg.py Outdated

esc added this to the 0.0.3 milestone Jun 30, 2023

kc611 added 4 commits July 1, 2023 11:07

Added block type identification capability to from_dict and from_yaml

1f5d955

Added yaml based block building capabilities to SCFG

ac2802e

Automated Bahmann paper figure tests

7a3b4c2

Adapted test suite to changes

e1097ad

esc requested changes Jul 5, 2023

View reviewed changes

Comment thread numba_rvsdg/tests/test_utils.py Outdated

Comment thread numba_rvsdg/tests/test_utils.py Outdated

esc and others added 4 commits July 5, 2023 17:39

refactor YAML writing to use indent for readability

fcdc9b9

As title

Apply suggestions from code review

8cd3009

Co-authored-by: esc <esc@users.noreply.github.com>

Used isinstance instead of string comparison in to_dict method

462c716

Added equality assertions for synthetic assignments and synthetic bra…

e4dd79c

…nching properties

kc611 mentioned this pull request Jul 7, 2023

Refactoring jump targets and backedges to update inplace #83

Open

fix stability of SYNTH_ASSIGN block insertion

42434fd

This removes non-determinism from the insertion order of the SYNTH_ASSIGN blocks such that the resulting SCFG output is stable and can be tested appropriately.

esc mentioned this pull request Jul 13, 2023

Refactor numba_rvsdg/tests/test_figures.py #85

Open

esc reviewed Jul 13, 2023

View reviewed changes

esc requested changes Jul 13, 2023

View reviewed changes

kc611 added 2 commits July 17, 2023 16:06

Refactored make_scfg to make it more readable

851c03a

Added support for cyclic graph creation

d0a0fa6

esc requested changes Jul 19, 2023

View reviewed changes

kc611 and others added 2 commits July 20, 2023 18:27

Added SCFGIO class

7b71b2c

fix typos

cadd031

As title

esc approved these changes Jul 31, 2023

View reviewed changes

esc merged commit b10fb94 into numba:main Jul 31, 2023


		seen = set()

		def make_scfg(curr_heads: set, exiting: str = None):

		SCFGRenderer(self).view(name)


		def find_outer_graph(graph_dict: dict):

		return outer_blocks


		def extract_block_info(blocks, current_name, block_ref_dict, edges, backedges):

Conversation

kc611 commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esc commented Jun 30, 2023

Uh oh!

esc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

esc commented Jun 30, 2023

Uh oh!

kc611 commented Jun 30, 2023

Uh oh!

esc commented Jun 30, 2023

Uh oh!

kc611 commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esc commented Jun 30, 2023

Uh oh!

esc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

esc commented Jul 5, 2023

Uh oh!

esc commented Jul 5, 2023

Uh oh!

kc611 commented Jul 6, 2023

Uh oh!

esc commented Jul 11, 2023

Uh oh!

esc commented Jul 12, 2023

Uh oh!

esc commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kc611 commented Apr 24, 2023 •

edited

Loading

kc611 commented Jun 30, 2023 •

edited

Loading

esc left a comment •

edited

Loading