New TaskComputer #4510

Wiezzel · 2019-07-18T15:26:40Z

Created NewTaskComputer class for computing tasks created with the new Task API. Support for old-style tasks is kept as well. TaskComputerAdapter was introduced to dispatch tasks between new and old
task computer.

Created NewTaskComputer class for computing tasks created with the new Task API. Support for old-style tasks is kept as well. TaskComputerAdapter was introduced to dispatch tasks between new and old task computer.

Wiezzel · 2019-07-23T15:31:35Z

@Krigpl @mfranciszkiewicz Unit tests for the NewTaskComputer are yet to come but you can already start the review.

Krigpl · 2019-07-25T13:05:06Z

golem/task/taskserver.py

@@ -419,7 +419,7 @@ def resource_failure(self, task_id: str, reason: str) -> None:

        self.task_computer.task_interrupted()
        self.send_task_failed(
-            self.task_computer.assigned_subtask['subtask_id'],
+            self.task_computer.assigned_subtask_id,


This is suspicious. It either means that assigned_subtask_id doesn't get cleared after task_interrupted or it is cleared and it's empty here.

Hmm... This is actually a race condition. In most cases assigned_subtask_id won't be cleaned yet. 😛
Good catch!

I still think it's better to get this fixed - like by grabbing the subtask_id before calling task_interrupted and using that value here.

Yea, for sure.

Krigpl · 2019-07-25T13:07:12Z

golem/task/taskcomputer.py

+            task_id = self.assigned_task_id
+            subtask_id = self.assigned_subtask_id
+            computation = self._new_computer.compute()
+            self._handle_computation_results(task_id, subtask_id, computation)


That returns deferred but it's not yielded anywhere - expected? If so, please put a comment.

That's on purpose. The computation deferred resolves once the computation is complete not when it's started. And this method is supposed to just start the computation. I remember adding a comment about it but must've deleted it by accident.

It still needs an error handler, right? twisted complains when there are unyielded deferreds with unhandled errors.

_handle_computation_results() has a catch-all block so it shouldn't raise any error. I can put the _send_results() inside this block as well.

Wait, there is a comment but somehow it got three lines up.

Krigpl · 2019-07-25T13:17:44Z

golem/task/taskcomputer.py

+        return self._assigned_task.env_id
+
+    @defer.inlineCallbacks
+    def change_config(


So what's the decision with change_config? I believe it doesn't necessarily belong here but it's too awkward to remove just now?

There's no final decision yet. @mfranciszkiewicz is researching this topic. This is most probably a temporary solution. And it's compatible with existing code.

Let's keep the method in this PR

maaktweluit

LGTM!

Some small nitty comments, still approving

golem/task/taskcomputer.py

requirements_to-freeze.txt

tests/golem/task/test_taskcomputeradapter.py

codecov · 2019-07-29T08:25:57Z

Codecov Report

Merging #4510 into develop will increase coverage by 0.02%.
The diff coverage is 98.02%.

@@             Coverage Diff             @@
##           develop    #4510      +/-   ##
===========================================
+ Coverage    90.29%   90.31%   +0.02%     
===========================================
  Files          225      225              
  Lines        19715    19932     +217     
===========================================
+ Hits         17802    18002     +200     
- Misses        1913     1930      +17

Krigpl · 2019-07-30T09:55:18Z

tests/golem/task/test_newtaskcomputer.py

+    def subtask_deadline(self):
+        return int(time.time()) + 3600
+
+    def get_task_header(self, **kwargs):


I guess you could just use factories from GM, here and for ctd, and remove these helpers.

Will take a look at these factories.

Well, after a look I don't see much value in using these factories because they generate some random ID's and I want to use particular ones so that I can easily make asserts without passing them around.

You can set the values of the fields you're interested in. Like TaskHeaderFactory(task_id=<your id>), so basically very similar logic to this one here.
Not insisting but I feel it's somewhat redundant to have these helpers.

Krigpl · 2019-07-30T09:58:04Z

tests/golem/task/test_newtaskcomputer.py

+            deadline=kwargs.get('subtask_deadline') or self.subtask_deadline
+        )
+
+    def patch_async(self, name, *args, **kwargs):


What is async supposed to mean here?

It supposed to mean that these patch method is suitable for asynchronous functions unlike the ordinary @patch decorator.

afaict it's only used with non-async functions here so I got confused, that's why I asked.

golem/task/taskcomputer.py

mfranciszkiewicz

One small thing

mfranciszkiewicz · 2019-07-30T14:13:35Z

golem/task/taskcomputer.py

+        sync_wait(self._new_computer.prepare())
+
+        # Should this node behave as provider and compute tasks?
+        self.compute_tasks = task_server.config_desc.accept_tasks \


compute_tasks should become a @property because afair task_server.config_desc.in_shutdown is changed dynamically during runtime

This is a part of the config and config needs to be explicitly changed be calling change_config. That has been the case with the old TaskComputer.

If some code modifies the in_shutdown setting without calling update_config then probably it should be fixed but that's not a part of this PR.

change_config is called on the client after updating this setting, so should be good :)

https://github.com/golemfactory/golem/blob/develop/golem/node.py#L377

tests/golem/task/test_taskcomputeradapter.py

* Refactored _get_task_api_service() and _get_task_dir() to use _assigned_task instead of method parameters. * Added support_direct_computation property to TaskComputerAdapter (needed by dummy task computation). * Fixed DummyTask.computation_failed() which had passing None value as a not-none parameter to DummyTask.computation_finished(). * Added unit test for NewTaskComputer._get_task_api_service() * Moved _runtime assignment after calling _app_client.compute() in NewTaskComputer.compute() (otherwise it would be always None).

Wiezzel added the clay label Jul 18, 2019

Wiezzel self-assigned this Jul 18, 2019

Wiezzel force-pushed the new_taskcomputer branch 9 times, most recently from 8ec549d to 392128f Compare July 23, 2019 14:56

NewTaskComputer

63ce003

Created NewTaskComputer class for computing tasks created with the new Task API. Support for old-style tasks is kept as well. TaskComputerAdapter was introduced to dispatch tasks between new and old task computer.

Wiezzel force-pushed the new_taskcomputer branch from 392128f to 63ce003 Compare July 23, 2019 15:11

Wiezzel changed the title ~~[WIP] New TaskComputer~~ New TaskComputer Jul 23, 2019

Wiezzel marked this pull request as ready for review July 23, 2019 15:30

Wiezzel requested review from mfranciszkiewicz and Krigpl July 23, 2019 15:30

Krigpl reviewed Jul 25, 2019

View reviewed changes

Merge branch 'develop' into new_taskcomputer

7e42786

Wiezzel requested a review from shadeofblue as a code owner July 25, 2019 15:19

Wiezzel force-pushed the new_taskcomputer branch from 28c07cf to 7e42786 Compare July 25, 2019 15:43

Wiezzel added 2 commits July 29, 2019 09:19

Merge branch 'develop' into new_taskcomputer

9d520b0

Introduced deferred_from_future as workaround for bug in Twisted

5d7e23e

maaktweluit approved these changes Jul 29, 2019

View reviewed changes

golem/task/taskcomputer.py Outdated Show resolved Hide resolved

requirements_to-freeze.txt Show resolved Hide resolved

tests/golem/task/test_taskcomputeradapter.py Outdated Show resolved Hide resolved

Unit tests and some fixes for NewTaskComputer

54cec12

Wiezzel force-pushed the new_taskcomputer branch from 166049c to 54cec12 Compare July 29, 2019 13:42

Krigpl reviewed Jul 30, 2019

View reviewed changes

Merge branch 'develop' into new_taskcomputer

0ac8c24

Wiezzel force-pushed the new_taskcomputer branch from 583788b to 0ac8c24 Compare July 30, 2019 10:34

Krigpl approved these changes Jul 30, 2019

View reviewed changes

Krigpl reviewed Jul 30, 2019

View reviewed changes

golem/task/taskcomputer.py Outdated Show resolved Hide resolved

Krigpl reviewed Jul 30, 2019

View reviewed changes

golem/task/taskcomputer.py Outdated Show resolved Hide resolved

Wiezzel force-pushed the new_taskcomputer branch from 35e3c7e to 11e053d Compare July 30, 2019 13:54

mfranciszkiewicz approved these changes Jul 31, 2019

View reviewed changes

Krigpl reviewed Jul 31, 2019

View reviewed changes

tests/golem/task/test_taskcomputeradapter.py Outdated Show resolved Hide resolved

Wiezzel force-pushed the new_taskcomputer branch from 11e053d to cadc186 Compare July 31, 2019 09:11

Wiezzel merged commit 4fd68a6 into develop Jul 31, 2019

etam deleted the new_taskcomputer branch July 31, 2019 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New TaskComputer #4510

New TaskComputer #4510

Wiezzel commented Jul 18, 2019 •

edited

Loading

Wiezzel commented Jul 23, 2019

Krigpl Jul 25, 2019

Wiezzel Jul 25, 2019

Krigpl Jul 29, 2019

Wiezzel Jul 29, 2019

Krigpl Jul 25, 2019

Wiezzel Jul 25, 2019

Krigpl Jul 29, 2019

Wiezzel Jul 29, 2019

Wiezzel Jul 29, 2019 •

edited

Loading

Krigpl Jul 25, 2019

Wiezzel Jul 25, 2019

mfranciszkiewicz Jul 29, 2019

maaktweluit left a comment

codecov bot commented Jul 29, 2019 •

edited

Loading

Krigpl Jul 30, 2019

Wiezzel Jul 30, 2019

Wiezzel Jul 30, 2019

Krigpl Jul 30, 2019

Krigpl Jul 30, 2019

Wiezzel Jul 30, 2019

Krigpl Jul 30, 2019

mfranciszkiewicz left a comment

mfranciszkiewicz Jul 30, 2019

Wiezzel Jul 31, 2019

maaktweluit Jul 31, 2019

New TaskComputer #4510

New TaskComputer #4510

Conversation

Wiezzel commented Jul 18, 2019 • edited Loading

Wiezzel commented Jul 23, 2019

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

Wiezzel Jul 29, 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

maaktweluit left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2019 • edited Loading

Codecov Report

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

mfranciszkiewicz 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

Wiezzel commented Jul 18, 2019 •

edited

Loading

Wiezzel Jul 29, 2019 •

edited

Loading

codecov bot commented Jul 29, 2019 •

edited

Loading