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

Dispatch App #2588

Merged
merged 27 commits into from
Jun 7, 2018
Merged

Dispatch App #2588

merged 27 commits into from
Jun 7, 2018

Conversation

andrewkaufman
Copy link
Contributor

I've added a new app called dispatch which can be used from the commandline or via a new DispatchDialogue to dispatch graphs. It can take an existing script and a list of nodes to dispatch, or it can be given node classes and construct the script on-the-fly.

ecbcba6 is a bit hacky, but I've discussed it with @johnhaddon and we think its the best way to go for now. I'd also like to avoid 83ba507 if we can think of a way to wrap that up outside the gui app startup.

This dialogue is primarily for use in the gui mode of the upcoming `dispatch` app, but may be used separately for custom dispatching.
This exposes the tool menu, which provides node documentation, convenience functions like "revert to defaults", as well as the ability to add customized behaviour (such as per-node presets).
This allows startup files to modify the application parameters, which can be useful for enforcing facility specific settings.
This entire file was copied directly from startup/gui/project.py and should likely be kept in-sync. Is it desirable to maintain this separately from the gui app or should we unify project variables and initial dispatcher settings across the apps?

self.__script = script
# hold a reference to the script window so plugs which launch child windows work properly
self.__scriptWindow = GafferUI.ScriptWindow.acquire( script )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't like needing this one, but it seemed the easiest way to ensure color swatches and ramps worked as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Nice! I like!

I've only done fairly limited testing, and in particular haven't tested the error handling modes of the dialogue. Other than the things I've mentioned inline, the only real issue I have is that my current directory is filling up with dispatch directories. This is because there's no job directory set on the dispatcher unless applyUserDefaults is turned on, and its off by default in both gui and non-gui modes. I think I advocated for automatically applying user defaults in gui mode (as for the main gui) and not for non-gui mode (as for python scripts). Did that turn out to be problematic? If so, having seen the side effects of not setting them on the dispatcher, I'm more sympathetic to them being on everywhere by default, which I think is what you preferred anyway...

Thanks...
John

## A dialogue which can be used to dispatch tasks
class DispatchDialogue( GafferUI.Dialogue ) :

def __init__( self, script, tasks, dispatcherType, applyUserDefaults=False, title="Dispatch Tasks", sizeMode=GafferUI.Window.SizeMode.Manual, **kw ) :
Copy link
Member

Choose a reason for hiding this comment

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

Is script redundant here, since everything in tasks should be parented to it already? In other words, can we just derive script from tasks? If we can, we should I think, because otherwise we're just making the API harder to understand, and giving folks a chance to pass conflicting arguments.

The applyUserDefaults argument seems out of place here too, and it's ambiguous as to what we're applying the defaults to. Perhaps rather than a dispatcherType argument, we should pass actual dispatchers, so the calling code is responsible for setting them up however it wants beforehand? I've noted a few potential benefits to this elsewhere in the review...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to drop the script arg.

From the sounds of your intro to the review, you’re leaning towards always applying userDefaults to dispatchers (which would make me very happy) and in which case I can drop this arg and we can separate the discussion of type vs constructed dispatchers from this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061

Example usage :

```
gaffer dispatch -script comp.gfr -nodes ImageWriter -dispatcher Local -settings -dispatcher.executeInBackground 1
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use the executeInBackground flag in the example? It seems like it doesn't have any real effect for a non-gui dispatch, and for a gui dispatch it means the window is closed long before the dispatch is complete. Perhaps we'd be better off hiding that setting even?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll swap that for frameRange or something. I just wanted to show that you can set dispatcher plugs.

I’m hesitant to hide it in the gui though. Is there a use case outside the dispatch app where some code calls launches the dialogue then continues on to do other things while the execution is happening?

Copy link
Member

Choose a reason for hiding this comment

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

I’ll swap that for frameRange or something.

Perfect - that sounds like a much more useful example.

I’m hesitant to hide it in the gui though. Is there a use case outside the dispatch app where some code calls launches the dialogue then continues on to do other things while the execution is happening?

I can see it being useful in a DispatchDialogue embedded in the main gui app, via Jabuka for instance. But for the standalone dispatch app I really can't see a valid use. Not a biggy though...

Copy link
Member

Choose a reason for hiding this comment

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

Actually, maybe it'd be useful in standalone dispatch if we added the option to keep the dialogue open and make more dispatches. Let's not worry about hiding this for now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 38ad464

```
gaffer dispatch -script comp.gfr -nodes ImageWriter -dispatcher Local -settings -dispatcher.executeInBackground 1

gaffer dispatch -gui -nodes GafferDispatch.SystemCommand -dispatcher Local -settings -SystemCommand.command "ls -l"
Copy link
Member

Choose a reason for hiding this comment

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

This gives the following error :

ERROR : gaffer dispatch : setting "SystemCommand.command" : name 'ls' is not defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 38ad464


dispatcherType = args["dispatcher"].value or GafferDispatch.Dispatcher.getDefaultDispatcherType()

if args["gui"].value :
Copy link
Member

Choose a reason for hiding this comment

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

Needing this initial if gui split seems awkward, and it seems like its forced on us by the fact that the DispatchDialogue is making the dispatcher, rather than being given it. If we were more in charge, we'd have a single "make a dispatcher" code path, and all the gui logic could go in one block lower down. I think this implies that my first impressions of the DispatchDialogue constructor may be right...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally wrote it (both this block and the constructor) the way you’re suggesting and then changed it to this when I realized we needed the drop down of dispatchers in the dialogue.

I don’t think it’s reasonable to require the client of DispatchDialogue to construct all the dispatchers upfront. That defeats the purpose of registering dispatchers, and in the majority of use cases we have today, it’s perfectly valid for the end user to choose any dispatcher.

It’s also not very friendly for the client of dispatch app. To make the gui work the command would be something like:

gaffer dispatch -gui -nodes SystemCommand -dispatchers Local Tractor

and we’d use the first dispatcher as the default. But then if you omitted the -gui what would we do? Surely not dispatch with both. Erroring for giving two things to a parameter with a plural name doesn’t seem right either...

One thing I realized I’m missing currently is applying the command line dispatcher settings to all dispatchers... currently the frameRange, for instance, can be out of sync, which probably isn’t desirable. I realize this would manifest itself as yet another dialogue constructor arg, but I think it’s probably the right thing to do.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think it’s reasonable to require the client of DispatchDialogue to construct all the dispatchers upfront.

It's totally reasonable; it's about giving control to the client code. For convenience it could have a default value which means "use all the registered dispatchers".

in the majority of use cases we have today, it’s perfectly valid for the end user to choose any dispatcher.

Do you have a plan for the minority of cases?

It’s also not very friendly for the client of dispatch app. To make the gui work the command would be something like:

gaffer dispatch -gui -nodes SystemCommand -dispatchers Local Tractor

Again, the default would be "show 'em all". The command line argument could remain singular (-dispatcher), and constrain the UI to show only that dispatcher.

I realize this would manifest itself as yet another dialogue constructor arg, but I think it’s probably the right thing to do.

The fact that it requires another dialogue constructor arg is a sign that the dialogue constructor isn't right. But I agree, we should apply the frame range to all the dispatchers. We should just do it outside the dialogue...

Copy link
Contributor Author

@andrewkaufman andrewkaufman May 30, 2018

Choose a reason for hiding this comment

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

It's totally reasonable; it's about giving control to the client code.

@lento was actually in favor of your approach originally, but thinking of a future where the dispatchers are in the graph, and the tool author would drop down the ones that should be available, and expose a switch if they wanted to offer a choice. But that future is a ways off, and kind of implies we'd remove the entire Dispatcher tab, only exposing the plugs that were promoted intentionally.

And then I mentioned that if we added a 3rd dispatcher, he'd have to go updating all his tools to account for it, and he started seeing the value in my approach.

But lets say for the moment I agreed to yours...

For convenience it could have a default value which means "use all the registered dispatchers".

Ok, sounds good.

The command line argument could remain singular (-dispatcher), and constrain the UI to show only that dispatcher.

What about a command line invocation that wants to give users a choice of dispatchers, but also specify a default dispatcher that doesn’t match the registered default dispatcher?

But I agree, we should apply the frame range to all the dispatchers. We should just do it outside the dialogue...

So how would a client of the dialogue use both the default "use all the registered dispatchers" arg and also set the frame range on all of them to match? Seems like we still need the extra constructor arg to me.


And back to my approach...

it's about giving control to the client code.

Control comes with a maintenance burden.

Do you have a plan for the minority of cases?

Trust the end user to be reasonable, rather than tying their hands to one dispatcher or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like my last reply comes off more argumentative than I intended it. I’m willing to add the ‘client provides all the dispatchers’ feature if we can come up with an approach that also enables the ‘client provides minimal defaults and still gets all the other dispatchers’ workflow. That 2nd approach would have to work for both clients of the dialogue and clients of the commandline app to be useful at IE though.

Copy link
Member

Choose a reason for hiding this comment

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

What about a command line invocation that wants to give users a choice of dispatchers, but also specify a default dispatcher that doesn’t match the registered default dispatcher?

I guess that would require extra arguments. Is this something we need right now?

So how would a client of the dialogue use both the default "use all the registered dispatchers" arg and also set the frame range on all of them to match? Seems like we still need the extra constructor arg to me.

Hmm, true. Very good point. But I'm afraid this doesn't push me towards wanting the dialogue to construct the dispatchers internally.

Control comes with a maintenance burden.

Poor APIs come with a maintenance burden. If the dialogue constructs the dispatchers internally, we're going to have to tweak the API every time we find we need more control over that construction. We have perfectly good APIs for querying available dispatchers, creating them and setting their plugs. The permutations are many; I predict a mess if we try to hide them inside the dialogue and add more and more parameters as we find we need more control.

Trust the end user to be reasonable, rather than tying their hands to one dispatcher or the other.

Personally I'm a fan of that approach, but we're always being asked to police problems via technology. And that was just a single example.

I’m willing to add the ‘client provides all the dispatchers’ feature if we can come up with an approach that also enables the ‘client provides minimal defaults and still gets all the other dispatchers’ workflow.

Great! I propose the following :

  • The DispatchDialogue constructor takes a list of already-constructed dispatchers as I've proposed. It doesn't have any friendly fallback to creating them all, since as you point out, that won't be very useful. This gives us flexibility and a simpler core API but with less convenience.
  • We add a utility method somewhere which provides the convenience you're looking for. Perhaps a static DispatchDialogue.createWithDefaultDispatchers() method, or a DispatcherAlgo::createAll() method. I'll leave the details to you.

How does that sound?

Copy link
Contributor Author

@andrewkaufman andrewkaufman May 31, 2018

Choose a reason for hiding this comment

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

I’ll try that approach (probably tomorrow) and see how it feels.

What about a command line invocation that wants to give users a choice of dispatchers, but also specify a default dispatcher that doesn’t match the registered default dispatcher?

I guess that would require extra arguments. Is this something we need right now?

Yes, we need this now. The default is Local, I’d like to keep it that way at IE, but I’d like quite a few of our command line invocations to start the gui with Farm selected and with Local available if the user sees fit to use it.

So what’s the plan for tackling this use case? It shouldn’t affect your design for DispatchDialogue, but will impact the app parameters in some way.

Copy link
Member

Choose a reason for hiding this comment

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

It seems there's going to be a bit of conflict between the gui and non-gui flavours here whatever we do, because multiple dispatchers don't make sense for the non-gui flavour. I like the way you have it already, which is that a singular -dispatcher argument specifies the dispatcher used for the non-gui mode, and also the default dispatcher for gui mode. So maybe we just need an -otherDispatchers argument that is used to specify additional dispatchers to choose from in the gui mode? We could allow wildcards in that parameter so that * could be used to use all available dispatchers, without needing to know their names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061, ea8aa51, 38ad464, 7854975

return 1

try :
plug.setValue( eval( value ) )
Copy link
Member

Choose a reason for hiding this comment

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

This is the source of the name 'ls' is not defined error I noted above. Because we're doing an eval, you need to double quote on the command line like so : -SystemCommand.comand "'ls -l'". This dates back to code I wrote for the execute app I think, because we needed a way of parsing things like Color3f(). It's really awkward to use though, and much more user-facing in the dispatch app than in the execute app.

It would be nice to just pass strings without quoting, and to be able to pass 0 0 0 for Color3fs etc. What do you think? Something we can take on now, or a todo for later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it’s fiddly, and it definitely tripped me up when writing the tests. I took a brief look into t, and seemed to be quickly delving into the guts of IECore.ParameterParser, and bailed out...

Unless you have a solid idea how to tackle it, I’m inclned to add the todo and fixup the help example with double quotes.

Copy link
Member

Choose a reason for hiding this comment

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

Unless you have a solid idea how to tackle it, I’m inclned to add the todo and fixup the help example with double quotes.

That's fine with me. I don't think a better parser is particularly tricky (it'd be a lot like ParameterParser, but for Plugs), but it is a distraction from the main task. I do want to move Applications to use Plugs for command line arguments, so maybe that's the right time to handle the parsing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 38ad464

with GafferUI.ListContainer( GafferUI.ListContainer.Orientation.Horizontal, spacing=2, borderWidth=2 ) :
GafferUI.Label( "Dispatcher" )
GafferUI.Label( "<h4>Current Dispatcher</h4>" )
Copy link
Member

Choose a reason for hiding this comment

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

The "Current" part seems a bit redundant/confusing here. Actually, I wonder if we even need the label, since there's a big old "Dispatcher" label on the tab itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually added Current because just saying Dispatcher seemed redundant to me. I don’t like the visuals of our drop down lists when they aren’t accompanied by a label though. Perhaps just “Current” or “Choose”?

Copy link
Member

Choose a reason for hiding this comment

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

"Current dispatcher" is doubly redundant though. I would go with either just "Dispatcher" or no label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061

self.__dispatchersMenu = GafferUI.MultiSelectionMenu( allowMultipleSelection = False, allowEmptySelection = False )
self.__dispatchersMenu.append( list(GafferDispatch.Dispatcher.registeredDispatchers()) )
self.__dispatchersMenu.setSelection( [ dispatcherType ] )
self.__dispatchersMenuChanged = self.__dispatchersMenu.selectionChangedSignal().connect( Gaffer.WeakMethod( self.__dispatcherChanged ) )

self.__dispatcherFrame = GafferUI.Frame( borderStyle=GafferUI.Frame.BorderStyle.None, borderWidth=0 )
self.__dispatcherFrame = GafferUI.Frame( borderWidth=2 )
Copy link
Member

Choose a reason for hiding this comment

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

To my eyes, this looked better without the border. It avoided the appearance of endless nesting, and matched the layout of the node tabs, making it easier to adjust when switching between them. Perhaps we do need some separation between the NodeUI and the dispatchers menu though? In which case maybe just a horizontal divider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll fiddle a bit more when I’m in, but I think I added it to make the frame around the dispatcher settings lineup with the edge of a PythonCommand’s text field

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 the goal should be to make the dispatcher's NodeUI line up with the other NodeUIs in general, and to have the appearance of the same number of nesting levels on each tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that even possible unless we totally ditch the dropdown?

Copy link
Member

Choose a reason for hiding this comment

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

It should be I think. Containers with zero border shouldn't indent their contents at all, so it should be possible to have a dropdown and still have the NodeUIs line up in terms of left/right/bottom. The top will obviously be pushed down though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061


self.__script = script
# hold a reference to the script window so plugs which launch child windows work properly
self.__scriptWindow = GafferUI.ScriptWindow.acquire( script )
Copy link
Member

Choose a reason for hiding this comment

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

As you say, this is icky. Let's at least add a comment making it clear that this is a workaround, and mentioning swatches/ramps as being the culprits...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061

with GafferUI.ListContainer( spacing=2, borderWidth=2 ) as dispatcherTab :
with GafferUI.ListContainer( GafferUI.ListContainer.Orientation.Horizontal, spacing=2, borderWidth=2 ) :
GafferUI.Label( "Dispatcher" )
self.__dispatchersMenu = GafferUI.MultiSelectionMenu( allowMultipleSelection = False, allowEmptySelection = False )
Copy link
Member

Choose a reason for hiding this comment

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

With my pipeline hat on, I could definitely see myself wanting to constrain the UI to not provide a choice of dispatcher, but only allow dispatch to the farm for instance. If we passed in the dispatchers via the constructor as I suggested above, we could then skip making this menu when there was only a single dispatcher. Actually, this would also declutter the UI for me here, where I only have a local dispatcher anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I admittadly haven’t had any coffee yet, but I’m struggling to think of a scenario in the IE pipeline where we’d want to restrict the available dispatchers on a per-dispatch basis. Even something like a long sim can be justifiable to run locally, and something seemingly fast, like a model publish can be justifiable to run on a farm. From our pipeline perspective, I’d be happy to set the default type and trust users to stick to the sensible choice for each scenario

What I have been wanting for a while is a global way to deregister a dispatcher (so IE can remove GafferTractor.TractorDispatcher from the menu and the task nodes).

If you wanted, I could hide the menu when there’s only one registered dispatcher... you might find it a bit odd for the UI experience to change when you install tractor though.

Copy link
Member

Choose a reason for hiding this comment

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

What I have been wanting for a while is a global way to deregister a dispatcher (so IE can remove GafferTractor.TractorDispatcher from the menu and the task nodes).

Wait, what? You get Tractor in the menu? I had to hack around for ages the other day until I could test the TractorDispatcher. I thought it had been hidden deliberately...

I’m struggling to think of a scenario in the IE pipeline where we’d want to restrict the available dispatchers on a per-dispatch basis.

Do we count out licenses properly for local dispatch?

If you wanted, I could hide the menu when there’s only one registered dispatcher... you might find it a bit odd for the UI experience to change when you install tractor though.

There are definitely arguments both ways, but it's also an odd UI experience to have a huge button that opens a menu with no choices in it. Plus, I still think it's reasonable to want to open the DispatchDialogue with one dispatcher specifically.

Copy link
Contributor Author

@andrewkaufman andrewkaufman May 30, 2018

Choose a reason for hiding this comment

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

Wait, what? You get Tractor in the menu? I had to hack around for ages the other day until I could test the TractorDispatcher. I thought it had been hidden deliberately...

There is a series of events you have to do to get it (its buried in some IEDo method), which R&D almost never does, but artists do all the time. This is how all those taskNode.dispatcher.tractor plugs leaked into production scenes without us noticing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 57a0061, 3e24ca1

#
##########################################################################

## \todo : This entire file was copied directly from startup/gui/project.py
Copy link
Member

Choose a reason for hiding this comment

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

Having the same settings seems like a good idea, at least until we get some experience with the new app in practice. What I did for startup/view/ocio.py was to just symlink it to startup/gui/ocio.py. I'm not sure that's great, but I think it's preferable to a copy that'll inevitably get out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should clarify, it’s not an exact copy. All the code here does exist in the gui file, but that one has other settings we don’t need here (bookmarks, image format) and some that we explicitly want the opposite of (local executeInBackground userDefault).

What I would like to be consistent across all apps is the registered dispatchers and the location of their job directories.

To do that, I’d need the project variables on the script to be consistent (and exist) as well, or to move the default job directories somewhere else inside ~/gaffer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I made this comment after you signed off, or it got buried in the slew of other comments. What are your thoughts now you know its not an exact copy of the gui file?

Copy link
Member

Choose a reason for hiding this comment

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

I'm still inclined to just symlink. It seems like the bookmarks and image format should be harmless and might even be useful (it does look like the bookmarks want a ScriptWindow, but that could be fixed). The "executeInBackground" default doesn't seem project-related at all really, so we could move it somewhere else in the gui startup, maybe "startup/gui/localDispatcher.py"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still todo. Though that symlink suggestion seems quite fragile now I think about it... any new setting accicentally put in "startup/gui/proect.py" would affect the dispatch app, possibly without the committer realizing it.

Also addressed other related and more trivial changes from the code review.
Note that currently the app only supports a single dispatcher, even in gui mode.
I applied the same change to the ScriptEditor as it was also using the "message" class for html.
@andrewkaufman
Copy link
Contributor Author

Ok, I think I've got everything except keeping the gui open on success and deciding if we're going to generalize the project settings across both apps. I'll try to get those done Monday.

This allows the dialogue to either remain open or close after a successful dispatch. The default is to remain open.
@andrewkaufman
Copy link
Contributor Author

andrewkaufman commented Jun 4, 2018

6b3251f adds the ability to keep the dialogue open or close it. I went for a much simpler set of options than the OpDialogue, but I think it covers all of our current needs.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Couple of little things that just came up in my latest testing :

  • The "INFO : RESULT" message in the details panel at the end seems somewhat redundant. Can it ever show anything other than "0"? Perhaps we omit that message and hide the details pane unless there are other messages?
  • A local foreground dispatch which fails (e.g. gaffer dispatch -gui -nodes GafferDispatch.SystemCommand -settings -SystemCommand.command "'blaargh'") reports success in the GUI and outputs the error messages to the shell. I know there's a conceptual difference between the success of the dispatch and the success of the resulting job, but in this case we do know if the job succeeded before returning from LocalDispatcher.dispatch(). I think this is more an issue with the LocalDispatcher, so probably should be dealt with separately, but I wanted to mention it.

that symlink suggestion seems quite fragile now I think about it... any new setting accicentally put in "startup/gui/proect.py" would affect the dispatch app, possibly without the committer realizing it.

The same issue applies even if we move the setup to a utility function that is called by the configs, except then we have even less idea of what is affected by it. I suggest we just add a comment at the top of project.py noting that the dispatch app shares this file.

@staticmethod
def __create( path ) :

path = path.split( "." )
Copy link
Member

Choose a reason for hiding this comment

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

This is still giving nasty errors for some values :

> gaffer dispatch -nodes GafferDispatch

gaffer Traceback (most recent call last):
  File "/disk1/john/dev/build/gaffer/bin/gaffer.py", line 169, in <module>
    result = app.run()
  File "/disk1/john/dev/build/gaffer/python/Gaffer/Application.py", line 110, in run
    return self.__run()
  File "/disk1/john/dev/build/gaffer/python/Gaffer/Application.py", line 142, in __run
    return self._run( self.parameters().getValidatedValue() )
  File "/disk1/john/dev/build/gaffer/apps/dispatch/dispatch-1.py", line 164, in _run
    node = self.__create( nodeName )
  File "/disk1/john/dev/build/gaffer/apps/dispatch/dispatch-1.py", line 294, in __create
    return result()
TypeError: 'module' object is not callable
> gaffer dispatch -nodes GafferDispatch.Oops

Traceback (most recent call last):
  File "/disk1/john/dev/build/gaffer/bin/gaffer.py", line 169, in <module>
    result = app.run()
  File "/disk1/john/dev/build/gaffer/python/Gaffer/Application.py", line 110, in run
    return self.__run()
  File "/disk1/john/dev/build/gaffer/python/Gaffer/Application.py", line 142, in __run
    return self._run( self.parameters().getValidatedValue() )
  File "/disk1/john/dev/build/gaffer/apps/dispatch/dispatch-1.py", line 164, in _run
    node = self.__create( nodeName )
  File "/disk1/john/dev/build/gaffer/apps/dispatch/dispatch-1.py", line 292, in __create
    result = getattr( result, n )
AttributeError: 'module' object has no attribute 'Oops'

@andrewkaufman
Copy link
Contributor Author

"INFO : RESULT" message

Addressed in 7cd76ea

symlink suggestion

Addressed in 192e231, c81d4b0, and 1358468

nasty errors

Addressed in 7f8f858

local foreground dispatch which fails

Not yet addressed, but I agree, this does seem like a problem... How do you recommend fixing it in LocalDispatcher?

@johnhaddon
Copy link
Member

local foreground dispatch which fails
Not yet addressed, but I agree, this does seem like a problem... How do you recommend fixing it in LocalDispatcher?

I think we want to allow the exception thrown by batch.execute() to percolate back up out of LocalDispatcher.dispatch(), instead of suppressing it. I'm unsure of the details of what to do with the __reportFailed() stuff we're doing at the same time as suppressing, because I don't know who's listening (is the Job/JobPool stuff actually relevant to a foreground dispatch?). Either we rethrow the exception I guess, or we just don't bother catching it in the first place...

@andrewkaufman
Copy link
Contributor Author

local foreground dispatch which fails

Addressed in f793784 and 167fcaa. The latter improves the error dialogue that's displayed in the gui app for the same broken SystemCommand.

I'm unsure of the details of what to do with the __reportFailed() stuff we're doing at the same time as suppressing, because I don't know who's listening (is the Job/JobPool stuff actually relevant to a foreground dispatch?).

Those jobs do display in the _LocalJobsWindow, although prior to these commits the jobs displayed in a semi-broken state. Now they display as failed and include the traceback in the debug messages.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me - are we ready to merge from your point of view?

try :
dispatcher.dispatch( [ s["n1"] ] )
except Exception as e :
self.assertTrue( "No such file or directory" in str(e) )
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick : this is what assertRaisesRegexp() is for...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5d9a43d

@andrewkaufman
Copy link
Contributor Author

are we ready to merge from your point of view?

All good from my perspective. I may have to come back to this once @ivanimanishi starts using it in Jabuka, but I'd rather merge as is and address his needs in a future PR.

@johnhaddon johnhaddon merged commit f4015e7 into GafferHQ:master Jun 7, 2018
@johnhaddon
Copy link
Member

Thanks Andrew!

@andrewkaufman andrewkaufman deleted the dispatchApp branch June 7, 2018 23:59
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