DYN1198- Heterogeneous list update gives infinite loop #9408

reddyashish · 2019-01-10T22:47:20Z

Purpose

This PR is to fix the Heterogeneous list crash that was reported. The infinite loop happens when more than 2 different data types of inputs are used in a list (like String, Int, Double) and that list is passed to some function.
When calculating replication combinations for a function, there is a "for" loop in Replicator.cs(Lines 217-245) that has a complexity in exponential order. The "reducedParams" list will be containing more than 10^10 possible combinations, which I think is redundant as they are being duplicated. So removed the "for" loop from there and then calculate the value of "arrayStats" (excluding duplicates) from each of the targets in the "targets" list. "arrayStats" will generally have all the distinct types of data that are passed into the function.

The graph "test_hetereogenous_list.dyn" in this PR demonstrates the case which was running into infinite loop before. The infinite loop crash and the regressions that were caught last time(on Dynamo master) are fixed here. However there is one particular case that is failing, as demonstrated in "TestTryGetValuesFromDictionary09" test with the nested dictionaries. This used to fail before but we dint catch it until now.
I have added that test with the "Category : Failure" attribute so that we can revisit this.

Declarations

Check these if you believe they are true

The code base 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

@aparajit-pratap @QilongTang @mjkkirschner

https://jira.autodesk.com/browse/QNTM-3928

This reverts commit 085fb51, reversing changes made to d055de4.

…Dynamo"" This reverts commit 12538ab.

DYN-1198

This reverts commit d91009e.

This reverts commit e8d7b8a.

This reverts commit 1458b46.

Use Polysurface.ByJoinedSurfaces node instead.

This reverts commit 2ed772f.

mjkkirschner · 2019-01-11T19:13:03Z

@reddyashish thanks for the write up! 😄

reddyashish · 2019-01-11T19:23:36Z

@aparajit-pratap I have added that failing test now.

mjkkirschner · 2019-01-11T19:45:05Z

looks good to me

mjkkirschner · 2019-01-11T19:45:42Z

@Racel @aparajit-pratap @reddyashish do we have a general workaround that we can add to the release notes?

reddyashish · 2019-01-11T20:49:50Z

My thought would be to suggest, not using more than one level of nesting for heterogenous lists that are passed to any function.

mjkkirschner · 2019-01-12T01:54:04Z

src/Engine/ProtoCore/Utils/ArrayUtils.cs

@@ -117,25 +118,43 @@ public static ClassNode GetGreatestCommonSubclassForArray(StackValue array, Runt
            return runtimeCore.DSExecutable.classTable.ClassNodes[orderedTypes.First()];
        }

-        public static Dictionary<int, StackValue> GetTypeExamplesForLayer(StackValue array, RuntimeCore runtimeCore)
+        public static IEnumerable<StackValue> GetTypeExamplesForLayer(StackValue array, RuntimeCore runtimeCore)


this is an API break I believe.

a Dictionary is an System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<TKey,TValue>> not general Ienumerable of T> I don't think.

I would really appreciate a summary for this method too 😈

@mjkkirschner the problem with GetTypeExamplesForLayer was that it returned with only 1 ArrayPtr even if there were [ArrayPtr, ArrayPtr] but the 2 array pointers can be different. It would always return with the first one.

So it would fail for such cases: [[1,2,3], [{dict}]] and for [[dict], [[dict]]] for the same reason, where dict is any dictionary. The fix is to add all ArrayPtr as unique items in the dictionary and return them.

src/Engine/ProtoCore/Utils/ArrayUtils.cs

mjkkirschner · 2019-01-12T02:02:15Z

src/Engine/ProtoCore/Utils/ArrayUtils.cs

-
-            return usageFreq;
+            // return flattened list of unique values
+            return usageFreq.Values.SelectMany(x => x.Select(y => y));


same question about selectMany

mjkkirschner · 2019-01-12T02:03:13Z

@aparajit-pratap looks good except for the API break and couple questions - can you now remove the failure category from the tests?

reddyashish · 2019-01-12T02:13:32Z

Removed the failure category from the tests and started the self serve.

…DYN1198

aparajit-pratap · 2019-01-12T13:40:59Z

@reddyashish I think the self-serve has passed other than the excel tests (which fail all the time). I'm merging this into master for now.

mjkkirschner · 2019-01-12T21:42:40Z

@aparajit-pratap are you intending to cherry pick this today?

* Heterogenous list crash fix * Adding test case for heterogeneous list crash * Adding tests * Adding new test * fix remaining edge cases * revert assemblysharedinfo file * Updating failing tests * add new method to prevent API break * simplify SelectMany

aparajit-pratap · 2019-01-13T01:54:32Z

@mjkkirschner cherry-picked here: #9415

reddyashish · 2019-01-13T16:52:14Z

Thanks Aparajit.

* Heterogenous list crash fix * Adding test case for heterogeneous list crash * Adding tests * Adding new test * fix remaining edge cases * revert assemblysharedinfo file * Updating failing tests * add new method to prevent API break * simplify SelectMany

#9415)" This reverts commit 5ddd3d5.

* Cherry-picking #9388 into 2.0.3 * Cherry-picking #9559 into 2.0.3 * Cherry-picking #9578 into 2.0.3 * cherry-picking #9632 into 2.0.3 * cherry-picking #9408 into 2.0.3 * cherry-picking #9441 into 2.0.3 * Adding gradient.png for the Test_PerforationsByImage test. This was missed while cherrypicking #9441 * Removing DSCoreDataTests.cs as this was the test fixture was introduced in a different commit and is not needed here.

reddyashish added 30 commits December 3, 2018 20:56

Add check for invalid cutom node name

7156f6e

https://jira.autodesk.com/browse/QNTM-3928

Removing trailing spaces

5c88c44

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

827b090

Moving the function to utilities class

d055de4

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

085fb51

Revert "Merge branch 'master' of https://github.com/DynamoDS/Dynamo"

12538ab

This reverts commit 085fb51, reversing changes made to d055de4.

Revert "Revert "Merge branch 'master' of https://github.com/DynamoDS/…

101857b

…Dynamo"" This reverts commit 12538ab.

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

5659d22

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

c3a97df

Adding tests - QNTM-3928

6987cfa

Comments

68194bd

Moving the test to NodeViewCustomizationTests.cs

77193b5

Deleting changes to SerializationTests.cs

3b522b1

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

2b85fc0

Create SerializationTests.cs

68eeba9

Update SerializationTests.cs

4a76a7a

moving the test to UtilityTests.ca

bb11dd8

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

9b37a3d

Heterogeneous list update gives infinite loop

e8d7b8a

DYN-1198

Moving the test dyn file

d91009e

Revert "Moving the test dyn file"

02b0d99

This reverts commit d91009e.

Revert "Heterogeneous list update gives infinite loop"

71f2ca4

This reverts commit e8d7b8a.

Merge branch 'master' of https://github.com/reddyashish/Dynamo

11693cd

making Surface.join node obsolete

1458b46

Revert "making Surface.join node obsolete"

a4e0e98

This reverts commit 1458b46.

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

ee59554

Obsoleting Surface,Join node.

2ed772f

Use Polysurface.ByJoinedSurfaces node instead.

Heterogenous list crash fix

52b95a0

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into DYN1198

9a0681b

Revert "Obsoleting Surface,Join node."

10c3c3e

This reverts commit 2ed772f.

Adding new test

2c1fe9c

aparajit-pratap added 2 commits January 11, 2019 20:37

fix remaining edge cases

9929026

revert assemblysharedinfo file

4f3f6d9

mjkkirschner reviewed Jan 12, 2019

View reviewed changes

src/Engine/ProtoCore/Utils/ArrayUtils.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jan 12, 2019

View reviewed changes

Updating failing tests

5d75240

aparajit-pratap added 3 commits January 11, 2019 22:26

add new method to prevent API break

b21d0d5

Merge branch 'DYN1198' of https://github.com/reddyashish/Dynamo into …

0fd9e37

…DYN1198

simplify SelectMany

c26dcac

aparajit-pratap merged commit 0559802 into DynamoDS:master Jan 12, 2019

aparajit-pratap mentioned this pull request Jan 13, 2019

Cherry-pick DYN1198- Heterogeneous list update gives infinite loop (#9408) #9415

Merged

7 tasks

reddyashish added a commit that referenced this pull request Jan 14, 2019

Revert "DYN1198- Heterogeneous list update gives infinite loop (#9408) (

46039f8

#9415)" This reverts commit 5ddd3d5.

aparajit-pratap mentioned this pull request Jan 15, 2019

Revert partial fix to heterogeneous lists #9429

Merged

7 tasks

reddyashish mentioned this pull request May 21, 2019

Cherry-pick language changes into 2.0.3 branch for Refinery #9730

Merged

7 tasks

reddyashish added a commit to reddyashish/Dynamo that referenced this pull request May 23, 2019

cherry-picking DynamoDS#9408 into 2.0.3

65ef260

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN1198- Heterogeneous list update gives infinite loop #9408

DYN1198- Heterogeneous list update gives infinite loop #9408

reddyashish commented Jan 10, 2019 •

edited

Loading

mjkkirschner commented Jan 11, 2019

reddyashish commented Jan 11, 2019

mjkkirschner commented Jan 11, 2019

mjkkirschner commented Jan 11, 2019

reddyashish commented Jan 11, 2019

mjkkirschner Jan 12, 2019

mjkkirschner Jan 12, 2019 •

edited

Loading

mjkkirschner Jan 12, 2019

aparajit-pratap Jan 12, 2019 •

edited

Loading

mjkkirschner Jan 12, 2019

mjkkirschner commented Jan 12, 2019

reddyashish commented Jan 12, 2019

aparajit-pratap commented Jan 12, 2019 •

edited

Loading

mjkkirschner commented Jan 12, 2019

aparajit-pratap commented Jan 13, 2019

reddyashish commented Jan 13, 2019

DYN1198- Heterogeneous list update gives infinite loop #9408

DYN1198- Heterogeneous list update gives infinite loop #9408

Conversation

reddyashish commented Jan 10, 2019 • edited Loading

Purpose

Declarations

Reviewers

mjkkirschner commented Jan 11, 2019

reddyashish commented Jan 11, 2019

mjkkirschner commented Jan 11, 2019

mjkkirschner commented Jan 11, 2019

reddyashish commented Jan 11, 2019

mjkkirschner Jan 12, 2019

Choose a reason for hiding this comment

mjkkirschner Jan 12, 2019 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Jan 12, 2019

Choose a reason for hiding this comment

aparajit-pratap Jan 12, 2019 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Jan 12, 2019

Choose a reason for hiding this comment

mjkkirschner commented Jan 12, 2019

reddyashish commented Jan 12, 2019

aparajit-pratap commented Jan 12, 2019 • edited Loading

mjkkirschner commented Jan 12, 2019

aparajit-pratap commented Jan 13, 2019

reddyashish commented Jan 13, 2019

reddyashish commented Jan 10, 2019 •

edited

Loading

mjkkirschner Jan 12, 2019 •

edited

Loading

aparajit-pratap Jan 12, 2019 •

edited

Loading

aparajit-pratap commented Jan 12, 2019 •

edited

Loading