restore behavior of extension loadNodeLibrary method #10187

mjkkirschner · 2019-12-03T00:07:42Z

Purpose

https://jira.autodesk.com/browse/DYN-2325
previously the method LoadNodeLibrary on dynamoModel - accessed via the extensionLibraryLoader had the behavior that it would successfully load zerotouch OR NodeModel assemblies and add them to the library.

after this pr:
#9736

this no longer works for zerotouch assemblies as the call is redirected to LibraryServices.LoadNodeLibrary instead of ImportLibrary - which no longer raises a LibraryLoaded event .

to remedy this I simply added the event raise back to the branch of code where a zero touch assembly is encountered. The libraryLoaded event, among other things, adds the zero touch nodes to search which updates the library view.

@aparajit-pratap does this have a negative impact on the work you did here? If thats the case we can isolate this by only adding the event raise to the ExtensionLibraryLoader class and keep it out of DynamoModel - but it does seem like a significant change in behavior and cause of confusion (zero touch functions are in the VM, but none of them are loaded into the library)

I would like to get this or the alternative (or something better) into 2.5 to avoid breaking integrations which rely on this functionality.

we will also need to add a test that ensures the searchModel is updated when this function is called. ✔️

Update:

This PR now implements equals and getHashCode() for the NodeSearchElement base class - so that duplicate searchElements cannot be added to search if they contain matching properties. This was not a problem for builtin nodes, but for nodeModels and zeroTouch where we construct new nodeSearchElements whenever the importLibrary or loadNodeLibrary methods are called, this could lead to extra search entries being created if those apis were called from extensions.

The reason that we have this duplication problem when reverting back to ImportLibrary is that the method AddZeroTouchNodeToSearch is called when ImportLibrary is done importing, as it raises the event OnLibrariesImported for each library - then later, after all packages are loaded - the packageLoader raises the same handler via the PackageLoaded event. - calling AddZeroTouchNodeToSearch again.

~~so this solution works, but it leaves something to be desired, why do this extra work of scanning the zt functions again. I think other solutions might be~~

1. to remove the additional call in the packageLoader which re-raises the LibraryLoaded events - I believe @aparajit-pratap added this to finish importing all libraries into search only after all packages were loaded for a small speedup benefit, but it seems this may be the root of some of our issues. What do you think about removing this?

We can add a new method like ImportLibrary - which does not raise the LoadedEvent and does not add anything to search - package manager extension can use this method instead of the public one. I find this odd because it just seems strange to me to have a method which imports a library but does not finish adding it to search, but we can make this method very explicitly named and documented - perhaps its a parameter added to an overload of LoadNodeLibrary to control adding to search. ie LoadNodeLibrary(Assembly assem, addToSearch = true)

3. whats in this PR currently - overriding equals and getHashcode - so that when the zt packages are added again to the search we end up just resetting the search element tags - but don't add a new element - it's some wasted cycles per zt function imported per package.

~~In any case we can leave these overrides in NodeSearchModel if the team thinks this is the correct behavior - thats my opinion.~~

update2:

went with solution 2. Removed equality overrides as we would need to add them for each class and carefully test the assumptions each class makes. For now this PR restores the old behavior of the API for extensions authors and keeps the new behavior for package manager using an internal method.

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

FYIs

@jhauswirth

…ore behavior

mjkkirschner · 2019-12-03T01:54:13Z

self serve is running now.

src/DynamoCore/Models/DynamoModel.cs

review comments

do not add duplicate search entires

mjkkirschner · 2019-12-03T20:25:13Z

@QilongTang @reddyashish @aparajit-pratap - working with @aparajit-pratap we found that the nodeSearchModel did not override equals and gethashcode even though they are used as keys for the searchModel dictionary -

after implementing this keys are no longer duplicated when importing the library, even if the library is already imported. That seems like a good thing.

I would like to make the equality and hashcode checks more unique though, taking into account parameters, category etc just incase a node only differs by parameters or there is an overload - for zero touch that would usually be taken care of by the creationName containing the parameter names, but since this is the base class we need to consider all node search types.

mjkkirschner · 2019-12-03T23:05:50Z

There are some failing tests with this change -

Errors and Failures:
1) Test Failure : Dynamo.Tests.SearchModelTests.CanRefactorCustomNodeWhilePreservingDuplicates
     Expected: 2
  But was:  1

at Dynamo.Tests.SearchModelTests.CanRefactorCustomNodeWhilePreservingDuplicates()

2) Test Failure : ViewExtensionLibraryTests.LibraryResourceProviderTests.LibraryDataUpdatedEventRaised
     Expected: 6
  But was:  3

at ViewExtensionLibraryTests.LibraryResourceProviderTests.LibraryDataUpdatedEventRaised() in E:\Builds\mjkkirschner_Dynamo\loadNodeLibraryRegression\Dynamo\test\ViewExtensionLibraryTests\LibraryResourceProviderTests.cs:line 414

looking into them.

… guid as seperate entries add API test improve equality and hashcode

aparajit-pratap · 2019-12-04T02:32:47Z

to remove the additional call in the packageLoader which re-raises the LibraryLoaded events - I believe @aparajit-pratap added this to finish importing all libraries into search only after all packages were loaded for a small speedup benefit, but it seems this may be the root of some of our issues. What do you think about removing this?

@mjkkirschner I'm not sure what you mean by "removing this". We have to finish importing into VM and adding to search after all packages are loaded, don't we? Or are you suggesting we revert to the old inefficient way of doing things again and again for each package?

aparajit-pratap · 2019-12-04T02:34:49Z

I would like to make the equality and hashcode checks more unique though, taking into account parameters, category etc just incase a node only differs by parameters or there is an overload - for zero touch that would usually be taken care of by the creationName containing the parameter names, but since this is the base class we need to consider all node search types.

Can you give an example? Not sure, if I understand.

mjkkirschner · 2019-12-04T02:40:10Z

@mjkkirschner I'm not sure what you mean by "removing this". We have to finish importing into VM and adding to search after all packages are loaded, don't we? Or are you suggesting we revert to the old inefficient way of doing things again and again for each package?

yes - my understanding is that by reverting to importLibrary in this PR we've already reverted back to the inefficient way of doing this - now we're wasting even more time by doing both - once for each package, and then again for all of them at the end, which is why my PR broke your test.

mjkkirschner · 2019-12-04T02:41:55Z

Can you give an example? Not sure, if I understand.

I've already done this in the current PR - I was just worried about types which might somehow have the same name, but different parameters, I think this is unlikely but parameters is now added to the equality and hashcode check.

aparajit-pratap · 2019-12-04T02:46:42Z

I've already done this in the current PR - I was just worried about types which might somehow have the same name, but different parameters, I think this is unlikely but parameters is now added to the equality and hashcode check.

One example of this is:

So it's good to check for parameters as well.

aparajit-pratap · 2019-12-04T02:47:43Z

yes - my understanding is that by reverting to importLibrary in this PR we've already reverted back to the inefficient way of doing this - now we're wasting even more time by doing both - once for each package, and then again for all of them at the end, which is why my PR broke your test.

Hmm, let's discuss it again tomorrow.

use this to revert to old behavior for API users while keeping package manager behavior unchanged update tests to use new method

aparajit-pratap · 2019-12-04T21:50:59Z

src/DynamoCore/Models/DynamoModel.cs

+        /// <param name="suppressZeroTouchLibraryLoad">If True, zero touch types will not be added to search.
+        /// This is used by packageManager extension to defer adding ZT libraries to search until all libraries are loaded.
+        /// </param>
+        internal void LoadNodeLibrary(Assembly assem, bool suppressZeroTouchLibraryLoad)


Minor, but can we make the second parameter to this method a default argument set to true just coz it's called in a number of places internally with true?

aparajit-pratap · 2019-12-04T21:53:33Z

src/DynamoCore/Models/DynamoModel.cs

+        /// </summary>
+        /// <param name="assem">The assembly to load which contains the types to import.</param>
+        /// <param name="suppressZeroTouchLibraryLoad">If True, zero touch types will not be added to search.
+        /// This is used by packageManager extension to defer adding ZT libraries to search until all libraries are loaded.


Thanks for the code comments 👍

QilongTang

LGTM with a few comments

QilongTang · 2019-12-05T02:16:13Z

src/DynamoCore/Extensions/ExtensionLibraryLoader.cs

@@ -24,14 +24,28 @@ internal ExtensionLibraryLoader(DynamoModel model)
        }

        /// <summary>
-        /// Loads the node library.
+        /// Loads a zZeroTouch or NodeModel based node into the VM and search.


typo ZeroTouch

QilongTang · 2019-12-05T04:36:31Z

test/DynamoCoreTests/ExtensionTests.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Reflection;


Can you remind me where is this used in this file?

reddyashish · 2019-12-05T15:38:05Z

Looks good to me.

reddyashish

mjkkirschner · 2019-12-05T18:19:03Z

test/DynamoCoreTests/ExtensionTests.cs

+        public void ExtensionLoader_LoadNodeLibraryAddsZTNodesToSearch()
+        {
+            var assemPath = Path.Combine(testpkgPath, "Dynamo Samples", "bin", "SampleLibraryZeroTouch.dll");
+            var assembly = Assembly.LoadFrom(assemPath);


@QilongTang reflection is used to load this assembly.

* raise event when importing zt node via LoadNodeLibrary method to restore behavior * Update DynamoModel.cs review comments * test passes with this change- do not add duplicate search entires * update behavior of custom node test - no longer entries with the same guid as seperate entries add API test improve equality and hashcode * add new method to extensionLibraryLoader use this to revert to old behavior for API users while keeping package manager behavior unchanged update tests to use new method * review comments (cherry picked from commit 0605400)

raise event when importing zt node via LoadNodeLibrary method to rest…

27fb9fd

…ore behavior

mjkkirschner added this to the 2.5.0 milestone Dec 3, 2019

mjkkirschner requested review from aparajit-pratap and reddyashish December 3, 2019 00:15

mjkkirschner added PTAL Please Take A Look 👀 developer experience more high-level issues that are more specific to individuals actively developing on Dynamo. API labels Dec 3, 2019

aparajit-pratap reviewed Dec 3, 2019

View reviewed changes

src/DynamoCore/Models/DynamoModel.cs Outdated Show resolved Hide resolved

Update DynamoModel.cs

ba16570

review comments

mjkkirschner added the DNM Do not merge. For PRs. label Dec 3, 2019

test passes with this change-

418f7c1

do not add duplicate search entires

update behavior of custom node test - no longer entries with the same…

2225b71

… guid as seperate entries add API test improve equality and hashcode

add new method to extensionLibraryLoader

10919c7

use this to revert to old behavior for API users while keeping package manager behavior unchanged update tests to use new method

mjkkirschner removed the DNM Do not merge. For PRs. label Dec 4, 2019

aparajit-pratap approved these changes Dec 4, 2019

View reviewed changes

QilongTang approved these changes Dec 5, 2019

View reviewed changes

reddyashish approved these changes Dec 5, 2019

View reviewed changes

mjkkirschner commented Dec 5, 2019

View reviewed changes

review comments

651c687

mjkkirschner merged commit 0605400 into DynamoDS:master Dec 5, 2019

mjkkirschner mentioned this pull request Dec 5, 2019

restore behavior of extension loadNodeLibrary method (#10187) #10196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore behavior of extension loadNodeLibrary method #10187

restore behavior of extension loadNodeLibrary method #10187

mjkkirschner commented Dec 3, 2019 •

edited

Loading

mjkkirschner commented Dec 3, 2019

mjkkirschner commented Dec 3, 2019

mjkkirschner commented Dec 3, 2019

aparajit-pratap commented Dec 4, 2019 •

edited

Loading

aparajit-pratap commented Dec 4, 2019

mjkkirschner commented Dec 4, 2019

mjkkirschner commented Dec 4, 2019

aparajit-pratap commented Dec 4, 2019

aparajit-pratap commented Dec 4, 2019

aparajit-pratap Dec 4, 2019

aparajit-pratap Dec 4, 2019

QilongTang left a comment

QilongTang Dec 5, 2019

QilongTang Dec 5, 2019

reddyashish commented Dec 5, 2019

reddyashish left a comment

mjkkirschner Dec 5, 2019

restore behavior of extension loadNodeLibrary method #10187

restore behavior of extension loadNodeLibrary method #10187

Conversation

mjkkirschner commented Dec 3, 2019 • edited Loading

Purpose

Update:

update2:

Declarations

Reviewers

FYIs

mjkkirschner commented Dec 3, 2019

mjkkirschner commented Dec 3, 2019

mjkkirschner commented Dec 3, 2019

aparajit-pratap commented Dec 4, 2019 • edited Loading

aparajit-pratap commented Dec 4, 2019

mjkkirschner commented Dec 4, 2019

mjkkirschner commented Dec 4, 2019

aparajit-pratap commented Dec 4, 2019

aparajit-pratap commented Dec 4, 2019

aparajit-pratap Dec 4, 2019

Choose a reason for hiding this comment

aparajit-pratap Dec 4, 2019

Choose a reason for hiding this comment

QilongTang left a comment

Choose a reason for hiding this comment

QilongTang Dec 5, 2019

Choose a reason for hiding this comment

QilongTang Dec 5, 2019

Choose a reason for hiding this comment

reddyashish commented Dec 5, 2019

reddyashish left a comment

Choose a reason for hiding this comment

mjkkirschner Dec 5, 2019

Choose a reason for hiding this comment

mjkkirschner commented Dec 3, 2019 •

edited

Loading

aparajit-pratap commented Dec 4, 2019 •

edited

Loading