[Spike] perform static cyclic dependency detection before each graph run #11940

aparajit-pratap · 2021-08-17T04:42:21Z

Purpose

This adds the capability to perform a static cyclic dependency detection at the Dynamo graph level before each graph run (delta execution). If a cycle is detected, other than reporting node warnings on the nodes that are participating in the cycle, it prevents the graph from executing in the first place and getting into a warning state thus saving on some execution cycles (pun unintended).

Pros

Potential performance improvement as we save on unnecessary graph execution

Cons

If there is a sub-graph (disconnected graph) that is not involved in a cycle, even then it will skip execution, which is a regression in current behavior. This can be improved by still running that portion of the graph selectively. I've added this point to the list of ToDo's.

Note:
We cannot remove the existing cyclic dependency checks from codegen as they are still required to detect cycles in DS code in CBNs. In the absence of this check, CBN code cycles if present will not be reported to users. An example of such a code is the following where there is a clear cycle in the code. This cannot be detected by the graph level check implemented in this spike.

Profiling Results:

Note: The profiling assumes we can do away with the codegen check for cyclic dependency but as mentioned, we cannot do so yet.

Profiling was carried out on a D4R/GD graph for graph execution only. It uses the following packages.

Before:
Notice that just the cyclic dependency check at codegen takes nearly 1800ms or about 11.3% of the total time. This is saved after this new approach.

After:
Savings of about 1300 ms or about 8% improvement overall.

TODO:

Check if cyclic dep checks can be removed from codegen - as described above, this cannot be done yet as the graph level cyclic dep check is insufficient to detect cycles in CBN DS code.
Check if fixes made in PR Dyn-3640 - Recover execution after clearing cyclic dependency from graph #11896 are still necessary - these would still be necessary to clear those errors that are detected by the codegen check.
Profile existing compile-time cyclic dep checks (+ execution) in engine vs. checks in this PR
Code/tech debt removal if the above are found to be true - DYN-3925
Check if we can run only those portions of the graph that are unaffected by a cycle - selective execution in other words (DYN-3926)
Fix regressions/update tests - some tests need to be updated as this PR follows a different code path when it comes to dealing with cyclic dependencies; the biggest change being that the graph execution is skipped altogether.
There is one unresolved regression: Dynamo.Tests.DelayedGraphExecutionTests.DelayedExecutionNodeToCodeTest. It is still a mystery as no circular dependency is found on running the test graph in Dynamo, however, there is one found when running the test. This needs to be investigated further.
Investigate a way to perform the cyclic dependency check in codegen only for ASTs corresponding to CBNs in a graph thereby making it possible to still leverage the perf benefits and simplicity of the graph-based analysis in this PR for the rest of the dynamo graph - https://jira.autodesk.com/browse/DYN-3954

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

FYIs

@saintentropy

mjkkirschner · 2021-08-17T13:01:49Z

src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs

+ // Detect cyclic dependencies in graph
+ var cyclicNodes = new List<NodeModel>();
+
+ foreach (var node in ModifiedNodes)


hmm, so this appears that it will start multiple DFS searches from each modified node - on opening a new graph all nodes will be modified so this has the possibility of recomputing the same information multiple times -and could have a performance like O(n(n+v)) if I've got that right.

can't each invocation of DFS share the visited data structure? If one DFS has already computed this subtree of the graph whats the reason to recompute it?

@mjkkirschner, you're right. I made a mistake earlier. I was initializing the visited and recursion lists for each modified node. I have now moved their initialization outside the for loop so it's done only once. That way if a node is already visited it will be skipped. The overall complexity of this algo is O(v+e). The reason why I'm running this on all nodes as there could be disconnected portions of the graph and I am trying to collect all nodes involved in a cycle and report warnings on those.

The good thing is this will run on all nodes only the first time a graph is opened and executed but subsequently, it will only run on the modified nodes for each delta execution.

pinzart90 · 2021-08-17T14:15:51Z

src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs

+ /// <param name="recursionTracker"></param>
+ /// <param name="cyclicNodes">list of all nodes participating in a cycle</param>
+ /// <returns>true if graph is cyclic, false otherwise</returns>
+ private bool IsGraphCyclic(NodeModel node, HashSet<NodeModel> visitTracker, 


is there no other similar method already implemented that we could reuse (optionally enhance)?

There is a better algorithm used called StronglyConnectedComponent but it is used at a lower, graphnode level in the engine. We could probably use it for this too at the dynamo nodemodel level but we'll need to generalize (templatize) it to work for any graph vertex, which will be tricky as getting the adjacent nodes for each type is very different. It would be easier to rewrite it to make it work specifically with NodeModel type.

My first goal here was to establish that this idea could work in general, we can always refine the exact algo later and make it more efficient, which at the moment, I can think of the stronglyconnectedcomponent algo.

Ok
Is this supposed to detect all cycles or just the first one ?

I think this will detect only one cycle for a given node but the outer method call will record at most one cycle for all nodes and report warnings on all of them.

sm6srw · 2021-08-19T12:59:44Z

src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs

+ if(warning.ID == WarningID.InvalidStaticCyclicDependency && 
+ node.GUID == warning.GraphNodeGuid)
+ {
+ node.ClearErrorsAndWarnings();


So these warnings are from a previous execution when the graph was in a cyclic state?

Yes, if a cyclic warning is present for this node from a previous execution and the graph is found to be acyclic for the node in the current execution, the warning will be cleared for that node.

ClearErrorsAndWarnings will clear everything (all previous persistent warnings or errors)
Do you think there might be a case where older (persistent) errors should not be cleared ?

Yes, it will clear everything but I'm reducing the surface area of it clearing warnings only when the node in question had a cyclic warning before, but you're right, this will still clear any and all other warnings that the node previously had as well. However, I can't think of a scenario where that would do any harm as I'm relying on the fact that the same warnings should reappear if the node is re-executed.

Most of the internal (not created by API users) are indeed mostly regenerated when re executing. We do have example of non execution related warnings (like the Gizmo warning)
I guess the issue would be if API users create warnings that do not get recreated by re execution
Not a big issue though...warning infrastructure does not support it yet

pinzart90

I have not looked at the cycle detection algorithm too much (because we said we're going to look into stronglyconnectedcomponent later on)
Asides from that, LGTM

mjkkirschner · 2021-08-25T20:53:54Z

src/Engine/ProtoCore/BuildStatus.cs

+ {
+#if DEBUG
+
+ Console.WriteLine("Warning:{0}", message);


you can use System.Diagnostics.Debug.Writeline I think to avoid the conditional compilation if you want - I think that call does not end up logging in Release builds.

pinzart90 · 2021-08-26T13:44:59Z

@aparajit-pratap can we run the codegen cycle detection only for cbns ?
The static cycle dep could then still be a benefit (assuming it cannot parse ds code at that time)

aparajit-pratap · 2021-08-26T16:31:20Z

@aparajit-pratap can we run the codegen cycle detection only for cbns ?
The static cycle dep could then still be a benefit (assuming it cannot parse ds code at that time)

@pinzart90 codegen is performed on the entire AST that is compiled for the entire graph. At the moment I don't think there is a way to detect which AST's correspond to CBNs vs. those that don't although that might be something that is possible to implement. I can make this a follow-up task if we wish to proceed with this spike at some stage. Thanks for the suggestion.

perform static cyclic dependency detection before each graph run

e90e2fc

aparajit-pratap changed the title ~~perform static cyclic dependency detection before each graph run~~ [Spike] perform static cyclic dependency detection before each graph run Aug 17, 2021

aparajit-pratap requested review from mjkkirschner, pinzart and sm6srw August 17, 2021 04:50

code comments

b50785c

mjkkirschner reviewed Aug 17, 2021

View reviewed changes

review comment

5254f48

pinzart90 reviewed Aug 17, 2021

View reviewed changes

clear only cyclic dep warnings

91620e1

sm6srw reviewed Aug 19, 2021

View reviewed changes

aparajit-pratap added 3 commits August 24, 2021 15:35

update tests

1f86301

remove cyclic graphs as the tests will not apply to them anymore

c526ce7

comment out dead code

20b5df7

pinzart90 approved these changes Aug 25, 2021

View reviewed changes

aparajit-pratap added PTAL Please Take A Look 👀 DNM Do not merge. For PRs. labels Aug 25, 2021

mjkkirschner reviewed Aug 25, 2021

View reviewed changes

aparajit-pratap marked this pull request as draft August 26, 2021 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spike] perform static cyclic dependency detection before each graph run #11940

[Spike] perform static cyclic dependency detection before each graph run #11940

aparajit-pratap commented Aug 17, 2021 •

edited

mjkkirschner Aug 17, 2021

aparajit-pratap Aug 17, 2021

pinzart90 Aug 17, 2021

aparajit-pratap Aug 17, 2021

aparajit-pratap Aug 17, 2021

pinzart90 Aug 17, 2021

aparajit-pratap Aug 17, 2021 •

edited

sm6srw Aug 19, 2021

aparajit-pratap Aug 19, 2021

pinzart90 Aug 24, 2021

aparajit-pratap Aug 24, 2021

pinzart90 Aug 24, 2021

pinzart90 left a comment

mjkkirschner Aug 25, 2021 •

edited

pinzart90 commented Aug 26, 2021

aparajit-pratap commented Aug 26, 2021 •

edited

[Spike] perform static cyclic dependency detection before each graph run #11940

Are you sure you want to change the base?

[Spike] perform static cyclic dependency detection before each graph run #11940

Conversation

aparajit-pratap commented Aug 17, 2021 • edited

Purpose

Pros

Cons

Profiling Results:

Declarations

FYIs

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

aparajit-pratap Aug 17, 2021 • 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

pinzart90 left a comment

Choose a reason for hiding this comment

mjkkirschner Aug 25, 2021 • edited

Choose a reason for hiding this comment

pinzart90 commented Aug 26, 2021

aparajit-pratap commented Aug 26, 2021 • edited

aparajit-pratap commented Aug 17, 2021 •

edited

aparajit-pratap Aug 17, 2021 •

edited

mjkkirschner Aug 25, 2021 •

edited

aparajit-pratap commented Aug 26, 2021 •

edited