Register custom node before package load reset #10591

mmisol · 2020-04-23T14:15:29Z

Purpose

This fixes a problem where existing custom nodes in the home workspace
became unresolved after a package that contained binaries was loaded.

The cause of the problem was that the compiled function was not
available at the time of the VM reset. Now the data is registered on
package load, by queueing it in pendingCustomNodeSyncData. This results
in a CompileCustomNodeAsyncTask being scheduled before the update of
the home workspace graph takes place.

This also removes the DisableRun flag, ~~hopefully~~ without breaking any
tests.

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.

Reviewers

@reddyashish @aparajit-pratap @mjkkirschner

FYIs

@QilongTang

Enlarged the mutually exclusive context in ResetVMAndResyncGraph to also include ReInitializeLiveRunner. This is important as the reinitialization of the runner resets the runtime core, making the Design Script executable null, which will cause any running executions to crash Dynamo, even respecting the mutually exclusive context.

This fixes a problem where existing custom nodes in the home workspace became unresolved after a package that contained binaries was loaded. The cause of the problem was that the compiled function was not available at the time of the VM reset. Now the data is registered on package load, by queueing it in pendingCustomNodeSyncData. This results in a CompileCustomNodeAsyncTask being scheduled before the update of the home workspace graph takes place.

mjkkirschner · 2020-04-23T17:04:36Z

test/Libraries/PackageManagerTests/PackageLoaderTests.cs

+            var functionNodes = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Function>();
+            Assert.AreEqual(1, functionNodes.Count());
+            var functionNode = functionNodes.First();
+            Assert.AreEqual(ElementState.Active, functionNode.State);


can we also assert this custom node returns the correct value?

To be honest I'm not exactly sure what the example is doing, as I took it straight from @aparajit-pratap repro. I can probably check that it returns some value at least.

This example load the dyf first then followed by the dyn home workspace, at this point, we should have a working custom node and a dummy node. Then we load the package and make sure custom node still function correctly. I would encourage before and after the package load, we assert custom node in Active State and give correct value.

mjkkirschner · 2020-04-23T17:05:51Z

src/DynamoCore/Models/DynamoModel.cs


            foreach (var def in CustomNodeManager.LoadedDefinitions)
                RegisterCustomNodeDefinitionWithEngine(def);
        }

+        private void EngineController_RequestCustomNodeRegistration(object sender, EventArgs e)
+        {
+            foreach (var def in CustomNodeManager.LoadedDefinitions)


I'm curious about the timing of this - as I think @reddyashish mentioned - what if the package to be loaded contains custom nodes, would those exist in the LoadedDefinitions list before this call is made?

That seems worth investigating. Taking a look at the interaction during the PackgeLoaded event, fired on the first stage of LoadPackages, there are some steps related to custom nodes that I did not fully capture. I'll take a closer look and see if I can find where this happens and add it to the internal documentation.

mjkkirschner · 2020-04-23T17:07:02Z

src/DynamoCore/Engine/EngineController.cs

@@ -518,8 +513,18 @@ private void OnLibraryLoaded()
        /// </summary>
        private void LibraryLoaded(object sender, LibraryServices.LibraryLoadedEventArgs e)
        {
-            if(e.LibraryPaths.Any())
+            if (e.LibraryPaths.Any())


so libraryPaths will include paths to custom nodes or only binaries?

This is used for binaries only

QilongTang · 2020-04-23T19:22:17Z

src/DynamoCore/Models/DynamoModel.cs

@@ -1039,6 +1039,7 @@ private void OnAsyncTaskStateChanged(DynamoScheduler sender, TaskStateChangedEve
        public void Dispose()
        {
            EngineController.TraceReconcliationComplete -= EngineController_TraceReconcliationComplete;
+            EngineController.RequestCustomNodeRegistration -= EngineController_RequestCustomNodeRegistration;


Thank you for make sure we unsubscribe. Things like this take much more time to identify later and do memory analysis

aparajit-pratap · 2020-04-23T19:48:32Z

src/DynamoCore/Models/DynamoModel.cs

@@ -1467,11 +1469,18 @@ protected void ResetEngineInternal()

            EngineController.MessageLogged += LogMessage;
            EngineController.TraceReconcliationComplete += EngineController_TraceReconcliationComplete;
+            EngineController.RequestCustomNodeRegistration += EngineController_RequestCustomNodeRegistration;


~~I wonder if we should just move all these subscriptions to the constructor for EngineController?~~

Can we move the EngineController.OnLibraryLoaded call out of the EngineController constructor to after this line and

Move the call to OnRequestCustomNodeRegistration within EngineController.OnLibraryLoaded here:

Dynamo/src/DynamoCore/Engine/EngineController.cs

Line 514 in c00c80b

}

This way we can remove the foreach here:

Dynamo/src/DynamoCore/Models/DynamoModel.cs

Line 1471 in c00c80b

foreach (var def in CustomNodeManager.LoadedDefinitions)

as the same code will already be called within EngineController.OnLibraryLoaded in OnRequestCustomNodeRegistration, thereby leading to more code reuse.

You could make EngineController.OnLibraryLoaded internal.

src/DynamoCore/Engine/EngineController.cs

aparajit-pratap · 2020-04-23T20:03:48Z

src/DynamoCore/Engine/EngineController.cs

+            if (e.LibraryPaths.Any())
+            {
+                // Need to make compiled custom nodes available before resetting the VM
+                OnRequestCustomNodeRegistration();
                OnLibraryLoaded();


So now it looks like the custom node definitions are scheduled to be added to the VM before it is reset in OnLibraryLoaded and therefore could be added before it is reset, which could again wipe out all of the definitions potentially. Wouldn't it be safer to call OnRequestCustomNodeRegistration after OnLibraryLoaded?

This is where the method names might be misleading. I named this OnRequestCustomNodeRegistration mainly because it calls RegisterCustomNodeDefinitionWithEngine downstream. However, that function does not register anything with the RuntimeCore, that's the misleading part. What this does is fill a queue that gets processed afterwards, when calling OnLibraryLoaded.

I'm not sure I understand. The pendingCustomNodeSyncData queue is processed in EngineController.ProcessPendingCustomNodeSyncData, which is called in HomeWorkspaceModel.Run. Run is not called from OnLibraryLoaded. My question is, what is the harm in calling OnRequestCustomNodeRegistration after the VM is reset in OnLibraryLoaded?

Run is not called from OnLibraryLoaded

You are right, I had overlooked this. Looking at the sequence of calls, the most appropriate place for this code should be in HomeWorkspaceModel.LibraryLoaded, which is the event handler triggering the run. That way we avoid doing extra work if no home workspace is open. Do you agree?

Please see my comments below.

aparajit-pratap

@mmisol suggestion to make one refactor (please see comments).
Also, I still have the question about how does this fix to reload custom node definitions get rid of the need for the disableRun flag.

aparajit-pratap · 2020-04-26T03:55:17Z

src/DynamoCore/Models/DynamoModel.cs

@@ -1467,11 +1469,18 @@ protected void ResetEngineInternal()

            EngineController.MessageLogged += LogMessage;
            EngineController.TraceReconcliationComplete += EngineController_TraceReconcliationComplete;
+            EngineController.RequestCustomNodeRegistration += EngineController_RequestCustomNodeRegistration;


Can we move the EngineController.OnLibraryLoaded call out of the EngineController constructor to after this line and

Move the call to OnRequestCustomNodeRegistration within EngineController.OnLibraryLoaded here:

Dynamo/src/DynamoCore/Engine/EngineController.cs

Line 514 in c00c80b

}

This way we can remove the foreach here:

Dynamo/src/DynamoCore/Models/DynamoModel.cs

Line 1471 in c00c80b

foreach (var def in CustomNodeManager.LoadedDefinitions)

as the same code will already be called within EngineController.OnLibraryLoaded in OnRequestCustomNodeRegistration, thereby leading to more code reuse.

You could make EngineController.OnLibraryLoaded internal.

mmisol · 2020-04-27T13:52:39Z

Hi @aparajit-pratap . In my comment #10591 (comment) I try to explain why the code was moved. Let me know if the point did not make sense and let's discuss, as the suggested refactor does not take it into account.

aparajit-pratap · 2020-04-27T14:21:38Z

Hi @aparajit-pratap . In my comment #10591 (comment) I try to explain why the code was moved. Let me know if the point did not make sense and let's discuss, as the suggested refactor does not take it into account.

Yes, I did see your comment but I still propose the refactor in preference to your #10591 (comment).

mmisol · 2020-04-28T12:47:01Z

Hi @aparajit-pratap .

After our discussion I went ahead and reviewed the calls being done on package load regarding custom nodes. It turns out the handler that performs the queuing of the custom node is not registered for a custom node that is not used. That would mean going with the refactor you suggested could be a good idea, as it simplifies the code.

However, after doing that, the custom node test introduced here started to fail. The custom node appears with the warning both before and after installing the package.

Since this is a suggested refactor, but it doesn't quite work, can we leave it as a separate task?

mmisol · 2020-04-28T13:17:30Z

Regarding the refactor and the results I see, I guess we'd also need to pass the EngineController_RequestCustomNodeRegistration event handler to the constructor of EngineController and also assign it before calling OnLibraryLoaded. I tried that too and it fixes the issue with the custom node. However, the node from the QRImage package disappears from the design after loading the package, so there seems to be something else going on.

Again, I think a refactor would be nice too, but we should probably take a look at this in a more global context rather than this PR.

aparajit-pratap · 2020-04-29T13:30:53Z

@mmisol thanks for trying out the suggested changes. AFAIK it shouldn't have caused any failures as it is essentially doing the same thing as your PR. The sequence of calls is exactly the same except that the code is in different methods (I can check it out once again when I find the time). Anyway, I don't wish to hold you up. I'm okay with these changes as-is. Thanks a lot for working on this.

mmisol · 2020-04-29T13:39:20Z

No problem @aparajit-pratap ! You are more than welcome.

Can I ask for your approval if you feel confident about this PR? Thanks!

* add namespace conflict tests for short name replacer and node to code (#10611) * Register custom node before package load reset (#10591) This fixes a problem where existing custom nodes in the home workspace became unresolved after a package that contained binaries was loaded. The cause of the problem was that the compiled function was not available at the time of the execution after a VM reset. Now the data is registered on package load, by queueing it in pendingCustomNodeSyncData. This results in a CompileCustomNodeAsyncTask being scheduled before the update of the home workspace graph takes place. * Increase coverage of the Core folder (#10609) * Add a test for the CrashPromptArgs class * Update DynamoCoreTests.csproj * Added NotificationObject tests * Updated the test, NotificationObject coverage at 100% * Changed some access modifiers * Added coverage for Updates/BinaryVersion * Added comments and fixed names * Revert "Added comments and fixed names" This reverts commit 42cd024. * Revert "Added coverage for Updates/BinaryVersion" This reverts commit 679deca. * Increased coverage in CrashPromptArgs * Added some Core coverage * DYN-2560 - Increase the code coverage: DynamoCore/Models Folder First Part (#10612) * DYN-2560 - Increase the code coverage: DynamoCore/Models Folder I started adding just one test method TestOnRequestDispatcherBeginInvoke() for testing the DynamoModelEvents class. * DYN-2560 - Adding test cases for DynamoModelEvents Adding test cases for DynamoModelEvents * DYN-2560 - Adding test cases for DynamoModelEvents I added all the test cases for the all events in the DynamoModelEvent class, i just need to fix the last 6 of them. * DYN-2560 - Adding test cases for DynamoModelEventsArgs I added several test cases for the classes inside the DynamoModelEventsArgs.cs file. ZoomEventArgs TaskDialogEventArgs EvaluationCompletedEventArgs DynamoModelUpdateArgs FunctionNamePromptEventArgs PresetsNamePromptEventArgs ViewOperationEventArgs PointEventArgs WorkspaceEventArgs ModelEventArgs * DYN-2560 - Code Review Comments Based in the comment done by Aaron in the GitHub pull request, I added more description comments for the method TestTaskDialogEventArgs() and also I added comments for a local function * DYN-2560 - Code Review Comments 2 There was a spelling error in two methods for the word "Internally", then I fixed this error in the two places. * Python Engine Enum (#10618) * Cherrypick * Comments * Add unit test * Comments * Handle runtime table gaps on code block deletion (#10605) When the runtime table are built there is an implicit assumption that the code block ids are consecutive. However, that is not always the case, as the deletion of a procedure causes the deletion of its child code blocks, which may generate gaps in the id numbering. In order to make the code resiliant to these gaps, the runtime tables are sized based on the largest code block id, rather than in the amount of code blocks. * Validate ASM installations before loading (#10621) The check for the specific assemblies tbb.dll and tbbmalloc.dll is generalized to a full file list validation of detected ASM locations. This way, Dynamo is guarded against any incomplete/unusual ASM binary folders that other applications might include. The lists of files for each version were taken from LibG. They cannot be reused from LibG without involving major changes in the preloader, so the lists should be kept in sync as new major release of ASM occur. * SQ bug fix (#10622) * (1) Null reference bug fix from SQ dashboard * Add support for debug modes (#10603) * Add support for debug modes * Increase coverage of the Updates folder (#10628) * Add a test for the CrashPromptArgs class * Update DynamoCoreTests.csproj * Added NotificationObject tests * Updated the test, NotificationObject coverage at 100% * Changed some access modifiers * Added coverage for Updates/BinaryVersion * Added comments and fixed names * Added asserts for happy path, changed namespace and deleted a console function * Removed using * Revert "Removed using" This reverts commit 022823d. * Removed a change that should not be there * Added coverage for Updates folder * Tests for CPython3 Engine as well as for IronPython. * [WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. (#10624) * reduce number of serialization tests by factor 3~ * reduce wpf json serialization tests * Handle missing instance calling method statically (#10630) A code block node calling an instance method in its static form would make Dynamo crash if the instance was not provided. An example of this would be calling 'Curve.Patch();'. The cause of the issue was that the default argument was ultimately tried to be interpreted as a pointer. By avoiding that wrong conversion the engine is now able to surface the real problem as a human-readable warning. * Python3 Selection Under Debug Modes (#10629) * Cherrypick Python3 changes * Cleanup * Use Debug Modes * CleanUp * Rename Function * Clean Up * Do not use anouymous function as handler * Revert newer language change * Add adp analytics to Dynamo (#10576) * add ADPTracker register * Fix non-array item search for dot operation (#10633) When an array is passed to the dot operation, it needs to get an item to determine the actual class being processed. This was done in ArrayUtils.GetFirstNonArrayStackValue, but the function only checked the first item of the array and its descendants. This caused the dot operation to failed when passed an array that contained an empty array as its first item. In order to fix the problem, the function was changed to check for non-array items in the entire array. The function should stop as soon as the first non-array item is found. * Addressing some comments * some more comments * Update AssemblyInfo.cs * Update AssemblyInfo.cs * Marking the python node as modified when its engine property is modified Also updated some tests. * Remove unwanted check. * changes to test * Update AssemblySharedInfo.cs * Using python engine's AcquireLock to avoid deadlock. The deadlock was happening only when multiple PythonEngine's are initialized on a thread from 2 different test fixtures in the same run. The main difference is calling this function PythonEngine.BeginAllowThreads(). Also we do not want to initialze the python engine if it is already initialized. Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: Bruno Yorda <[email protected]> Co-authored-by: Roberto T <[email protected]> Co-authored-by: Aaron (Qilong) <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: Tibi <[email protected]> Co-authored-by: Michael Kirschner <[email protected]>

mmisol added 6 commits April 17, 2020 12:26

Mark UpdateCmdLineInterpreter as obsolete

8a9d04e

Remove DisableRun flag

1998100

Merge remote-tracking branch 'origin' into remove_disable_run

3533097

Add test

49a8a74

mmisol requested review from mjkkirschner, aparajit-pratap and reddyashish April 23, 2020 14:15

mjkkirschner reviewed Apr 23, 2020

View reviewed changes

QilongTang reviewed Apr 23, 2020

View reviewed changes

aparajit-pratap reviewed Apr 23, 2020

View reviewed changes

src/DynamoCore/Engine/EngineController.cs Show resolved Hide resolved

Change test to not use geometry

19d956c

aparajit-pratap reviewed Apr 23, 2020

View reviewed changes

mmisol added 2 commits April 23, 2020 16:25

Assert node status before package load

1ad4ce4

Queue CNs on HW library loaded

8ea3c63

aparajit-pratap requested changes Apr 26, 2020

View reviewed changes

aparajit-pratap mentioned this pull request Apr 26, 2020

[DRAFT] [DNM] Suggestions for #10591 mmisol/Dynamo#1

Open

aparajit-pratap approved these changes Apr 29, 2020

View reviewed changes

mmisol merged commit 31cb44e into DynamoDS:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register custom node before package load reset #10591

Register custom node before package load reset #10591

mmisol commented Apr 23, 2020 •

edited

Loading

mjkkirschner Apr 23, 2020

mmisol Apr 23, 2020

QilongTang Apr 23, 2020

mjkkirschner Apr 23, 2020

mmisol Apr 23, 2020 •

edited

Loading

mjkkirschner Apr 23, 2020

mmisol Apr 23, 2020

QilongTang Apr 23, 2020

aparajit-pratap Apr 23, 2020 •

edited

Loading

aparajit-pratap Apr 26, 2020 •

edited

Loading

aparajit-pratap Apr 23, 2020 •

edited

Loading

mmisol Apr 23, 2020 •

edited

Loading

aparajit-pratap Apr 24, 2020

mmisol Apr 24, 2020

aparajit-pratap Apr 26, 2020

aparajit-pratap left a comment

aparajit-pratap Apr 26, 2020 •

edited

Loading

mmisol commented Apr 27, 2020

aparajit-pratap commented Apr 27, 2020 •

edited

Loading

mmisol commented Apr 28, 2020

mmisol commented Apr 28, 2020

aparajit-pratap commented Apr 29, 2020 •

edited

Loading

mmisol commented Apr 29, 2020

Register custom node before package load reset #10591

Register custom node before package load reset #10591

Conversation

mmisol commented Apr 23, 2020 • edited Loading

Purpose

Declarations

Reviewers

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

mmisol Apr 23, 2020 • edited Loading

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 Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

mmisol Apr 23, 2020 • edited Loading

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 left a comment

Choose a reason for hiding this comment

aparajit-pratap Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

mmisol commented Apr 27, 2020

aparajit-pratap commented Apr 27, 2020 • edited Loading

mmisol commented Apr 28, 2020

mmisol commented Apr 28, 2020

aparajit-pratap commented Apr 29, 2020 • edited Loading

mmisol commented Apr 29, 2020

mmisol commented Apr 23, 2020 •

edited

Loading

mmisol Apr 23, 2020 •

edited

Loading

aparajit-pratap Apr 23, 2020 •

edited

Loading

aparajit-pratap Apr 26, 2020 •

edited

Loading

aparajit-pratap Apr 23, 2020 •

edited

Loading

mmisol Apr 23, 2020 •

edited

Loading

aparajit-pratap Apr 26, 2020 •

edited

Loading

aparajit-pratap commented Apr 27, 2020 •

edited

Loading

aparajit-pratap commented Apr 29, 2020 •

edited

Loading