DYN-6527: Fix graph update for primitive input nodes that are first initialized to null #14703

aparajit-pratap · 2023-12-06T14:33:36Z

Purpose

Dropdown nodes (inheriting from DropDownBase) that produce primitive inputs were affected by changes done in #12835 where if SelectedIndex was set to -1 (the default value), then when the node was updated to a different dropdown enum value, it wouldn't update. The node would continue to return null in this case as the SelectedIndex value of -1 builds a null node.

This issue is not specific to dropdown node and can happen when primitive input ASTs (e.g. strings) are constructed in the following way:
Case 1:

var_a = null;
var_b = var_a;
...
// update var_b
var_b = "new string"; // var_b does not update

as opposed to this case where it works:
Case 2:

var_b = "old string";
....
// update
var_b = "new string"; // var_b updates

If I change the default value of SelectedIndex in DropdownBase to 0, such that a null AST is not first constructed, the node update works as expected.

~~WIP: Trying to work on a fix to handle the case where the input variable (graphnode) is first assigned to another variable that is null, and is later updated to a new primitive value.~~

In case 1, the instructions generated look like:

==============Start Node==============
[a.0.1414]push null
[a.0.1415]pop %t_39_01312cae28ba4c749d09ffe9d1be6e4f
[a.0.1416]dep 428[ExprUID] True[SSA]
==============End Node==============
==============Start Node==============
[a.0.1417]push %t_39_01312cae28ba4c749d09ffe9d1be6e4f
[a.0.1418]pop _lx
[a.0.1419]push _lx
[a.0.1420]pop var_22759f72156c4b68930de54494586bd4
[a.0.1421]dep 428[ExprUID] False[SSA]
==============End Node==============
[a.0.1422]push null
[a.0.1423]retb

In case 2, they look like:

==============Start Node==============
[a.0.1414]push "one"
[a.0.1415]pop _lx
[a.0.1416]push _lx
[a.0.1417]pop var_0f7fcd7ccf20424f9230dc55d301231a
[a.0.1418]dep 428[ExprUID] False[SSA]
==============End Node==============
[a.0.1419]push null
[a.0.1420]retb

After the optimization to skip recompilation in #12835, execution should pick up from the instruction after the pop _lx in the update cycle for it to work, but owing to the AST being an identifier in case 1, and not a primitive, the program counter was not being set to that instruction (after the pop).

The attempted fix is to avoid doing a static single assignment (SSA) transformation to convert something like a = null; to temp_identifier = null; a = temp_identifier;

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.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

aparajit-pratap · 2023-12-07T19:07:13Z

src/Engine/ProtoAssociative/CodeGen_SSA.cs

@@ -275,7 +275,7 @@ private List<AssociativeNode> BuildSSA(List<AssociativeNode> astList, ProtoCore.
                        BinaryExpressionNode bnode = (node as BinaryExpressionNode);
                        int generatedUID = ProtoCore.DSASM.Constants.kInvalidIndex;

-                        if (context.applySSATransform && core.Options.GenerateSSA)
+                        if (context.applySSATransform && core.Options.GenerateSSA && !bnode.IsInputExpression)


Skip SSA for cases such as this:

a = null; // update a a = <some primitive>;

which would after SSA turn into this:

temp = null; a = temp; ... a = <some primitive>; // breaks!

do you think this is a good constraint to have on SSA, or should we mark this as a temporary workaround and file a task to fix the optimization in the future?

I can't see any reason why we need to SSA a simple expression like a = null; in the first place, which essentially states that each variable should be assigned exactly once.

github-actions · 2023-12-07T19:23:00Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-07T19:25:22Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-07T19:26:04Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-07T19:26:33Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-07T19:46:37Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-07T19:54:31Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-07T22:11:27Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-07T22:14:32Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-08T03:14:15Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-08T03:15:28Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

…nitialized to null (DynamoDS#14703) * remove coreclr-ncalc references * add failing test for dropdown node * cleanup * update tests * attempt initial fix * cleanup * update test * review comments * add code comments (cherry picked from commit 8987869)

* remove duplicate dyf file warning from preview generation process (#14711) - dynamo would issue a warning and fail to publish a package if an 'unqualified' file is being used, such as a dyf file already under package control - this check is done when publishing package locally, but I have incorrectly added it to the process of creating a build preview, which stops the process for both local and online submit workflow (cherry picked from commit 3ea1c5c) * update (#14710) Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> (cherry picked from commit d5e6c9b) * remove package version limitation (#14716) - now allows package version to start with 0 - cannot have 0.0.0 package version (cherry picked from commit 992e54c) * Localize menu items is Graphic Element Scale dropdown (#14714) * Fix PostDiff job * fix (cherry picked from commit 006113e) * remove dynamo sandbox app.config (#14713) * remove config * remove autogen stuff (cherry picked from commit 0947455) * DYN-6527: Fix graph update for primitive input nodes that are first initialized to null (#14703) * remove coreclr-ncalc references * add failing test for dropdown node * cleanup * update tests * attempt initial fix * cleanup * update test * review comments * add code comments (cherry picked from commit 8987869) --------- Co-authored-by: Deyan Nenov <dnenov@archilizer.com> Co-authored-by: pinzart90 <46732933+pinzart90@users.noreply.github.com> Co-authored-by: Ashish Aggarwal <ashish.aggarwal@autodesk.com> Co-authored-by: Michael Kirschner <mjk.kirschner@gmail.com> Co-authored-by: aparajit-pratap <aparajit.pratap@autodesk.com>

…nitialized to null (DynamoDS#14703) * remove coreclr-ncalc references * add failing test for dropdown node * cleanup * update tests * attempt initial fix * cleanup * update test * review comments * add code comments

* DYN-6527: Fix graph update for primitive input nodes that are first initialized to null (#14703) * remove coreclr-ncalc references * add failing test for dropdown node * cleanup * update tests * attempt initial fix * cleanup * update test * review comments * add code comments * cherry-pick PR 14637 --------- Co-authored-by: Michael Kirschner <mjk.kirschner@gmail.com>

* DYN-6527: Fix graph update for primitive input nodes that are first initialized to null (#14703) * remove coreclr-ncalc references * add failing test for dropdown node * cleanup * update tests * attempt initial fix * cleanup * update test * review comments * add code comments * cherry-pick PR 14637 * cherry-pick PR 14394 * cherry-pick PR 14740 * Fix for element binding messagebox display on saveas (#14424) * remove coreclr-ncalc references * fix for element binding messagebox on saveas * cherry pick PRs 14424, 14500 * build fix --------- Co-authored-by: Michael Kirschner <mjk.kirschner@gmail.com>

aparajit-pratap added 30 commits August 23, 2023 15:53

remove coreclr-ncalc references

64b4706

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

4a82ae7

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

dbc3e12

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

81ace20

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

a7cafdc

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

55c654d

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

7ede2a4

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

c43a02d

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

1951d1a

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

cd100b1

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

c04c193

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

35edbdb

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

5c6a4a3

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

1641149

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

eff8d9b

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

8415e17

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

8a3368b

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

3350614

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

be0a127

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

a3d62e9

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

a3c806e

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

e93499d

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

a551e0e

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

1889a02

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

f3a7315

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

06e96a0

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

823d317

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

f411221

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

7c590f3

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

02e4acf

aparajit-pratap commented Dec 7, 2023

View reviewed changes

update test

600fea5

review comments

d688294

aparajit-pratap changed the title ~~[WIP] Test input modification graph updates with dropdown nodes~~ DYN-6527: Test input modification graph updates with dropdown nodes Dec 7, 2023

aparajit-pratap changed the title ~~DYN-6527: Test input modification graph updates with dropdown nodes~~ DYN-6527: Fix graph update for primitive input nodes that are first initialized to null Dec 7, 2023

aparajit-pratap requested review from sm6srw, pinzart90 and saintentropy December 7, 2023 21:47

add code comments

ba8f011

aparajit-pratap requested a review from mjkkirschner December 8, 2023 17:15

mjkkirschner approved these changes Dec 8, 2023

View reviewed changes

aparajit-pratap merged commit 8987869 into DynamoDS:master Dec 8, 2023
22 checks passed

aparajit-pratap deleted the testDropdown branch December 8, 2023 17:37

saintentropy mentioned this pull request Dec 9, 2023

Diff between RC branch and 3.1 Switch on Main branch #14725

Merged

9 tasks

aparajit-pratap mentioned this pull request Mar 14, 2024

Cherry-pick PRs 14703, 14637 #15020

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6527: Fix graph update for primitive input nodes that are first initialized to null #14703

DYN-6527: Fix graph update for primitive input nodes that are first initialized to null #14703

aparajit-pratap commented Dec 6, 2023 •

edited

Loading

aparajit-pratap Dec 7, 2023

mjkkirschner Dec 7, 2023

aparajit-pratap Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

DYN-6527: Fix graph update for primitive input nodes that are first initialized to null #14703

DYN-6527: Fix graph update for primitive input nodes that are first initialized to null #14703

Conversation

aparajit-pratap commented Dec 6, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

aparajit-pratap Dec 7, 2023

Choose a reason for hiding this comment

mjkkirschner Dec 7, 2023

Choose a reason for hiding this comment

aparajit-pratap Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 7, 2023

UI Smoke Tests

github-actions bot commented Dec 8, 2023

UI Smoke Tests

github-actions bot commented Dec 8, 2023

UI Smoke Tests

aparajit-pratap commented Dec 6, 2023 •

edited

Loading