ExecuteButton (and Ctrl+E/R shortcuts) should use the current despatcher

[andrewkaufman]

This was extracted from #109, since the scope of that issue has been limited.

Currently the Execute button just calls execute() directly on the node, without using a Despatcher of any kind. It should use the LocalDespatcher instead, so that preDespatchSIgnal() and postDespatchSignal() can be relied upon in all cases.

We should also add an option to the LocalDespatcher - "execute in background", so that it's possible to execute either in the current session in a blocking way, or in a background process.

[johnhaddon]

I think there should be a concept of "the current despatcher" (in the UI, not the API) which is what the Ctrl+E/R shortcuts use. It should default to the LocalDespatcher, but could be set to a FarmDespatcher of some sort if that's what the user wants.

I think "execute in background" should be the default for the LocalDespatcher - I just looked and was surprised to find it didn't operate that way already...

[andrewkaufman]

Note that Ctrl+R is broken currently, as script._executeUILastExecuted doesn't seem to stick outside the _execute call in DespatcherUI.

[johnhaddon]

Yep - that's an unintended consequence of ScriptNode using the new leaner Cortex wrappers. There are ways we could work around it in DespatcherUI, but I'm trying to come up with a way of dealing with it better on the Cortex side. So for now I've been keeping it as a little thorn in my side to encourage me to do better...

[andrewkaufman]

I'm about to add this current dispatcher concept, and it seemed natural to do it the same way as _executeUILastExecuted. Given the deadlines, I'm tempted to just get both working in DispatcherUI. Do you have a preferred way of doing that? Or do you think the Cortex side fix isn't far off?

[andrewkaufman]

Also, any advice on how to execute in the background? Is that going to have to be a system call or some QThread magic? How should we go about saving the scene before starting the background execution (#310)?

[andrewkaufman]

I think we should include a basic launcher UI as well. So pressing Execute (or Ctrl+e) would launch the Dispatch UI, where the dispatcher plugs, like executeInBackground and frameRange (#878), would be exposed. Should this UI just be the NodeEditor for the Dispatcher node launched as a popup?

When the UI is first launched, it should set the frame range to match the script frame range. After that, it should respect the plug value.

Ctrl+R shouldn't launch the UI but rather re-dispatch with the existing settings.

[johnhaddon]

Do you think the current dispatcher would be a per-script thing then? I kindof think it might be a global or per-app thing. I think I'd find it strange to open a new script, and find that the current dispatcher had changed.

For background execution I think I'm in favour of ExecutableNode::execute() getting called in a separate process for a couple of reasons :

• Avoid crashing the main app if execution goes bad.
• Match more closely what would happen with a farm dispatcher. The gui app runs a lot of UI config files and loads all the UI modules too, and that won't happen on the farm.

There's already an app called "execute" which will execute a set of nodes over a range of frames. I was thinking that that would be the low level tool that dispatchers would use to execute something in a separate process. If that seems reasonable, then I think it implies that we should use a background thread in the main gui app to control the process, but have it launch gaffer execute via subprocess.Popen() to dispatch each clump of operations. Having the coordinating work done in the gui process will make it a lot easier to report back progress/errors via the UI. Does that sound OK?

ScriptNode::serialiseToFile() will do the job for saving a temp copy of the scene. But we also need to decide where to save it, and how that is specified. Since most dispatchers will need to save some temporary files, I think there should be a string plug on the base class to specify this location. Then we can use a config file to set that to a sensible default location like $project:rootDirectory/despatcher.

I think the launcher UI should be a NodeUI for the current despatcher, rather than a full blown NodeEditor. The NodeEditor displays some irrelevancies like the node name and type, and also expects the node to live on a script. Making our own Window and putting a NodeUI in it let's us achieve the same thing without the extra gumph.

[andrewkaufman]

So should currentDispatcher be stored in the app preferences then? Maybe with a dropdown based on registered dispatcher names?

The Popen/execute bit sounds fine to me. Are there any good examples of maintaining a background thread which reports feedback to the main thread? Or should I check Jabuka for that?

For the default serialseToFile location, what do you think about as subdir it in the script dir? ${project:rootDirectory}/scripts/dispatcher or ${project:rootDirectory}/scripts/.dispatcher or ${project:rootDirectory}/scripts/backup

[johnhaddon]

I'm not sure where currentDispatcher should be stored right now - maybe we can decide that at a later date when we have the luxury of more than one dispatcher implementation?

The OpDialogue class provides a pretty simple example of doing stuff in the background. It just uses the standard threading.Thread() python class to launch stuff on a thread, and then calls GafferUI.EventLoop.executeOnUIThread() when it wants to trigger something in the UI. I'd avoid using any Qt stuff directly - that's just an implementation detail as far as GafferUI is concerned and operating at a higher level is preferred. The GafferUI.MessageWidget also takes care of receiving IECore.msg() messages on any thread and ensuring that they're displayed on the main UI thread - that might be useful for collecting and reporting feedback, message logs etc.

There's one wrinkle in the threading stuff though, and that is that GafferUI.EventLoop.executeOnUIThread() etc is only available from GafferUI and LocalDespatcher is in Gaffer so can't depend on that. We need to decide what that means for our design - perhaps it's the UI that launches the background thread and calls despatch() on it?

For the serialiseToFile() location, I don't think ${project:rootDirectory}/scripts is ideal - I'd rather leave users to manage what's in there however they want, without the confusion of auto-generated files mixed in. I suggested $project:rootDirectory/despatcher because I thought some despatchers would want to store other temporary items too (tractor scripts maybe?). Also, such temporary files might want to be cleaned up on a per-job basis, which also implies that they should be in their own directory well away from user files.

[andrewkaufman]

So are you thinking a plug called dispatcherTempDir which isn't specific to the temp script file name? I had originally thought you meant fileName specifically, and to call serialiseToFile() from the base class directly before doDisptach, but re-reading this now I'm not sure what you intended.

About squirreling away the temp script files, users of other apps do often want access to the farm scene directly, and might find it annoying to have to go digging around for it, which is why I thought at least specifying that path explicitly would be better.

[johnhaddon]

Yes, I was thinking of a plug to define a generic temp directory for dispatcher related stuff. But I don't think the base class should necessarily automatically call serialiseToFile() automatically - it seems reasonable that some Dispatcher implementations might just operate directly without needing the script saved at all. I was thinking the derived class would call serialiseToFile(), but using the location defined by the base class.

I don't think ${project:rootDirectory}/despatcher/${jobName}/theScriptFile.gfr is necessarily too squirrelled away is it? And if it had ${project:rootDirectory}/despatcher/${jobName}/theTractorScript.alf (or whatever other things the job needs) nicely alongside it'd be quite tidy I thought. And you could clean up jobs quite easily, without worrying about mucking around deleting things in the precious scripts directory, or from multiple different locations.

I don't want this discussion to be a blocker on making progress though - any location is better than the current behaviour of ExecutableRender::execute() overwriting the user's current script.

[andrewkaufman]

Actually, immediately after writing that, I realized the LocalDispatcher needs control over the serializing, to avoid it when running in blocking mode (executeInBackground == False). I can make the tempDir plug instead, but I have a couple questions. Should Dispatcher be creating the tempDir in dispatch, or should that be left to the individual dispatchers as well? What is the appropriate config file to set the tempDir plug value? Is ${jobName} a real variable already?

[johnhaddon]

I think the Dispatcher base class should probably just have a protected jobDirectory() method which the derived classes call when they want somewhere to put things - if it doesn't get called then no directory gets created.

[johnhaddon]

Oh, and ${jobName} isn't a real variable no - I presume we'll want a way of specifying a name for each dispatched job though? Would that be another plug on the base class? I'm not sure we should make it a variable in the sense of a Context entry yet - for now we could just use the value of the plug for determining the directory name and the heading in the LocalDispatcherUI.

[andrewkaufman]

And which gui app config to set it from?

[andrewkaufman]

For executeInBackground, I'm wondering if we've under-thought this... If the UI starts the thread (regardless of which dispatcher is being used), then does localDispatcher["executeInBackground"] actually do anything? Isn't executeInBackground the only option at this point, in which case, should we just ditch that plug?

Gaffer.Dispatcher._uniqueTasks returns a list of Node,Context pairs to be executed. The execute app accepts a node and frame, but not a context. Is it safe to assume the only context entry that varies between Tasks is the frame? Or is there a way to pass a context via command line?

Using the execute app frames parameter limits us to whole frame execution (since FrameLists only accept ints). Again, passing the real context might be better.

[johnhaddon]

I arrived at the same conclusion about executeInBackground last night - let's give it a go as being something just the UI worries about (no plug) and see how it pans out.

Good point about the execute app - do you think it's reasonable to extend that with a parameter to deal with arbitrary contexts or did you have some other mechanism in mind? I think my preference is to extend the execute app - for certain farm dispatchers (Tractor from what I remember) everything will have to be expressed as a command line anyway, so we might as well get that functionality in now rather than invent two separate mechanisms. Since the LocalDispatcher is going to be implemented in Python, it's pretty easy to convert context entries to human readable text with repr( context["someKey"] ) and then put them back in with context["someKey"] = eval( theText ).

When we support clumping and must-execute-all-at-onceness, do you anticipate clumping together Contexts which differ in more than just the frame? If not, then we can just have a parameter for specifying a constant base context, and use the existing frames parameter to vary on top of that. Since the frame range the user selects before dispatch is a FrameList, let's work on the assumption that the FrameList parameter is sufficient for now - it won't be too hard to add an alternative means of specifying frames later, but let's not complicate things until it's actually necessary.

So I think the command line format would basically look like this :

gaffer execute -script filename.gfr -nodes MyImageWriter -frames 1-10 -context -exampleFloat 10.0 -exampleInt 4 -exampleString foo

The op app has an example of how to set up a parameter to accept multiple flag arguments as the context argument does above.

[andrewkaufman]

I have this mostly wrapped up, but I'm running into an issue with the -context args. Most of them work fine, but some of them are more complex and fail. Is it expected that any entry in the context should be serialisable with repr?

It seems the context from any of the image related ExecutableNodes (renders and writers) includes image:defaultFormat which is GafferImage.FormatData and I run into trouble. A simple test shows the same problem I think:

d = GafferImage.FormatData()
q = eval( repr(d) )