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

Fix attribute forwarding for tasks with dynamic dependencies #2478

Merged
merged 4 commits into from
Aug 8, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions luigi/worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import signal
import subprocess
import sys
import contextlib

try:
import Queue
Expand Down Expand Up @@ -135,16 +136,8 @@ def __init__(self, task, worker_id, result_queue, status_reporter,
self.check_unfulfilled_deps = check_unfulfilled_deps

def _run_get_new_deps(self):
# forward some attributes before running
for reporter_attr, task_attr in six.iteritems(self.forward_reporter_attributes):
setattr(self.task, task_attr, getattr(self.status_reporter, reporter_attr))

task_gen = self.task.run()

# reset attributes again
for reporter_attr, task_attr in six.iteritems(self.forward_reporter_attributes):
setattr(self.task, task_attr, None)

if not isinstance(task_gen, types.GeneratorType):
return None

Expand Down Expand Up @@ -202,7 +195,8 @@ def run(self):
expl = 'Task is an external data dependency ' \
'and data does not exist (yet?).'
else:
new_deps = self._run_get_new_deps()
with self._forward_attributes():
new_deps = self._run_get_new_deps()
status = DONE if not new_deps else PENDING

if new_deps:
Expand Down Expand Up @@ -258,6 +252,20 @@ def terminate(self):
except ImportError:
return super(TaskProcess, self).terminate()

@contextlib.contextmanager
def _forward_attributes(self):
# forward configured attributes to the task
for reporter_attr, task_attr in six.iteritems(self.forward_reporter_attributes):
setattr(self.task, task_attr, getattr(self.status_reporter, reporter_attr))

try:
yield self

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the white-space here and the love above I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔︎

finally:
# reset attributes again
for reporter_attr, task_attr in six.iteritems(self.forward_reporter_attributes):
setattr(self.task, task_attr, None)


# This code and the task_process_context config key currently feels a bit ad-hoc.
# Discussion on generalizing it into a plugin system: https://github.com/spotify/luigi/issues/1897
Expand Down
76 changes: 76 additions & 0 deletions test/task_forwarded_attributes_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# -*- coding: utf-8 -*-
#
# Copyright 2012-2015 Spotify AB
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

from helpers import LuigiTestCase, RunOnceTask

import luigi
import luigi.scheduler
import luigi.worker


forwarded_attributes = set(luigi.worker.TaskProcess.forward_reporter_attributes.values())
Copy link
Contributor

Choose a reason for hiding this comment

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

make this constant in capital letters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔︎



class NonYieldingTask(RunOnceTask):

# need to accept messages in order for the "scheduler_message" attribute to be not None
accepts_messages = True

def gather_forwarded_attributes(self):
attrs = set()
for attr in forwarded_attributes:
if getattr(self, attr, None) is not None:
attrs.add(attr)
return attrs

def run(self):
self.attributes_while_running = self.gather_forwarded_attributes()

RunOnceTask.run(self)


class YieldingTask(NonYieldingTask):

def run(self):
self.attributes_before_yield = self.gather_forwarded_attributes()

yield RunOnceTask()

self.attributes_after_yield = self.gather_forwarded_attributes()

RunOnceTask.run(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite unintuitive, maybe add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔︎



class TaskForwardedAttributesTest(LuigiTestCase):

def run_task(self, task):
sch = luigi.scheduler.Scheduler()
with luigi.worker.Worker(scheduler=sch) as w:
w.add(task)
w.run()
return task

def test_non_yielding_task(self):
task = self.run_task(NonYieldingTask())

self.assertEqual(task.attributes_while_running, forwarded_attributes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am i correct that this test passes before AND after the code changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct.


def test_yielding_task(self):
task = self.run_task(YieldingTask())

self.assertEqual(task.attributes_before_yield, forwarded_attributes)
self.assertEqual(task.attributes_after_yield, forwarded_attributes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

But this one would fail before this code change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.