Tests for CPython3 Engine as well as for IronPython. #10631

reddyashish · 2020-05-05T14:33:12Z

Purpose

This PR is in reference to the task: https://jira.autodesk.com/browse/DYN-2190
This adds more tests to the DynamoPythonTests solution and executes the current tests with both the IronPython and CPython3 Engine.

There is one failing test related to the Integer marshaling.
Update on the test:
Integers of type int64 and lower are now being marshaled but BigIntegers are failing as PythonNet doesn't have a class like the PyInt or PyLong. This will need some more work and will file a task for it.

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

@DynamoDS/dynamo

…DynamoDS#10611)

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.

* 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

… Part (DynamoDS#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.

* Cherrypick * Comments * Add unit test * Comments

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.

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.

* (1) Null reference bug fix from SQ dashboard

* Add support for debug modes

* 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

…serialization tests. (DynamoDS#10624) * reduce number of serialization tests by factor 3~ * reduce wpf json serialization tests

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.

* Cherrypick Python3 changes * Cleanup * Use Debug Modes * CleanUp * Rename Function * Clean Up * Do not use anouymous function as handler * Revert newer language change

* add ADPTracker register

src/Libraries/PythonNodeModels/PythonNode.cs

aparajit-pratap · 2020-05-07T14:11:35Z

test/Libraries/DynamoPythonTests/PythonEditTests.cs

+
+            foreach (var pythonEngine in pythonEngineVersionsList)
+            {
+                UpdateEnginePropertyForPythonNode(pynode, pythonEngine);


Is the workspace in an unsaved state at this stage due to changes to engine property?

The workspace doesn't have any changes as we are just modifying the Engine property on the node.

I think that means it does have unsaved changes- the engine property is a serializable value of the node right?

But this flag ViewModel.Model.CurrentWorkspace.HasUnsavedChanges is false.

that sounds like a bug!?

The python node object is also marked as unmodified. Should it be marked as modified if the workspace is expected to have any changes?

I will look more into this.

it makes sense to me, that if the engine is changed the node should be marked as modified as this should re-execute the graph, right?

Added the comment above to address this.

test/Libraries/DynamoPythonTests/PythonEditTests.cs

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.

reddyashish · 2020-05-08T12:51:24Z

src/Libraries/PythonNodeModelsWpf/PythonNodeModelsWpf.csproj

@@ -100,6 +100,7 @@
 	<ProjectReference Include="..\DSIronPython\DSIronPython.csproj">
      <Project>{9eef4f42-6b3b-4358-9a8a-c2701539a822}</Project>
      <Name>DSIronPython</Name>
+      <Private>False</Private>


This will not copy the DSIronPython.dll into the nodes folder as we have that binary in the debug folder. Before this, it was being copied into 2 locations.

mmisol

Left a few comments.

I guess my main concern is what to do with the failing test. If it feels it's going to take long to fix, we can probably mark it as failure and track as a separate task.

mmisol · 2020-05-08T13:00:37Z

src/Libraries/PythonNodeModels/PythonNode.cs

@@ -33,6 +37,7 @@ public abstract class PythonNodeBase : VariableInputNode
        protected PythonNodeBase(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(inPorts, outPorts)
        {
            ArgumentLacing = LacingStrategy.Disabled;
+            pythonEngineVersionsList = Enum.GetValues(typeof(PythonEngineVersion)).Cast<PythonEngineVersion>();


Please notice we are re-initializing a static field each time a Python node is created.

Also, adding a field to our code just to be used in the tests is not something we should do if we can avoid it. In this case it seems easy to avoid, we just need to a helper method in our test class.

Initially, I was using a static list in the tests to get the values. But the tests were failing because it couldn't find the PythonNodeModels.dll (the enum is defined in here) at runtime. May be the static field was the problem. Getting the list of values inside a function worked. So changed it back. Thanks @mmisol.

mmisol · 2020-05-08T13:11:51Z

test/Libraries/DynamoPythonTests/PythonEditTests.cs

+            var pythonTokens = dynObj["Nodes"].Where(t => t.Value<string>("NodeType") == "PythonScriptNode").Select(t => t);
+            Assert.IsNotNull(pythonTokens);
+            Assert.IsTrue(pythonTokens.Any(t => t.Value<string>("Engine") == PythonEngineVersion.IronPython2.ToString()));
+        }


How about if you changed the value to CPython3 before saving? This would serve to check that the property can serialize both values properly.

This test was added by Aaron to check serialization and deserialization of the Engine property on a global level. There is another test PythonNodeEnginePropertyChangeTest that is added to assert the python node values for both the engines.

I saw that test but it's not quite the same thing. My point regarding this suggestion is that it could catch a bug on serialization where the Engine of a python node would always be set to IronPython2. Not saying that's a bug we have or that it is likely to ever appear, but I just thought it would make the test more interesting to change the engine before serializing.

mmisol · 2020-05-08T13:15:24Z

test/Libraries/DynamoPythonTests/PythonEditTests.cs

        }

        [Test]
+        // This is failing for CPython3 Engine. 


Should we mark this as failing and track as a follow-up?

Marked it as failure.

mmisol

Looks good to me. Please remember to create a bug to track the test failure

mmisol · 2020-05-08T15:10:33Z

test/Libraries/DynamoPythonTests/PythonEditTests.cs

+            var pythonTokens = dynObj["Nodes"].Where(t => t.Value<string>("NodeType") == "PythonScriptNode").Select(t => t);
+            Assert.IsNotNull(pythonTokens);
+            Assert.IsTrue(pythonTokens.Any(t => t.Value<string>("Engine") == PythonEngineVersion.IronPython2.ToString()));
+        }


I saw that test but it's not quite the same thing. My point regarding this suggestion is that it could catch a bug on serialization where the Engine of a python node would always be set to IronPython2. Not saying that's a bug we have or that it is likely to ever appear, but I just thought it would make the test more interesting to change the engine before serializing.

Also updated some tests.

reddyashish · 2020-05-11T10:13:24Z

src/Libraries/PythonNodeModels/PythonNode.cs

@@ -49,7 +51,8 @@ public PythonEngineVersion Engine
                if (engine != value)
                {
                    engine = value;
-                    RaisePropertyChanged("Engine");
+                    RaisePropertyChanged(nameof(Engine));
+                    OnNodeModified();


@mjkkirschner @aparajit-pratap this will mark the node as modified when the engine property is modified. Also, the workspace will have unsaved changes.

reddyashish · 2020-05-11T12:19:59Z

test/Libraries/DynamoPythonTests/PythonEditTests.cs

-                "cb037a9debd54ce79a4007b6ea11de25",
-                "a9bb1b12fbbd4aa19299f0d30c9f99b2",
-                "b6bd3049034f488a9bed0373f05fd021"
+                "845d532f-df87-4d93-9f2e-d66509413ea6",


@QilongTang This test was not asserting the truth values as it doesn't match the GUID pattern. Had to insert the '-' character.

@reddyashish Thanks for fixing! Make sense

reddyashish · 2020-05-11T12:21:38Z

test/Libraries/DynamoPythonTests/PythonEditTests.cs

+        }
+
+        [Test]
+        public void TestWorkspaceWithMultiplePythonEngines()


This test will verify that workspace has unsaved changes when the PythonEngine property is modified on a node. Also checks that python nodes with different Python Engines in a single workspace works.

QilongTang · 2020-05-11T14:28:21Z

src/Libraries/PythonNodeModels/PythonNode.cs

@@ -82,14 +85,16 @@ protected override string GetInputTooltip(int index)
            var vals = additionalBindings.Select(x => x.Item2).ToList();
            vals.Add(AstFactory.BuildExprList(inputAstNodes));

-            Func<string, IList, IList, object> pythonEvaluatorMethod;


This will create conflict with the DSPython3 branch, just FYI

aparajit-pratap · 2020-05-11T20:32:30Z

src/Libraries/DSCPython/CPythonEvaluator.cs

+                            else if(PyLong.IsLongType(pyObj))
+                            {
+                                return PyLong.AsLong(pyObj).ToInt64();
+                            }


@reddyashish is this the fix for the int to double param method resolution issue?

curious as well as I thought that was filed as a separate bug.

also ... isn't a Long the same as Int64?

Not for double. This is the case for int64 bit integers. Before this only int32 were working.
I will be filing the issue for BigInteger marshaling.

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.

mjkkirschner · 2020-05-13T16:06:01Z

src/Libraries/DSCPython/CPythonEvaluator.cs

-                    {
-                        int amt = Math.Min(bindingNames.Count, bindingValues.Count);
+                    PythonEngine.Initialize();
+                    PythonEngine.BeginAllowThreads();


@reddyashish - you will need to tell me what is happening here 😉 - did one of your new tests introduce a deadlock?
@Dewb FYI

Just saw your comment, in what situations (which test jobs) are multiple test fixtures creating multiple python engines in the same process?

I think the explanation is here https://github.com/pythonnet/pythonnet/wiki/Threading

The Python interpreter only actually runs single-threaded. You can create additional threads, but only one thread at a time has the Global Interpreter Lock (GIL), and only that thread can run.

So that would mean only one thread should call PythonNet at a time. Not sure what test was failing though.

Sorry guys, was waiting for the self-serve. Tested it locally. I wrote the explanation below.

It isn't happening with a single test but when the PythonEngine is initialized from 2 different test fixtures. They are PythonEditTests and IronPythonTests(will probably need to change this name as we have tests for both engines in here).

reddyashish · 2020-05-13T16:27:01Z

Team, the Nunit tests were running into a deadlock situation for DynamoPythonTests.dll. This was happening specifically when the function CPythonEvalutor.EvaluatePythonScript was called from 2 different test fixtures in a single test run. They were passing separately and also when all the tests are in a single test fixture.
To avoid this, we have to call PythonEngine.BeginAllowThreads() and use thread locks during python evaluation. We will also not be initializing the PythonEngine if it is already initialized from the previous tests.

For more information, please take a look at pythonnet/pythonnet#109.

reddyashish · 2020-05-14T14:36:06Z

self-serve link: https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/603/

reddyashish · 2020-05-14T14:54:50Z

Merging this.

aparajit-pratap and others added 11 commits April 28, 2020 16:15

add namespace conflict tests for short name replacer and node to code (…

415fa68

…DynamoDS#10611)

Python Engine Enum (DynamoDS#10618)

16c6e98

* Cherrypick * Comments * Add unit test * Comments

SQ bug fix (DynamoDS#10622)

395d775

* (1) Null reference bug fix from SQ dashboard

Add support for debug modes (DynamoDS#10603)

36496ff

* Add support for debug modes

Tests for CPython3 Engine as well as for IronPython.

3d5d6ca

reddyashish added the WIP label May 5, 2020

mjkkirschner and others added 4 commits May 5, 2020 14:23

[WIP][FEEDBACK] Reduce test time by substantially reducing number of …

7391433

…serialization tests. (DynamoDS#10624) * reduce number of serialization tests by factor 3~ * reduce wpf json serialization tests

Python3 Selection Under Debug Modes (DynamoDS#10629)

0cdd190

* 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 (DynamoDS#10576)

f398219

* add ADPTracker register

aparajit-pratap reviewed May 7, 2020

View reviewed changes

mmisol and others added 2 commits May 7, 2020 12:12

Addressing some comments

0e461cc

reddyashish changed the title ~~[WIP] Tests for CPython3 Engine as well as for IronPython.~~ Tests for CPython3 Engine as well as for IronPython. May 8, 2020

reddyashish removed the WIP label May 8, 2020

reddyashish commented May 8, 2020

View reviewed changes

mmisol reviewed May 8, 2020

View reviewed changes

reddyashish added 3 commits May 8, 2020 10:49

some more comments

75fba57

Update AssemblyInfo.cs

190f913

Update AssemblyInfo.cs

0136ee6

reddyashish added the PTAL Please Take A Look 👀 label May 8, 2020

mmisol approved these changes May 8, 2020

View reviewed changes

reddyashish added 2 commits May 11, 2020 05:59

Marking the python node as modified when its engine property is modified

5804d1d

Also updated some tests.

Remove unwanted check.

1d91f14

reddyashish commented May 11, 2020

View reviewed changes

changes to test

45ea1aa

reddyashish commented May 11, 2020

View reviewed changes

QilongTang reviewed May 11, 2020

View reviewed changes

aparajit-pratap reviewed May 11, 2020

View reviewed changes

reddyashish added 3 commits May 12, 2020 07:18

Merge branch 'master' into Python3

d985ab6

Update AssemblySharedInfo.cs

eae590f

mjkkirschner reviewed May 13, 2020

View reviewed changes

reddyashish requested a review from Dewb May 13, 2020 16:41

reddyashish merged commit 734b16c into DynamoDS:DSCPython3 May 14, 2020

mjkkirschner mentioned this pull request May 27, 2021

add mechanism to reload Cpython modules and mark graph dirty #11714

Merged

13 tasks

reddyashish deleted the Python3 branch November 23, 2022 07:34

Tests for CPython3 Engine as well as for IronPython. #10631

Tests for CPython3 Engine as well as for IronPython. #10631

Conversation

reddyashish commented May 5, 2020 • edited Loading

Purpose

Declarations

Reviewers

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

reddyashish May 8, 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

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

reddyashish May 8, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner May 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddyashish May 13, 2020 • edited Loading

Choose a reason for hiding this comment

reddyashish May 13, 2020 • edited Loading

Choose a reason for hiding this comment

reddyashish commented May 13, 2020 • edited Loading

reddyashish commented May 14, 2020

reddyashish commented May 14, 2020

reddyashish commented May 5, 2020 •

edited

Loading

reddyashish May 8, 2020 •

edited

Loading

reddyashish May 8, 2020 •

edited

Loading

mjkkirschner May 11, 2020 •

edited

Loading

reddyashish May 13, 2020 •

edited

Loading

reddyashish May 13, 2020 •

edited

Loading

reddyashish commented May 13, 2020 •

edited

Loading