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

[ENH] Offload work to a separate thread #3627

Merged
merged 3 commits into from
Mar 15, 2019

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Feb 28, 2019

Issue

Fixes #3579

Description of changes
  • ConcurrentMixin - a new class - base class for concurrent mixins
  • TaskState - a task class to be used within ConcurrentMixin
  • ConcurrentWidgetMixin - a mixin to be used along with OWWidget
  • OWConcurrentWidget - an example of using ConcurrentWidgetMixin
  • OWFreeviz and OWMDS - extend OWConcurrentWidget and add runner methods to calculate projectiona in threads
Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT force-pushed the concurrent_widget_mixin branch 2 times, most recently from 82c31bf to da32aa2 Compare March 1, 2019 09:29
@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #3627 into master will increase coverage by 0.05%.
The diff coverage is 95.8%.

@@            Coverage Diff             @@
##           master    #3627      +/-   ##
==========================================
+ Coverage   84.23%   84.28%   +0.05%     
==========================================
  Files         370      372       +2     
  Lines       67469    67726     +257     
==========================================
+ Hits        56832    57086     +254     
- Misses      10637    10640       +3

@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #3627 into master will increase coverage by 0.08%.
The diff coverage is 95.99%.

@@            Coverage Diff             @@
##           master    #3627      +/-   ##
==========================================
+ Coverage   84.34%   84.43%   +0.08%     
==========================================
  Files         370      372       +2     
  Lines       67948    68220     +272     
==========================================
+ Hits        57313    57599     +286     
+ Misses      10635    10621      -14

@VesnaT VesnaT force-pushed the concurrent_widget_mixin branch 6 times, most recently from 6fa6114 to 12d4415 Compare March 1, 2019 14:34
@pavlin-policar
Copy link
Collaborator

One major limitation of the current approach is that we can define only one single task by implementing _prepare_task. We can do whatever we want here, but the name kind of implies that the widget can only run one single task. The behaviour of everything is also very unclear since we implement _prepare_task that returns something that start will somehow manage to call, but it's not really clear how and when just by looking at the function names.

One instance where this might be ugly would be when doing imputation. There are like 6 different ways to impute values, each of which presumably calls a different function. So in that case, _prepare_task would end up being 6 if statements that would call appropriate functions.

To me, an ideal API would let me pass whatever function I want and run that on a thread, without having to override anything. Very similar to executor.submit. An example of this might be

def very_complicated_task():
    return 42

# Somewhere inside the widget
self.start(very_complicated_task)

This wouldn't do anything with the progress bar or partial results or support cancelling, but we'd still get the result in _set_results. It seems to me that this simplest case should work. It's also completely clear what's going on here. You gave some function to start, which will run the task. Very straightforward.

This, then, introduces the problem: "How do we get the state parameter to the task when we have a proper runner with progress updates?". One way we could do this might be to do something similar to dependency injection i.e. add the state parameter if the method has a parameter of type TaskState.

# ConcurrentWidgetMixin

    def start(self, task: Callable):
        """ Call to start the task. """
        self.__cancel_task(wait=False)

        if self.data is None:
            self.__set_state_ready()
            return

        state = TaskState(self)

        assert callable(task), "`task` must be callable!"
        signature = inspect.signature(task)
        for param in signature.parameters:
            if issubclass(signature.parameters[param].annotation, TaskState):
                task = partial(task, **{param: state})

        self.__set_state_busy()
        self.__start_task(task, state)

then we could define our runner method accept a state e.g.

def very_complicated_task(state: TaskState):
    state.set_progress_value(50)
    return 42

# Somewhere inside the widget
self.start(very_complicated_task)

And boom. We can pass in simple functions or more complicated runners like in t-SNE, the API is very simple and straightforward. But it's magic. I am still unsure of how bad of an idea this is. Either way, I think we should try to remove _prepare_task in some way or another.

Copy link
Collaborator

@pavlin-policar pavlin-policar left a comment

Choose a reason for hiding this comment

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

Having looked at this some more, I think we need to change our API a little bit. I'm opposed to having on_done expose futures. The public API should be as simple as possible: start, cancel and shutdown are quite clear. Handlers that we need to define on the widgets should be _on_partial_result and _on_done.

I realise now that there should be something to handle exceptions inside the thread _on_exception. It should be simple to change the implementation of the current on_done to check for an exception.

Orange/widgets/tests/base.py Outdated Show resolved Hide resolved
Orange/widgets/tests/base.py Outdated Show resolved Hide resolved
Orange/widgets/utils/concurrent.py Outdated Show resolved Hide resolved
Orange/widgets/utils/tests/concurrent_example.py Outdated Show resolved Hide resolved
Orange/widgets/utils/concurrent.py Outdated Show resolved Hide resolved
Orange/widgets/utils/concurrent.py Outdated Show resolved Hide resolved
Orange/widgets/utils/concurrent.py Outdated Show resolved Hide resolved
Orange/widgets/visualize/owfreeviz.py Outdated Show resolved Hide resolved
Orange/widgets/visualize/owfreeviz.py Show resolved Hide resolved
@VesnaT VesnaT force-pushed the concurrent_widget_mixin branch 4 times, most recently from 3217a99 to ee8a726 Compare March 4, 2019 10:39
@VesnaT VesnaT changed the title [WIP] FreeViz: Offload work to a separate thread [ENH] FreeViz: Offload work to a separate thread Mar 4, 2019
@VesnaT VesnaT force-pushed the concurrent_widget_mixin branch 3 times, most recently from d27978a to f8ebebe Compare March 5, 2019 07:21
@VesnaT VesnaT changed the title [ENH] FreeViz: Offload work to a separate thread [ENH] Offload work to a separate thread Mar 5, 2019
@VesnaT VesnaT changed the title [ENH] Offload work to a separate thread [WIP] Offload work to a separate thread Mar 8, 2019
@VesnaT VesnaT changed the title [WIP] Offload work to a separate thread [ENH] Offload work to a separate thread Mar 8, 2019
Orange/widgets/unsupervised/owmds.py Outdated Show resolved Hide resolved
Orange/widgets/unsupervised/owmds.py Outdated Show resolved Hide resolved
Orange/widgets/utils/tests/concurrent_example.py Outdated Show resolved Hide resolved
Orange/widgets/visualize/owfreeviz.py Outdated Show resolved Hide resolved
Orange/widgets/visualize/owfreeviz.py Outdated Show resolved Hide resolved
Orange/widgets/utils/concurrent.py Show resolved Hide resolved
Orange/widgets/utils/tests/concurrent_example.py Outdated Show resolved Hide resolved
@janezd janezd merged commit b6a88e3 into biolab:master Mar 15, 2019
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.

4 participants