Add ThreadPoolExecutor to workflowrunner #806

xjules · 2019-11-20T16:25:29Z

Issue
Resolves #802

Approach
Add ThreadPoolExecutor as an attribute to workflowrunner.
Exception is provided workflowrunner.exception()

markusdregi · 2019-11-27T10:30:44Z

python/res/job_queue/workflow_runner.py

+
+    def run(self):
+        try:
+            if self._target:


Should this not be done by super instead? I.e. super(WorkflowRunnerThread, self).run(self)?

Let's discuss this - need to know an opinion on this one

markusdregi · 2019-11-27T10:30:46Z

python/res/job_queue/workflow_runner.py

+        super(WorkflowRunnerThread, self).__init__(target=target, name=name)
+        self.setDaemon(True)
+        self._exception = None
+        # in python2.7 it's called __target therefore we need to inject this too


I don't understand this comment.

The difference of Python2.7 vs Python3.3+ is thread.__target vs thread._target; therefore we set an attribute thread._target (which is tackled by init for Python3.3+)

markusdregi · 2019-11-27T13:27:01Z

I still don't see how this PR is resolving the given issue. It solves the problem in the tests, but that is more by changing the test, then adopting the code at large (which I find a bit scary).

I'll have try to have a closer look at the usage of the WorkflowRunner and come back to you with something more concrete ;)

oysteoh · 2019-11-28T08:08:52Z

python/res/job_queue/workflow_runner.py

-        self.__workflow.wait()
+        if self.runner_thread:
+            self.runner_thread.join()
+        print('DBG wait finnished with following exception', self.runner_thread._exception)


what happens if not self.runner_thread ? from what i understand your print will fail when calling _exception on it?

Good point, should be inside the if statement

ManInFez

This looks good to me. It fixes the potential leaking of threads during testing. We should look into how Ert is using this and protect the execution in a similar manner.

markusdregi

I would like to have another discussion regarding this PR before we merge it when Julius is back in the office :)

markusdregi

Beautiful 👏 Can you evaluate the usage in ERT and make an issue for changing the usage of WorkflowRunner in ERT if that should be done?

markusdregi · 2019-12-17T13:32:07Z

python/res/job_queue/workflow_runner.py

-        workflow_thread.setDaemon(True)
-        workflow_thread.run = self.__runWorkflow
-        workflow_thread.start()
+        #should we check if it's already running?


If that is intended usage then please go ahead ;)

You should remove the comment and make a decision as to whether you want to check this or not ;) I would lean towards the later

Comment removed!

markusdregi · 2019-12-17T13:32:49Z

python/res/job_queue/workflow_runner.py

@@ -20,12 +20,20 @@ def __init__(self, workflow, ert=None, context=None):

        self.__context = context
        self.__workflow_result = None
+        self.__workflow_executor = ThreadPoolExecutor(max_workers=1)


Double under scored prefix is reserved for Python. Please use single underscore instead 🙏

Ok, shall I change the existing ones too?

A new issue spawned for existing ones #861

markusdregi · 2019-12-17T13:35:07Z

python/res/job_queue/workflow_runner.py

 from res.job_queue import Workflow
 from res.util.substitution_list import SubstitutionList
-
+from concurrent.futures import ThreadPoolExecutor
+from concurrent import futures


Do we need both of these imports?

No, will be gone ... soon :)

markusdregi · 2019-12-17T13:35:57Z

Do we need to update the requirements file and the Komodo dependencies of libres?

ertomatic · 2019-12-19T13:42:37Z

Can one of the admins verify this patch?

xjules · 2020-01-06T14:10:50Z

Do we need to update the requirements file and the Komodo dependencies of libres?

No, we shouldn't.

markusdregi · 2020-01-06T14:50:14Z

Do we need to update the requirements file and the Komodo dependencies of libres?

No, we shouldn't.

Are you sure? I don't think futures comes with Python 2.7...

xjules · 2020-01-06T15:09:59Z

Do we need to update the requirements file and the Komodo dependencies of libres?

No, we shouldn't.

Are you sure? I don't think futures comes with Python 2.7...

~~That agrees, but it's included in libecl ... but can make it explicit in libres too.~~ That relates to future no futures
'futures` included in libres too now. The issue agronholm/pythonfutures#29 only relates to spawning new processes, where in our case we spawn only new threads within the same process, so we are good to go!

markusdregi · 2020-01-10T11:08:31Z

python/res/job_queue/workflow_runner.py

-        workflow_thread.setDaemon(True)
-        workflow_thread.run = self.__runWorkflow
-        workflow_thread.start()
+        self._workflow_job = self._workflow_executor.submit(self.__runWorkflow)


Are we not leaking a thread now if we call run twice and then cancel arbitrary many times? If so, I would say that we should raise an Exception if run is called twice and it is clear misuse of the API.

The thread should not leak as it is the part of ThreadPool, i.e., just the job submitted twice - of-course it shouldn't be done for different reasons, so yes we can raise an exception here.

markusdregi · 2020-01-10T13:35:58Z

python/res/job_queue/workflow_runner.py

-        workflow_thread.run = self.__runWorkflow
-        workflow_thread.start()
+        if self.isRunning():
+            raise Exception('An instance of workflow is already running!')


Never raw exceptions, perhaps an AssertionError?

ok. Was browsing around in libres and found raw Exception still ....

- ThreadPoolExecutor replacing Thread in workflow runner - mock worflow.run to throw exception - create ThreadPoolExecutor in __init__ - replacing underscores and clean up - add futures to requirements - raise AssertionError if workflow is running when starting

markusdregi

Good job 👍

Can you create a new tag on master listing that a new dependency is added after this is merged and add this to the dependency list for master and the new tag in komodo-releases?

xjules added the in progress label Nov 20, 2019

xjules self-assigned this Nov 20, 2019

xjules changed the title ~~Add workflow thread and join correspondingly~~ WIP: Add workflow thread and join correspondingly Nov 20, 2019

xjules force-pushed the workflow_runner_thread branch from 967748f to 42a4a57 Compare November 25, 2019 13:49

xjules changed the title ~~WIP: Add workflow thread and join correspondingly~~ Add workflow thread and join correspondingly Nov 26, 2019

xjules added ready for review and removed in progress labels Nov 26, 2019

markusdregi reviewed Nov 27, 2019

View reviewed changes

oysteoh reviewed Nov 28, 2019

View reviewed changes

xjules force-pushed the workflow_runner_thread branch from d26b3a5 to 765a198 Compare December 2, 2019 11:49

ManInFez approved these changes Dec 2, 2019

View reviewed changes

markusdregi suggested changes Dec 6, 2019

View reviewed changes

xjules force-pushed the workflow_runner_thread branch 2 times, most recently from cb54f89 to cc664d8 Compare December 17, 2019 12:02

xjules changed the title ~~Add workflow thread and join correspondingly~~ Add ThreadPoolExecutor to workflowrunner Dec 17, 2019

markusdregi reviewed Dec 17, 2019

View reviewed changes

xjules force-pushed the workflow_runner_thread branch from cc664d8 to 4e41c22 Compare January 6, 2020 14:32

xjules mentioned this pull request Jan 6, 2020

Update workflow_runner to new libres API using context manager equinor/ert#629

Closed

xjules force-pushed the workflow_runner_thread branch from 4e41c22 to 67737f7 Compare January 7, 2020 08:23

xjules requested a review from markusdregi January 8, 2020 11:15

xjules force-pushed the workflow_runner_thread branch 2 times, most recently from f5aed36 to e0416e3 Compare January 10, 2020 08:28

markusdregi reviewed Jan 10, 2020

View reviewed changes

xjules force-pushed the workflow_runner_thread branch from e0416e3 to d33ad72 Compare January 10, 2020 12:45

markusdregi reviewed Jan 10, 2020

View reviewed changes

xjules requested a review from markusdregi January 10, 2020 13:53

xjules force-pushed the workflow_runner_thread branch from d33ad72 to ac8b37b Compare January 10, 2020 13:57

markusdregi approved these changes Jan 13, 2020

View reviewed changes

xjules merged commit 33aa63b into equinor:master Jan 13, 2020

xjules deleted the workflow_runner_thread branch January 13, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ThreadPoolExecutor to workflowrunner #806

Add ThreadPoolExecutor to workflowrunner #806

xjules commented Nov 20, 2019 •

edited

Loading

markusdregi Nov 27, 2019

xjules Nov 28, 2019

markusdregi Nov 27, 2019

xjules Nov 28, 2019

markusdregi commented Nov 27, 2019 •

edited

Loading

oysteoh Nov 28, 2019

xjules Nov 28, 2019

ManInFez left a comment

markusdregi left a comment

markusdregi left a comment

markusdregi Dec 17, 2019

xjules Jan 6, 2020

markusdregi Jan 8, 2020

xjules Jan 10, 2020

markusdregi Dec 17, 2019

xjules Jan 6, 2020

xjules Jan 6, 2020

markusdregi Dec 17, 2019

xjules Jan 6, 2020

markusdregi commented Dec 17, 2019

ertomatic commented Dec 19, 2019

xjules commented Jan 6, 2020

markusdregi commented Jan 6, 2020

xjules commented Jan 6, 2020 •

edited

Loading

markusdregi Jan 10, 2020

xjules Jan 10, 2020

markusdregi Jan 10, 2020

xjules Jan 10, 2020 •

edited

Loading

markusdregi left a comment

Add ThreadPoolExecutor to workflowrunner #806

Add ThreadPoolExecutor to workflowrunner #806

Conversation

xjules commented Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusdregi commented Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManInFez left a comment

Choose a reason for hiding this comment

markusdregi left a comment

Choose a reason for hiding this comment

markusdregi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusdregi commented Dec 17, 2019

ertomatic commented Dec 19, 2019

xjules commented Jan 6, 2020

markusdregi commented Jan 6, 2020

xjules commented Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjules Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

markusdregi left a comment

Choose a reason for hiding this comment

xjules commented Nov 20, 2019 •

edited

Loading

markusdregi commented Nov 27, 2019 •

edited

Loading

xjules commented Jan 6, 2020 •

edited

Loading

xjules Jan 10, 2020 •

edited

Loading