Dyn 3477 - Avoid unexpected DS function-not-found errors in CBNs, invalidate function definitions on opening new workspace #12002

aparajit-pratap · 2021-09-01T03:25:03Z

Purpose

This PR fixes a bug where when a graph containing more than one CBN is opened, where one of those CBNs contains a function definition, while the other contains a function call, there can be a function not found warning if the CBN with the function call is pre-compiled first. This is obviously unexpected for the user as the function definition actually exists in another CBN.

This is fixed by splitting the pre-compilation of CBNs into two passes. The first pass parses and compiles the CBN like normal except that in the first pass any function not found warnings are skipped. In the second pass of pre-compilation, the CBN code (except for function definitions) is compiled again turning back on those warnings as in the second pass it needs to take into account any function definitions that may have already been added to library management core in the first pass. Only after the second pass any function not found warnings will be real as by this time all function definitions should have been taken into account in the first pass.

Note that this two-stage pre-compilation of CBNs only needs to happen on opening a workspace, not in other cases of CBN creation.

Additionally, any function definitions in CBNs from a previous workspace are invalidated (procedureNode is marked inactive) upon opening a new workspace so that they no longer continue to show up in autocomplete, and function not found warnings appear if an attempt is made to call them.

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. - ~~all tests pass with the exception of Dynamo.Tests.CoreTests.TestHasUnsavedChangesOnNodeRemovalEdgeCase, a fix is in progress ...~~
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

…ialized

pinzart90 · 2021-09-10T18:47:32Z

src/DynamoCore/Models/DynamoModel.cs

+            CustomNodeManager.AddUninitializedCustomNodesInPath(Path.GetDirectoryName(filePath), IsTestMode);
+
+            var currentHomeSpace = Workspaces.OfType<HomeWorkspaceModel>().FirstOrDefault();
+            currentHomeSpace.UndefineCBNFunctionDefinitions();


why do we need to undefine func definitions before we call ReCompileCodeBlockNodesForFunctionDefinitions ?

We need to undefine the function definitions if any in the previous workspace before we open a new workspace.

ReCompileCodeBlockForFunctionDefinitions will be called after new function definitions have been compiled once the new workspace has been parsed.

sm6srw

Nice, one question then LGTM!

sm6srw · 2021-09-13T12:21:42Z

src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs

@@ -565,12 +565,23 @@ private void NodeOnRequestSilenceNodeModifiedEvents(NodeModel node, bool value)
        public override void Clear()
        {
            WorkspaceClosed?.Invoke();
+            UndefineCBNFunctionDefinitions();


So we have never undefined function definitions before loading a new workspace until now? Where they still around after a new home workspace was loaded?

Yes, that is correct. There are two cores in the VM - one static and the other the runtime core. The runtime core was being reset every time a new workspace was opened but not the static core. I realized that if we reset the static core, we would also need to reload all libraries and packages again into the core, and we don't want to do that for every new workspace, so I decided to simply undefine the function definitions from the static core instead.

is this called both when opening a new workspace and creating a new workspace?

When opening an existing workspace, this function is called in another place - when reading the JSON. There is another call to the same method in this PR.

pinzart90 · 2021-09-13T17:09:20Z

So regarding
Additionally, any function definitions in CBNs from a previous workspace are invalidated (procedureNode is marked inactive) upon opening a new workspace so that they no longer continue to show up in autocomplete, and function not found warnings appear if an attempt is made to call them
I wonder if this was useful in certain cases...Maybe having multiple workspaces opened at the same time...so you can share function definitions ... (not sure about having access to func defs from a closed workspace though..)
@mjkkirschner @aparajit-pratap ?

aparajit-pratap · 2021-09-13T17:10:38Z

So regarding
Additionally, any function definitions in CBNs from a previous workspace are invalidated (procedureNode is marked inactive) upon opening a new workspace so that they no longer continue to show up in autocomplete, and function not found warnings appear if an attempt is made to call them
I wonder if this was useful in certain cases...Maybe having multiple workspaces opened at the same time...so you can share function definitions ...
@mjkkirschner @aparajit-pratap ?

We don't have multiple workspace support right now or a proposed design for how it should work w.r.t. function definitions.
With the way things stand currently, it is confusing to still be able to access a function that was defined in a previously closed workspace.

mjkkirschner · 2021-09-13T17:31:19Z

So regarding
Additionally, any function definitions in CBNs from a previous workspace are invalidated (procedureNode is marked inactive) upon opening a new workspace so that they no longer continue to show up in autocomplete, and function not found warnings appear if an attempt is made to call them
I wonder if this was useful in certain cases...Maybe having multiple workspaces opened at the same time...so you can share function definitions ...
@mjkkirschner @aparajit-pratap ?

We don't have multiple workspace support right now or a proposed design for how it should work w.r.t. function definitions.
With the way things stand currently, it is confusing to still be able to access a function that was defined in a previously closed workspace.

I agree - because it creates a situation where users can build broken graphs.

src/Engine/ProtoCore/Utils/ParserUtils.cs

src/DynamoCore/Engine/EngineController.cs

src/DynamoCore/Graph/Nodes/CodeBlockNode.cs

mjkkirschner · 2021-09-13T17:59:46Z

src/DynamoCore/Graph/Nodes/CodeBlockNode.cs

+            }
+            catch (Exception)
+            {
+                buildStatus = null;


should we log something?

I'm not sure if we can log any useful information here that would let the user debug the node. For example, how would we identify this code block node for the user? We are catching a similar exception here and not logging anything there either.

Any compilation warnings should appear on the code block node in any case.

mjkkirschner · 2021-09-13T18:00:33Z

src/DynamoCore/Graph/Nodes/CodeBlockNode.cs

+                // 
+                // To check function redefinition, we need to check other
+                // CBN to find out if it has been defined yet. Now just
+                // skip this warning.


should this be a todo/followup jira?

https://jira.autodesk.com/browse/DYN-4104

src/DynamoCore/Graph/Workspaces/NodesToCodeExtensions.cs

mjkkirschner · 2021-09-13T18:07:57Z

src/Engine/ProtoCore/Core.cs

-                                        .ToList();
-
-                foreach (var symbol in localSymbols)
+                procNodes = block.procedureTable.Procedures.Where(p => p.HashID == hash).ToList();


so what situation causes a procedure in the procedureTable to somehow have the same hashID, isn't it a hash table? How can there be more than one matching entry?

Procedures is a list, not a hashtable.

As long as the function name and the argument types and strings are the same, the same hash ID is generated. So if a function is deleted, then re-created, say if you delete a CBN and undo the deletion, a new entry for the same function with the same hash ID will be added, after the previous one has been marked inactive. The new one will be active.

It's surprising that it's not a table at all... any idea why it's implemented with a list?

Does it make sense to use some query method on the procedureTable instead of accessing the list directly?

Did you see this case come up or it's just defensive coding?

oh interesting.. so the procedure table will fill up with inactive functions - should it be cleaned up at some point? If we don't already do that?

@pinzart90 had the same question. I don't have an answer to why a list was chosen and not a dictionary/hash table. I just think it was a poor choice of data structure. We do this even for the instruction stream where that is another list that keeps growing. Deleting instead of maintaining an ever-expanding list (and marking stale items inactive) is definitely the way to go but it's a bigger change and probably outside the scope of this PR. There needs to be a separate spike/epic to optimize these data structures.

The reason its name contains the word "table" is probably because it is aimed at representing that concept of a function table in traditional compiler design.

mjkkirschner · 2021-09-13T18:08:45Z

src/Engine/ProtoCore/Core.cs

                    }
                }
            }

+            if (DSExecutable == null) return;


when does this happen?

When the method is invoked on the static core. There is no DSExecutable for this core.

mjkkirschner · 2021-09-13T18:19:27Z

@aparajit-pratap it looks good, I've got a few more questions in line -
but on the subject of performance, it seems we need to compile most of the AST twice on open - depending on the graph I guess this could be a little costly?

I guess we need to recompile only things that refer to those functions? ie, function calls, and function objects? Maybe this is an optimization we can skip for now, just wanted to start the discussion depending on if you have any performance concerns.

mjkkirschner

lgtm

aparajit-pratap · 2021-09-13T19:28:55Z

I guess we need to recompile only things that refer to those functions? ie, function calls, and function objects? Maybe this is an optimization we can skip for now, just wanted to start the discussion depending on if you have any performance concerns.

Yes, code now needs to be recompiled twice, which is a downside of this PR, however, for the most part compilation is relatively inexpensive as compared to actual execution (except for detecting cycles). I can distil the AST's down to only function call ASTs and recompile only those in the second pass as you suggested but I'm not sure whether that will make much of a difference as I would first need to walk each AST to check for which contain function calls on the rhs. Also, most of these function calls will most likely be FFI calls just because ZT methods are more common than DS function definitions. Definitely, something we can explore further.

mjkkirschner · 2021-09-13T20:17:24Z

@aparajit-pratap - I'm happy with the state for now if you are, we can try to optimize it further if we need, we could watch the perf CI test to see if opening graphs is affected by a significant margin.

aparajit-pratap added 3 commits August 31, 2021 20:00

reset library management core on opening new workspace

14bab8e

do not log function not found warnings during precompilation of CBNs

2ff1191

revert unchanged file

ed0b247

aparajit-pratap added the WIP label Sep 1, 2021

precompile all function definition ASTs first

96c41e9

aparajit-pratap changed the title ~~Dyn 3477 - Do not log function not found warnings during precompilation phase of CBNs.~~ Dyn 3477 - First precompile Function Definition ASTs in all CBNs Sep 2, 2021

aparajit-pratap added 5 commits September 2, 2021 22:25

tie loose ends

35e1877

refactor CBN constructors

1912f31

more refactoring

4130072

corrections - cbns need to be compiled as soon as they're being deser…

9d10b46

…ialized

cleanup

177296e

aparajit-pratap changed the title ~~Dyn 3477 - First precompile Function Definition ASTs in all CBNs~~ Dyn 3477 - Avoid DS function not found errors in CBNs when defined Sep 3, 2021

aparajit-pratap added 7 commits September 3, 2021 13:30

more cleanup

891b992

rename method

c735a5a

resolve merge conflicts

fd1e5f4

add missing code comments

ef99ba9

final fixes

235ea13

set CBN first pass compilation flags at the right place

9005eb8

undefine function definitions on closing/opening new workspace

0f0af6b

aparajit-pratap changed the title ~~Dyn 3477 - Avoid DS function not found errors in CBNs when defined~~ Dyn 3477 - Avoid unexpected DS function-not-found errors in CBNs, invalidate function definitions on opening new workspace Sep 8, 2021

aparajit-pratap added 2 commits September 8, 2021 12:15

mark all function defs inactive

86bf12d

add tests, fix workspace changed regression

792bd63

aparajit-pratap added PTAL Please Take A Look 👀 and removed WIP labels Sep 8, 2021

more tests

2ccc198

aparajit-pratap requested review from pinzart90 and sm6srw September 9, 2021 04:17

pinzart90 reviewed Sep 10, 2021

View reviewed changes

sm6srw approved these changes Sep 13, 2021

View reviewed changes

aparajit-pratap requested a review from mjkkirschner September 13, 2021 17:11