Return value from execute() #474

Closed
bitprophet opened this Issue Nov 9, 2011 · 10 comments

Projects

None yet

2 participants

@bitprophet
Member

While it's a more thorny proposal for things like parallel execution (but likely still do-able), normal serial execution in execute() should be storing the results of each host+task execution run, and returning e.g. a map of host string to result value, instead of None.

This way folks that do rely on tasks returning real values are still able to obtain that data without stuffing it into env.

Submitted on the mailing list by K. Danilov.

@bitprophet bitprophet was assigned Nov 9, 2011
@cloax
cloax commented Jan 6, 2012

Any chance the serial execution case might make its way into 1.4?

@bitprophet
Member

I'll add it for now because a few folks have asked about it and it should be quick to at least implement the serial aspect. But I might bump it back to a later release if the other dev for 1.4 takes too long, be warned :)

@markatto markatto added a commit to markatto/fabric that referenced this issue Jan 18, 2012
@markatto markatto Initial work on execute() return values (#474) 33797f1
@bitprophet
Member

Taking a stab at this now (sorry @markatto, yours is already a bit out of date due to other 1.4 changes :(). Remaining todos:

  • Decide what to do in the case of tasks which encountered errors but didn't abort (currently thinking some non-None sentinel value, though we've not really done this before so I'm uneasy about trying to get it right on the first try)
  • See whether gathering up parallel results will be feasible (should be)
@bitprophet bitprophet added a commit that referenced this issue Jan 24, 2012
@bitprophet bitprophet More tests re #474 74226da
@bitprophet bitprophet added a commit that referenced this issue Jan 28, 2012
@bitprophet bitprophet Changelog/docs re #474 678df55
@bitprophet
Member

This has been merged into master and should be all set, including parallel execution case. @markatto @cloax if you guys want to nab latest master as of now and give the feature a shot, that'd be great!

@bitprophet bitprophet closed this Feb 1, 2012
@cloax
cloax commented Feb 1, 2012

Awesome! Will give it a look. Much thanks!

@bitprophet
Member

In addition to the abovementioned issue, I found another one: large flights can now cause OSError 24, Too many open files, apparently due to the new Queue objects being used. (Commenting out just the use of the Queues, makes the error go away.)

Need to see if it's possible to only create the queue at runtime instead of prep time; this would make JobQueue less Fabric-agnostic but honestly that's a pipe dream and it's already caused some wonkyish code, so at this point I think I will just cross the streams. We'll see.

@bitprophet bitprophet reopened this Feb 3, 2012
@bitprophet
Member

I think I have fixed the OSError but found another semi related issue. In fixing the OSError I noticed that subprocesses could raise Exceptions which then A) print all ugly and such, and B) result in no return value whatsoever from the subprocess.

To match how we handle serially executed tasks (which will return the return value or the exception preventing return) I updated things to capture + store any exceptions. However, I then appear to run into this pickle/multiprocessing bug which creates TypeErrors when pickle somehow tries to re-initialize the exception with the wrong arguments.

Will keep poking to see exactly what it is doing and if there is any way to handle it on our end. To safely handle arbitrary exceptions without passing the exception itself would be difficult at best -- maybe just return the class name or something, which would at least be better than nothing.

@bitprophet
Member

Things should be cleared up now, though I'm not 100% sure about #547 (but it looked strongly related to tonight's changes.) That'll be handled there; this is closing again.

@bitprophet bitprophet closed this Feb 5, 2012
@cloax
cloax commented Feb 22, 2012

Just wanted to say thanks again for knocking this one out. Has been working great for my needs!

@bitprophet
Member

@cloax Glad it's working for you! :) always great to get positive feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment