Skip to content
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

ExecutableNode Process support #1671

Merged
merged 15 commits into from
Mar 21, 2016
Merged

Conversation

johnhaddon
Copy link
Member

This does the following :

  • Adds an API to TaskPlug to provide saner public access to the ExecutableNode methods, which are now "protected" (documented as protected but not actually protected yet for backwards compatibility reasons).
  • Exposes the processing performed by ExecutableNodes via the Process class. This makes it possible to monitor them.
  • Makes automatic string substitution work for ExecutableNodes (Support automatic variable substitution during ExecutableNode::execute() #887).
  • Removes the manual string substitutions from the various nodes which had bothered to implement them.

I think it's important that we do this, as it puts the ExecutableNode on a much better footing alongside the rest of our APIs, and #887 is the cause of numerous bugs (some of them yet to be discovered). It'd be great if we could go a bit further and actually break compatibility to get things really tidy, but I haven't done that here since we were hoping for 0.23 to remain the major version for a while.

It's important to note that although substitutions are now automagic, that is only the case if you use the TaskPlug API instead of the deprecated "protected" API to execute nodes (since I removed the manual substitutions). I've updated all Gaffer's code so this is the case, but there may be other code calling ExecutableNode::execute() directly rather than via a dispatcher (in Caribou perhaps?). If we're concerned about that, then I can split the removal of manual substitutions out of this PR, and put it into one we merge at a later date.

@andrewkaufman
Copy link
Contributor

Seems like an odd test to have failed on gcc 5 only

======================================================================

ERROR: testSerialisation (GafferUITest.EditorWidgetTest.EditorWidgetTest)

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/build/ImageEngine/gaffer/install/gaffer-0.23.1.0-linux/python/GafferUITest/TestCase.py", line 58, in tearDown

self.assertEqual( widgetInstances, [] )

AssertionError: Lists differ: [<GafferUI.TabbedContainer.Tab... != []

First list contains 6 additional elements.

First extra element 0:

<GafferUI.TabbedContainer.TabbedContainer object at 0x327aed0>

+ []

- [<GafferUI.TabbedContainer.TabbedContainer object at 0x327aed0>,

- <GafferUI.BusyWidget.BusyWidget object at 0x32830d0>,

- <GafferUI.Widget.Widget object at 0x327af50>,

- <GafferUI.InfoPathPreview.InfoPathPreview object at 0x327ac10>,

- <GafferUI.PathListingWidget.PathListingWidget object at 0x327ac50>,

- <GafferUI.Spacer.Spacer object at 0x3283110>]

@andrewkaufman
Copy link
Contributor

So I guess its now possible to monitor an executable, but we haven't exposed any monitors that do so? What did you have in mind for such a monitor?

@johnhaddon
Copy link
Member Author

I don't have any particular use cases for executable monitoring (maybe dispatcher debugging?), but I think as a general principle we should allow monitoring of any type of node process. Really, the main benefit right now is having the automatic substitutions...

These operate by using the current context to call the existing virtual implementation methods appropriately. The Task class has also been modified to replace the stored ExecutableNode with a TaskPlug.

This has the following benefits :

- Consistency with context management in the rest of the API. The context is now supplied implicitly by
  `Context::current()` rather than explicitly by arguments to the public methods. This makes for easier coding
  when performing several operations in the same context. It also fixes an API flaw spot whereby ExecutableNode
  clients were expected to not only make their context current, but they also to pass it to the ExecutableNode
  methods explicitly. This was easy to get wrong, either by omitting the context scoping or by passing the wrong
  context to the methods.
- Consistency with the ValuePlug API, which also hides all calls to internal ComputeNode methods.
- Nodes will be able to have more than one output task in the future.

The ExecutableNode virtual interface is now considered protected, and several methods are deprecated, but this is fully backwards compatible with the old interface.
Also use new TaskPlug API in preference to "protected" ExecutableNode API.
This makes it possible to monitor using a custom Monitor subclass. It also means that string plugs are automatically substituted during execution, relieving ExecutableNode implementations of the responsibility of doing that manually, and fixing numerous bugs where they weren't.

Fixes GafferHQ#887.
@johnhaddon
Copy link
Member Author

I've split out the removal of the manual string substitutions into #1674, so that can be merged for a future major version, and this can be merged now.

andrewkaufman added a commit that referenced this pull request Mar 21, 2016
@andrewkaufman andrewkaufman merged commit 98cec69 into GafferHQ:master Mar 21, 2016
@johnhaddon johnhaddon deleted the executeProcess branch March 22, 2016 10:55
johnhaddon added a commit to johnhaddon/gaffer that referenced this pull request Apr 7, 2016
This was broken by GafferHQ#1671. Although automatic substutions are generally a great thing, in this case they were not.
johnhaddon added a commit to johnhaddon/gaffer that referenced this pull request Apr 7, 2016
This was broken by GafferHQ#1671. Although automatic substutions are generally a great thing, in this case they were not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants