-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Added region aware testing #50
Conversation
@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks mostly fine. I left some comments about code that needs to be cleaned up.
@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 |
Yes, it refers to updating the |
OK, can you tick that checkbox, or is there still stuff missing? |
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. |
Ah, so you can't get a graph back from YAML that contains region blocks?
Yes, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One additional question, is the backedges
is empty anyway, can we add code to omit that in input YAML?
edges = {} | ||
backedges = {} | ||
|
||
def reverse_lookup(value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a type signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value
s are class constructors, not sure what type that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct signature for this would be type[BasicBlock]
numba_rvsdg/tests/test_scfg.py
Outdated
"0": ["1", "2"], | ||
"1": ["5"], | ||
"2": ["1", "5"], | ||
"3": ["1"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this case, it goes from 0
to 1
in the diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #50 (comment) except in this case since graphs are random it doesn't matter where they point.
'2': ['1', '4'] | ||
'3': ['0'] | ||
'4': [] | ||
backedges: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two mores suggestions.
@kc611 thank you for submitting this. I made two reviews with some sugggestions. Also, please don't forget #50 (comment) |
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:
But they do seem to pass? |
Co-authored-by: esc <[email protected]>
…nching properties
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:
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.
I pushed this change with 42434fd |
|
||
seen = set() | ||
|
||
def make_scfg(curr_heads: set, exiting: str = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should not be a closure, unless it really needs to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to write this such that it isn't a closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_dict
to 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.
@@ -872,6 +966,33 @@ def view(self, name: str = None): | |||
SCFGRenderer(self).view(name) | |||
|
|||
|
|||
def find_outer_graph(graph_dict: dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstring please.
return outer_blocks | ||
|
||
|
||
def extract_block_info(blocks, current_name, block_ref_dict, edges, backedges): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstring too please.
As titled, this PR adds region-aware testing by modifying the
from_yaml
andto_yaml
methods ofSCFG
.Following is the task list for this PR:
from_yaml
andto_yaml
to new format.fig3
andfig4