Add a frame range plug to Dispatcher, allowing for batched dispatching

[andrewkaufman]

We decided to use an IECore.FrameList style for this, so it should be a string plug. Some open questions:

• All dispatchers should look at their frame range and generate a list of Contexts based on it. Should Dispatcher provide a convenience function for that? This would take a Context as a prototype, and generate a vector of Borrowed contexts for each frame in the range.
  static void contexts( const Context *prototype, vector<ContextPtr> &contexts )
• Similarly, should we provide a convenience function for collecting the task list for a list of nodes? This would use contexts() internally and collect tasks per-node per-context.
  static void tasks( vector<const Node*> &nodes, ExecutableNode::Tasks &tasks )
• We need to address the fact that most ExecutableNodes aren't using the Context in executionHash, so we don't actually get new Tasks per frame yet.

[johnhaddon]

Do we even need to provide these utility methods if every dispatcher is going to have to call them anyway? What if we just do all that work privately in Dispatcher::dispatch(), and change doDispatch() to simply be Dispatcher::doDispatch( const TaskDescriptions &tasks ) - i.e. be given the fully resolved task tree upfront. Do we have use cases where a dispatcher would need to process any of that stuff itself? If not, I think I'm in favour of simplifying everything for now, and only if necessary introducing the level of granularity you suggest at a later date.

[johnhaddon]

Perhaps we should have two plugs for frame range - an enum-style frames one with options for "Current Frame Only", "Script Frame Range", and "Custom Frame Range", and a frameRange plug which would take the IECore::FrameList and only be used when frames is set to "Custom Frame Range"? I can imagine it being convenient to switch between current frame execution and execution of a custom range without having to keep typing the custom range back in.

[andrewkaufman]

I have a local commit that adds a framesModePlug with those enum values, and a frameRangePlug, and a convenience function IECore::FrameListPtr frameRange( const Context *context ) const. However, when implementing that function, I realized I don't know how to access the ScriptNode in order to query its frame range. Thoughts?

[johnhaddon]

I've been thinking about this, and I think Dispatcher is leaving too much work to the derived classes - and providing utility methods that they all end up having to call anyway only really part way solves that. It seems to me that the argument to doDispatch() should be the TaskDescriptions that are created by uniqueTasks(). That would remove redundant work that every derived class needs to do anyway, and mean that we wouldn't need the frameRange() method at all - that would all be done privately by the Dispatcher class itself before calling doDispatch().

Does that seem reasonable, or are there concerns about losing flexibility there?

[andrewkaufman]

Yeah, I agree, and I've done that already (locally) as I thought that's what you discussed in your comment a few days ago. I'm not using frameRange() within any doDispatch calls, but I don't think that removes the need for the method. Dispatcher::dispatch still needs to know the range to operate on, and the logic is complex enough that I'd rather it be its own function. Whether that's public, protected, or private is another story. Regardless, if ScriptRange is an option for the enum, then Dispatcher::dispatch needs access to the script, and I don't know how to get it at the moment, since we haven't added any Dispatcher as a child of a Script. Do you have any ideas on that? Or maybe we should removed that option, and initialize CustomRange to the current script range the first time the window opens?

Now, I still think frameRange() should be public, because I think its useful for outside parties to ask a dispatcher what frames it intends to dispatch. I can imagine Jabuka asking that in a pre-dispatch verification for example.

[johnhaddon]

Well, dispatch() takes a bunch of nodes, so you can use those to find a ScriptNode using node.scriptNode(). Sorry if I missed the point of your question - since you were using the node.scriptNode() trick already in LocalDespatcher._doDispatch() I assumed it must be about something else.

So is the problem actually that you want frameRange() to be a public method but you don't think any nodes should have to be passed to it?

[andrewkaufman]

