Makes ewoksorange.gui.owwidgets.thread returning TaskFuture#406
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
e1b9c68 to
b3938f0
Compare
| @@ -0,0 +1,9 @@ | |||
| class CompletedWorker: | |||
There was a problem hiding this comment.
This worker has been added for the specific use case of the OWEwoksWidgetNoThread and in order to propose an homogeneous API (_execute_ewoks_task return a TaskFuture whenever possible)
| exception = e | ||
| _logger.error(f"task failed: {e}", exc_info=True) | ||
| else: | ||
| exception = None | ||
|
|
There was a problem hiding this comment.
The exception is needed to fill the '_CompletedWorker' downstream.
| raw_future = _ConcurrentFuture() | ||
| if exception is not None: | ||
| raw_future.set_exception(exception) | ||
| else: | ||
| raw_future.set_result(self.__task_executor.output_variables) | ||
| return TaskFuture(raw_future, _CompletedWorker()) |
There was a problem hiding this comment.
Build a dummy TaskFuture to make sure we have a coherent API.
| class OWEwoksWidgetOneThread(_OWEwoksThreadedBaseWidget, **ow_build_opts): | ||
| """ | ||
| Single persistent background thread for task execution. | ||
| class _OWEwoksExecutorWidget(_OWEwoksThreadedBaseWidget, **ow_build_opts): |
There was a problem hiding this comment.
One of the main advantage of using the executor into a dedicated module is that we can have a widget that can be generalized and from which only the "executor" is changed.
| # Note: all the following variable to be removed since executor refactoring | ||
| self.__current_task_future: Optional[TaskFuture] = None | ||
| self.__last_output_variables: dict = {} | ||
| self.__last_task_succeeded: Optional[bool] = None | ||
| self.__last_task_done: Optional[bool] = None | ||
| self.__last_task_exception: Optional[Exception] = None |
There was a problem hiding this comment.
With the new design I think at one point we could get rid of those. It would simplify things and is more aligned with "expectation".
I started to reduce usage but this is not that straightforward so I just added a warning for future.
| def task_succeeded(self) -> Optional[bool]: | ||
| warnings.warn( | ||
| f"'task_succeeded' is deprecated since 6.0 Please use directly the future provided during task submission.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return self.__last_task_succeeded | ||
|
|
||
| @property | ||
| def task_done(self) -> Optional[bool]: | ||
| warnings.warn( | ||
| f"'task_done' is deprecated since 6.0 Please use directly the future provided during task submission.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return self.__last_task_done | ||
|
|
||
| @property | ||
| def task_exception(self) -> Optional[Exception]: | ||
| warnings.warn( | ||
| f"'task_exception' is deprecated since 6.0 Please use directly the future provided during task submission.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return self.__last_task_exception | ||
|
|
||
| def _get_task_outputs(self) -> dict: | ||
| """Return the last finished task's outputs.""" | ||
| warnings.warn( | ||
| f"'cancel_running_task' is deprecated since 6.0 Please use directly the future provided during task submission.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Those are using internally one of the following variables that we would like to remove in the future:
__current_task_future__last_output_variables__last_task_succeeded__last_task_done__last_task_exception
The best would be to able to deprecate them all and use the ̀TaskFuture ̀ instead but this is not that direct so I only reduce usage of it when this was direct.
To be considered in the future.
| super().__init__(*args, **kwargs) | ||
| self.__task_executor_queue = TaskExecutorQueue( | ||
| ewokstaskclass=self.ewokstaskclass | ||
| def cancel_running_task(self) -> None: |
There was a problem hiding this comment.
We should now use future. Internally we are only using it for test that will be removed once TaskExecutorQueue class is removed.
c7cbe0b to
a78a7ff
Compare
There was a problem hiding this comment.
replaced by test_executor (from previous PR)
8b58459 to
b2edd60
Compare
There was a problem hiding this comment.
Removed as all the classes it was testing are now deprecated. It also avoid having some modifications of the .github/workflows/test.yml to ignore deprecation warning. A bit violent but way simpler.
There was a problem hiding this comment.
Added test for OWEwoksWidgetNoThread that were not existing. Simple test to make sure the new API using future is working for this widget.
| # wait for the future to be started. Should be done through a QtEvent | ||
| sleep(0.2) |
There was a problem hiding this comment.
I was considering using an Event but this is probably overkilled.
| [project] | ||
| name = "ewoksorange" | ||
| version = "5.0.1" | ||
| version = "6.0.0" |
There was a problem hiding this comment.
I tried to minimize API break but this is huge modification. So for sure it will break existing behavior under the hood. More honest to consider this will part of a major release.
TaskFuture
57e77b8 to
e9d5528
Compare
f70b3e2 to
18977fd
Compare
# Conflicts: # src/ewoksorange/gui/concurrency/executor/_EwoksProcessWorker.py # Conflicts: # src/ewoksorange/gui/concurrency/executor/_EwoksProcessWorker.py # Conflicts: # src/ewoksorange/gui/concurrency/executor/_EwoksProcessHandle.py
…ead submission. # Conflicts: # src/ewoksorange/gui/concurrency/executor/EwoksExecutor.py
…sed when aborted # Conflicts: # src/ewoksorange/gui/owwidgets/threaded.py
… try/finally signal.
…inside the try / finally. This is not the purpose of this PR
…WEwoksWidgetWithTaskStack' under the generic '_OWEwoksExecutorWidget'
…ne', 'task_exception', '_get_task_outputs' and 'cancel_running_task'
…_done', 'task_exception', '_get_task_outputs'
18977fd to
00e253a
Compare
This PR benefits from the
ewoksorange.gui.concurrency.executormodule.#405 to be merged first.
ewoksorange.gui.concurrency.threadednow use the executors fromewoksorange.gui.concurrency.executorexecute_ewoks_taskAPI now return a ̀TaskFuture ̀ that we can cancel or abort.Note to reviewers: as the
ewoksorange.gui.owwidgets.threadedmodule contains a lot of modification this is probably simpler to look at the existing code today than on the diff.Note on AI: spend some time to force Claude do what I wanted 😇 .