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

[FEATURE] Task manager #3004

Merged
merged 48 commits into from
Dec 5, 2016
Merged

[FEATURE] Task manager #3004

merged 48 commits into from
Dec 5, 2016

Conversation

nyalldawson
Copy link
Collaborator

Prototype work in implementing a task manager for QGIS, with easy handling of submitting tasks to be run in the background and exposing their status to users.

Adds new classes:

  • QgsTask. An interface for long-running background tasks. Tasks must implement the run() method to do their processing, and periodically test for isCancelled() to gracefully abort.
  • QgsTaskManager. Handles groups of tasks - also available as a global instance for tracking application wide tasks. Adding a task to a manager will cause the manager to handle launching the run() method in a separate thread.
  • QgsTaskManagerWidget. A list view for showing active tasks and their progress, and for cancelling them

A new dock widget has been added with a task manager widget showing global tasks.

You can test with the following code:

from time import sleep
class TestTask( QgsTask ):

    def __init__(self, desc, time ):
        QgsTask.__init__(self, desc )
        self.time= time

    def run(self):
        wait_time = self.time / 100.0
        for i in range(101):
            sleep(wait_time)
            self.setProgress(i)
            if self.isCancelled():
                self.stopped()
                return 
        self.completed()

for time in range(10, 14):
    task = TestTask('wait {}'.format( time ), time ) 
    QgsTaskManager.instance().addTask( task )

Note that I get crashes with this. My suspicion is that it's due to some oddness in SIP, but it's quite possible there's bugs in the code too!

@wonder-sk Would appreciate you taking a look at this version!

@nyalldawson nyalldawson added the Prototype For comment only, do not merge. Avoid using this tag and open a QEP instead! label Apr 15, 2016
@m-kuhn
Copy link
Member

m-kuhn commented Apr 15, 2016

Sounds interesting. What's the use case for this? Processing?

@alexbruy
Copy link
Contributor

For Processing we already implemented mutithreading support and background tasks. Now waiting for 3.0 branch as it breaks Processing API.

@nyalldawson
Copy link
Collaborator Author

A couple of use cases:

  • make a central place for managing and reporting on the progress of long running tasks. Eg I want to add background high resolution map exports.
  • make it easy for plugins to run tasks in the background without having to have their own thread handling
  • avoid multi implementations of task reporting (eg all background tasks are shown in a single place in the gui, rather than fragmented around different areas)

Ideally processing would use this too. @alexbruy does this implementation look compatible with the processing background tasks?

@nyalldawson
Copy link
Collaborator Author

@wonder-sk There's something odd going on here. Eg, the following code

from time import sleep
from PyQt.QtCore import QObject

class TestTask( QgsTask ):

    def __init__(self, desc, time ):
        QgsTask.__init__(self, desc )
        self.time = time

    def run(self):
        wait_time = self.time / 100.0
        for i in range(101):
            sleep(wait_time)
            self.setProgress(i)
            if self.isCancelled():
                self.stopped()
                return 
        self.completed()

class MyPlugin( QObject ):

    def task_begun(self):
        print 'begun'

    def task_complete(self):
        print 'complete'

    def progress(self,val):
        print val

    def newTask(self):
        task = TestTask('wait {}'.format( 10 ), 10 ) 
        task.begun.connect(self.task_begun)
        task.taskCompleted.connect(self.task_complete)
        task.progressChanged.connect(self.progress)
        QgsTaskManager.instance().addTask( task )
m=MyPlugin()
m.newTask()

The TestTask.run method never actually gets called. I suspect there's something blocking this with PyQt and its handling of threads. https://riverbankcomputing.com/pipermail/pyqt/2011-February/029227.html makes me thing you may have hit this in the past... got any ideas?

@alexbruy
Copy link
Contributor

@nyalldawson we used similar approach. But seems we should drop our work and adopt Processing to use your task manager.

@nyalldawson
Copy link
Collaborator Author

nyalldawson commented Apr 18, 2016

@wonder-sk you're troubleshooting was spot-on. It was related to the task objects getting overwritten in Python.

Here's a screencast of the Task Manager in action:
https://www.youtube.com/watch?v=3KzwNErzkJg&feature=youtu.be

This is using the sample code:

from time import sleep
from PyQt.QtCore import QObject

class TestTask( QgsTask ):

    def __init__(self, desc, time ):
        QgsTask.__init__(self, desc )
        self.time = time

    def run(self):
        wait_time = self.time / 100.0
        for i in range(101):
            sleep(wait_time)
            self.setProgress(i)
            if self.isCancelled():
                 self.stopped()
                 return
        self.completed()

class MyPlugin( QObject ):

    def task_begun(self):
        print '"{}" begun'.format( self.sender().description() )

    def task_complete(self):
        print '"{}" complete'.format( self.sender().description() )

    def task_stopped(self):
        print '"{}" cancelled!'.format( self.sender().description() )

    def progress(self,val):
        #print val
        pass

    def newTask(self, task_name, length):
        task = TestTask(task_name, length ) 
        task.begun.connect(self.task_begun)
        task.taskCompleted.connect(self.task_complete)
        task.progressChanged.connect(self.progress)
        task.taskStopped.connect(self.task_stopped)
        QgsTaskManager.instance().addTask( task )
        return task

m=MyPlugin()
tasks=[]
for i in range(10):
    tasks.append(m.newTask( 'task {}'.format(i), 20))

@m-kuhn
Copy link
Member

m-kuhn commented Apr 18, 2016

That looks amazing.

What do you think about changing the icon to cancel the task? To me it looks like a "restart" icon rather than a "cancel" icon.

@pcav
Copy link
Member

pcav commented Apr 18, 2016

Agreed on both points, @m-kuhn

@nyalldawson
Copy link
Collaborator Author

@m-kuhn yeah the icons are just placeholders for now... I reused existing ones, but I'll make some proper ones for this.

@nirvn
Copy link
Contributor

nirvn commented Apr 18, 2016

Nice. One obvious task that comes to mind is those large composer exports (atlas et cie). Presumably the task would run in the background, have you planned a way to handle subsequent changes to project while the composer exports?

@nyalldawson
Copy link
Collaborator Author

Thanks to @NathanW2 there's a new high-level method for creating tasks from python functions without having to subclass QgsTask:

from time import sleep
import random

# a dumb test function
def run(time):
    wait_time = time / 100.0
    sum = 0
    iterations = 0
    for i in range(101):
        sleep(wait_time)

        #use yield to report progress
        yield i

        sum += random.randint(0,100)
        iterations += 1

        if random.randint(0,100) == 7:
            # use QgsTaskException to report errors
            raise QgsTaskException('bad value!')

    # use QgsTaskResult to return results of calculations or operations
    raise QgsTaskResult([sum,iterations])

task=QgsTask.fromFunction('waste cpu', run, 2 )
QgsTaskManager.instance().addTask(task)


#then can use task.error() or task.result() to see results
print task.error()
print task.result()

@m-kuhn
Copy link
Member

m-kuhn commented Apr 19, 2016

Is there still a todo list for this or is it ready for review? I think it's worth merging it soon to get a long testing period before the release.

@nyalldawson
Copy link
Collaborator Author

nyalldawson commented Apr 19, 2016

My TODO list would be:

  • finish the GUI for controlling tasks (eg adding the ability to remove old tasks, clear all, cancel all)
  • support progress-less tasks, for tasks which cannot report their progress
  • add support for tasks which cannot be cancelled
  • add python unit tests
  • pause/resume support
  • move instance to QgsApplication
  • handle removing layers with dependant tasks
  • task 'groups'
  • proper clean up of finished tasks
  • bonus points: investigate reporting task progress via desktop notifications, eg showing progress in Windows task bar

@m-kuhn
Copy link
Member

m-kuhn commented Apr 19, 2016

Maybe parts can be split out and added in a second batch (e.g. support for tasks which cannot be cancelled or report their progress)?

@nyalldawson
Copy link
Collaborator Author

I'm hoping to get this stuff done today anyway :P if not, i'll defer it to later

@luipir
Copy link
Contributor

luipir commented Apr 19, 2016

any reason to do not implement or plan pause and resume?

@nyalldawson
Copy link
Collaborator Author

@luipir good idea. I'll add it to the to-do list.

@m-kuhn
Copy link
Member

m-kuhn commented Apr 19, 2016

It would also be nice to have dependencies.
This way processing models could efficiently be processed in parallel.

@wonder-sk
Copy link
Member

I very much like the approach from @NathanW2

  • How about using ordinary "return" statement to return something from the task rather than raising an exception?
  • It would be good to be able to throw any kind of Python exception from a task, not just QgsTaskException (and task manager should understand it as a fatal error)
  • Not sure if it is a good practice to use "yield" statement for progress reporting... Like this it is impossible to run it as an ordinary function (the yield statement turns it into a generator)
  • How would we support cancellation here? I would suggest passing another argument to these functions - the argument could be used for both cancellation and progress reporting.

Few more suggestions for C++ API:

  • do we need explicit "can report progress" flag? We could simply consider progress as "unknown" by default (e.g. value -1), and if task will start setting progress value, then we assume it can report progress :)
  • it would be good to merge taskCompleted/taskStopped into one (I found that useful also with MTR) - handling of both cases is quite similar
  • would it make sense to put status handling into task manager rather to keep it in QgsTask? My rationale is that run() implementations should not be able change its state arbitrarily, and it is a responsibility of a task manager to keep track of state of its tasks
  • in run() maybe it would be simpler to just use return value true/false rather emitting a signal?

@wonder-sk
Copy link
Member

Some GUI ideas from PyCharm - I quite like the approach where user is notified about running tasks in status bar (compared to another dock widget) ...

If there is just one task:

one-task

If there are multiple tasks:

more-tasks

If user clicks status bar to see details:

task-details

@NathanW2
Copy link
Member

  • How about using ordinary "return" statement to return something from
    the task rather than raising an exception?

Not supported in Py2 only in Py3

On Wed, Apr 20, 2016 at 2:16 PM, Martin Dobias notifications@github.com
wrote:

Some GUI ideas from PyCharm - I quite like the approach where user is
notified about running tasks in status bar (compared to another dock
widget) ...

If there is just one task:

[image: one-task]
https://cloud.githubusercontent.com/assets/193367/14663233/2296ddca-06f1-11e6-9e79-d63e6d9fa825.png

If there are multiple tasks:

[image: more-tasks]
https://cloud.githubusercontent.com/assets/193367/14663238/372a1cf2-06f1-11e6-814a-9732009a8da0.png

If user clicks status bar to see details:

[image: task-details]
https://cloud.githubusercontent.com/assets/193367/14663245/57472ffc-06f1-11e6-9bcb-b0140e4ce43a.png


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3004 (comment)

@NathanW2
Copy link
Member

+1 to showing tasks in the status bar.

On Wed, Apr 20, 2016 at 2:17 PM, Nathan Woodrow madmanwoo@gmail.com wrote:

  • How about using ordinary "return" statement to return something from
    the task rather than raising an exception?

Not supported in Py2 only in Py3

On Wed, Apr 20, 2016 at 2:16 PM, Martin Dobias notifications@github.com
wrote:

Some GUI ideas from PyCharm - I quite like the approach where user is
notified about running tasks in status bar (compared to another dock
widget) ...

If there is just one task:

[image: one-task]
https://cloud.githubusercontent.com/assets/193367/14663233/2296ddca-06f1-11e6-9e79-d63e6d9fa825.png

If there are multiple tasks:

[image: more-tasks]
https://cloud.githubusercontent.com/assets/193367/14663238/372a1cf2-06f1-11e6-814a-9732009a8da0.png

If user clicks status bar to see details:

[image: task-details]
https://cloud.githubusercontent.com/assets/193367/14663245/57472ffc-06f1-11e6-9bcb-b0140e4ce43a.png


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3004 (comment)

@NathanW2
Copy link
Member

NathanW2 commented Apr 20, 2016

Following on from Martins comments about using yield. Here is an idea:

Using the task object itself to report the status directly to thatl:

def myfunc(task):
    for i in range(100):
         task.setProgress(i)
         if i > 100:
             raise Exception("Bam!")
         if i > 200:
             task.cancel()
    return True

This changes the task manager to be like this in the call:

class QgsTaskWrapper(QgsTask):

    def __init__(self, description, function, *extraArgs):
        QgsTask.__init__(self, description, QgsTask.ProgressReport)
        self.extraArgs = extraArgs
        self.function = function
        self.task_result = None
        self.task_error = None

    def run(self):
        try:
           self.function(self, *self.extraArgs)
        expect Exception as ex:
             # report error
             return
        self.completed()

This design might be a bit clearer on intent and also lets you do this:

class FakeTask:
     def setProgess(self, status):
          # Don't care 
          pass

myfunc(FakeTask)

If you want to unit test or use out side of the task manager.

The other option is to pass in it into the function as kwarg so it's optional for people:

def myfunc(**kwargs):
      # I don't care about reporting status so you can pass it in
      # and I will just ignore it
      # or kwargs['task'] if I do.
class QgsTaskWrapper(QgsTask):

    def __init__(self, description, function, *extraArgs):
        QgsTask.__init__(self, description, QgsTask.ProgressReport)
        self.extraArgs = extraArgs
        .....

    def run(self):
        try:
           extra = dict(task=self)
           self.function(*self.extraArgs, **extra)
        expect Exception as ex:
             # report error
             return
        self.completed()

@nyalldawson
Copy link
Collaborator Author

@NathanW2 that's much nicer! I'll do that approach.

@nirvn
Copy link
Contributor

nirvn commented Apr 21, 2016

@wonder-sk I like the UI shots above. This makes me think the plugin update check-up could be one of the first bit ported to @nyalldawson 's task manager.

Exciting stuff all around.

@nyalldawson
Copy link
Collaborator Author

nyalldawson commented Dec 5, 2016

@wonder-sk Ok, I've implemented everything you suggested, with the exceptions from my original response.

(ie, I consider this complete now ;) )

@wonder-sk
Copy link
Member

wonder-sk commented Dec 5, 2016

@nyalldawson Awesome! Thanks a lot for that!

For sub-tasks, your recent updates make my previous concerns invalid (for progress hijacking I must have misread the code, sorry).

For copy_func(), I would suggest we either remove it or add a note why it is necessary...

+100 from me, please merge! 🎉

@nyalldawson
Copy link
Collaborator Author

Thanks @wonder-sk

For copy_func(), I would suggest we either remove it or add a note why it is necessary...

I'll kill it.. like I said, it was an attempt to solve an unrelated crash which I'd just left in for safety. Easy enough to re-introduce if it's solving a real problem in future.

@nyalldawson nyalldawson closed this Dec 5, 2016
@nyalldawson nyalldawson reopened this Dec 5, 2016
@nyalldawson nyalldawson merged commit b1b6473 into qgis:master Dec 5, 2016
@nyalldawson nyalldawson deleted the task_manager branch December 5, 2016 07:47
@nyalldawson
Copy link
Collaborator Author

Merged! Thanks to everyone for all the detailed feedback and suggestions, and for the QGIS org for the grant to finish this off! :D

@NathanW2
Copy link
Member

NathanW2 commented Dec 5, 2016

nuyttbnh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feedback Waiting on the submitter for answers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet