Skip to content

DYN-1899: Node Execution Events #9823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
Jul 1, 2019
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
56dbd73
Update changes from librarie.js to fix QNTM-3710
ColinDayOrg Mar 22, 2018
eec64b9
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Mar 23, 2018
4362151
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jul 19, 2018
75643c7
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 2, 2018
6a07e6c
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 19, 2018
38b8461
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 21, 2018
44a00a8
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 29, 2018
5c04a06
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 7, 2018
a21fd6a
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 10, 2018
4235133
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
302b9dd
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
6bf5548
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
8d4511b
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 21, 2018
8f9df0f
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 8, 2019
e91ef30
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 9, 2019
82373cc
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 10, 2019
1434ba0
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 10, 2019
706974c
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 16, 2019
760ae95
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 4, 2019
6ffd883
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 15, 2019
5b1c073
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 27, 2019
f11f924
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Mar 27, 2019
710c7ee
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Apr 1, 2019
e0be270
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jun 3, 2019
c2dc9c2
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jun 14, 2019
5bbc94b
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jun 17, 2019
a7e3a82
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jun 24, 2019
49604e9
Add new events and tests
ColinDayOrg Jul 1, 2019
5609485
Remove unnecessary arguments
ColinDayOrg Jul 1, 2019
39d6e14
Remove unneeded arguments
ColinDayOrg Jul 1, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/DynamoCore/Engine/Profiling/NodeProfilingData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ private void RecordEvaluationState(object data)
if (!startTime.HasValue)
{
startTime = DateTime.Now;
node.OnNodeExecutionBegin(data);
return;
}

endTime = DateTime.Now;
node.OnNodeExecutionEnd(data);
}

internal TimeSpan? ExecutionTime
Expand Down
32 changes: 32 additions & 0 deletions src/DynamoCore/Graph/Nodes/NodeModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,38 @@ private void OnDispatchedToUI(object sender, UIDispatcherEventArgs e)
/// </summary>
public event Action<PortModel> PortDisconnected;

public class NodeExecutionEventArgs : EventArgs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the public properties and the class itself should have some ///summary tags please.

{
public Guid GUID { get; private set; }
public object Data { get; private set; }

public NodeExecutionEventArgs(NodeModel model, object data)
{
this.GUID = model.GUID;
this.Data = data;
}
}

/// <summary>
/// Event triggered before a node is executed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment should indicate that these events are only fired when profiling is enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively we could rename them to be less general.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes have been added to both events to indicate that they will only be fired when profiling is turned on. My thought is that the names should remain general in the case that it is decided to make the event firing non-dependent on profiling being turned on so to not break the API.

/// </summary>
public event Action<NodeModel, NodeExecutionEventArgs> NodeExecutionBegin;

internal void OnNodeExecutionBegin(object data)
{
NodeExecutionBegin?.Invoke(this, new NodeExecutionEventArgs(this, data));
Copy link
Member

@mjkkirschner mjkkirschner Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it appears this passes the nodes evaluated output to this handler?
Can you confirm that in one of the tests?
To clarify, I mean, what is data here?

}

/// <summary>
/// Event triggered after a node is executed.
/// </summary>
public event Action<NodeModel, NodeExecutionEventArgs> NodeExecutionEnd;

internal void OnNodeExecutionEnd(object data)
{
NodeExecutionEnd?.Invoke(this, new NodeExecutionEventArgs(this, data));
}

#endregion

#region public properties
Expand Down
57 changes: 56 additions & 1 deletion test/DynamoCoreTests/ProfilingTest.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
using System.Collections.Generic;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;

using Dynamo.Graph.Workspaces;
using Dynamo.Models;
using Dynamo.Graph.Nodes;
using NUnit.Framework;

namespace Dynamo.Tests
Expand Down Expand Up @@ -94,5 +96,58 @@ public void TestProfilingSingleNodePublicMethodsOnly()
Assert.IsNotNull(profilingData.NodeExecutionTime(node));
Assert.Greater(profilingData.NodeExecutionTime(node)?.Ticks, 0);
}

private static List<Guid> executingNodes = new List<Guid>();
private static int nodeExecutionBeginCount = 0;
private static int nodeExecutionEndCount = 0;

private static void onNodeExectuionBegin(NodeModel sender, NodeModel.NodeExecutionEventArgs args)
{
// Assert that the node has ot been executed more than once
CollectionAssert.DoesNotContain(executingNodes, args.GUID);

executingNodes.Add(args.GUID);
nodeExecutionBeginCount++;
}

private static void onNodeExectuionEnd(NodeModel sender, NodeModel.NodeExecutionEventArgs args)
{
// Assert that the node ending execution had the begin exectuion event fired previously
CollectionAssert.Contains(executingNodes, args.GUID);

nodeExecutionEndCount++;
}

[Test]
public void TestNodeExecutionEvents()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add test that asserts the value of the data object? Or get rid of that property if we think it should not be passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking into the tests for the data object it seemed to me that the data object was redundant because the NodeModel object was the important information which was already being passed in. I removed the class that contained the NodeModel GUID and the now extraneous data and just rely on the NodeModel object that was being passed through already.

{
// Note: This test file is saved in manual run mode
string openPath = Path.Combine(TestDirectory, @"core\profiling\createSomePoints.dyn");
OpenModel(openPath);

var nodes = CurrentDynamoModel.CurrentWorkspace.Nodes;
foreach(var node in nodes)
{
node.NodeExecutionBegin += onNodeExectuionBegin;
node.NodeExecutionEnd += onNodeExectuionEnd;
}

// Currently the node execution begin/end events are only enabled when profiling is enabled
var engineController = CurrentDynamoModel.EngineController;
var homeWorkspace = CurrentDynamoModel.Workspaces.OfType<HomeWorkspaceModel>().FirstOrDefault();
engineController.EnableProfiling(true, homeWorkspace, nodes);

BeginRun();

// Assert that all nodes were executed and the begin and end events were fired as expectd
Assert.AreEqual(4, nodeExecutionBeginCount);
Assert.AreEqual(4, nodeExecutionEndCount);

foreach (var node in nodes)
{
node.NodeExecutionBegin -= onNodeExectuionBegin;
node.NodeExecutionEnd -= onNodeExectuionEnd;
}
}
}
}