Yeah, I guess that sums it up. Up till now we haven't required that the nodes passed to dispatch() be children of a script. The LocalDispatcher requires it (as of my last pull request), but the custom dispatcher in GafferTest.DispatcherTest doesn't. Do you think that should be a requirement of all dispatchers? If so, I'll make it explicit in the class docs and fix those tests.

If we're going that way, what are your thoughts on my public frameRange() dilemma? Currently it's only argument is a Context. I guess it should require a script and context?

[johnhaddon]

I think a script is a pretty reasonable requirement, since most (all?) dispatchers will need to serialise it to work on it elsewhere. Making that a bit more explicit as you suggest sounds good.

My inclination is more just to keep frameRange() private until we have a genuine use case, since this stuff is all a bit in flux for the moment anyway. We discussed at one point that some nodes would have valid execution ranges, so even when executing across a whole script frame range, that might be truncated down to a different valid range. If that's the case then I think frameRange() would take the same const std::vector<ExecutableNodePtr> &nodes argument that dispatch() does, and the context would be provided implicitly by Context::current() to match. But if that's the case, then I'm not sure what the real use of frameRange() is to an outsider, so I'd still be happier it staying private for now...

[johnhaddon]

Just to expand on that last message, since I don't think I was very clear. It's not clear to me what the semantics of frameRange() would be to an outside query in the face of node execution ranges truncating the asked-for range. Does it return a truncated range or not? I'd like to defer that kind of decision until we have a) Nodes with valid execution ranges b) A more immediate public need for frameRange().

On the "must have a script" front, I notice that LocalDispatcher already checks for that and outputs an error message (but returns cleanly). How about we move that check to Dispatcher::dispatch(), also check that all nodes share the same script, and throw an exception so the caller knows that something is wrong?

[andrewkaufman]

Sure thing. I wasn't sure what the preference was, throwing or reporting errors. Since the DispatcherWindow was eventually going to be tracking messages, I went for that approach. But I guess that's not as useful when dispatching outside the UI.

As a side note, throwing does kind of suck in Gaffer right now. Any exceptions I've caused have generated a large/intimidating list of dangling widgets on shutdown. We should really clean that all up before users start seeing them. Otherwise their bug reports aren't going to contain any useful info.

[johnhaddon]

The preference is mostly to think in terms of an API layer which is useful in and of itself, and then a UI layer on top of that. Generally that implies exceptions are to be preferred, as they're the natural way of communicating errors to a caller, and they can always be caught and turned into messages by a UI layer. Messages just kindof shoot out the side, so an API caller wouldn't know anything was wrong...

Python stores the last exception thrown indefinitely (until another exception is thrown). And that exception keeps all the stack frames at the point it was thrown alive, which in turn keeps all the variables alive. It's not very convenient. The argument is that you shouldn't use object lifetimes to manage resources in python anyway, instead preferring explicit calls to closeMyFileNow() and deleteMyWidgetNowAndLeaveMeWithAUselessLifelessObject() and hasSomethingElseDeletedMyWidgetWithoutTellingMe(). And the argument is also that you shouldn't worry about object reference cycles, and leave them to the garbage collector, but that leads to arbitrary amounts of slowness at arbitrary times. This Python way of things doesn't work so well with the C++ APIs we have elsewhere, where ownership of things are shared by smart pointer, and it's not reasonable to destroy a resource explicitly because someone else might reasonable expect it to still be there. But the mismatch between the "Gaffer way" and the "Python way" is causing some pain, for sure, as witnessed in those shutdown messages.

Those shutdown messages are very much just a diagnostic tool to help me make things as clean as possible and with as little use of garbage collection as possible though. A lot of the things they detect can be brute-forced-under-the-carpet at shutdown if necessary, by clearing sys.exc_info(), clearing the contents of any existing ScriptNodes and performing a full garbage collection cycle. But I would much rather fix things so we run lean and clean with as little garbage collection as possible.

Your point about confusing users and muddying up bug reports is a good one though. Maybe we run with shutdown-diagnostics for developers and brute-force cleanup for users?