externalize: Dont register the copied tasks #1975

Tarrasch · 2016-12-28T10:23:38Z

This fixes an implementation detail, so that luigi.task.externalize
actually does what it says it does in the documentation. Hence I
hope pretty uncontroversial. This makes it so that calling externalize
don't register a new task. A pretty serious bug.

Now, there's still a caveat, as we're mostly hiding that we do side
affects, if anyone would actually inherit from what is returned by
externalize. That subtask wouldn't be registered either.

Have you tested this? If so, how?

This code is quite easy to get wrong somewhere. Luckily luigi is
decently tested. To make me even more confident I also added a couple of
tests. Including two tests that didn't pass before.

Further. I will try this in production too. Ensuring everything is actually working this time. :)

mention-bot · 2016-12-28T10:23:40Z

@Tarrasch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @erikbern, @steenzout and @daveFNbuck to be potential reviewers.

daveFNbuck · 2016-12-28T16:19:33Z

test/task_test.py

@@ -206,6 +206,24 @@ def run(self):
 self.assertIsNotNone(MyTask.run) # Check immutability
 self.assertIsNotNone(MyTask().run) # Check immutability

+ def test_dont_affects_registry(self):


test_externalize_doesnt_affect_the_registry would be more clear

daveFNbuck · 2016-12-28T16:20:31Z

test/task_test.py

+ # This first check is just an assumption rather than assertion
+ self.assertTrue(self.run_locally(['MyTask']))
+ luigi.task.externalize(MyTask)
+ self.assertTrue(self.run_locally(['MyTask']))


So the idea here is that MyTask is still not external?

Ah... Yea this code is hard to read. I'm adding a comment.

It tests that self.run_locally(['MyTask']) doesn't encounter any "ambiguous task" issues after running the externalize code.

daveFNbuck · 2016-12-28T16:21:00Z

test/task_test.py

@@ -206,6 +206,24 @@ def run(self):
 self.assertIsNotNone(MyTask.run) # Check immutability
 self.assertIsNotNone(MyTask().run) # Check immutability

+ def test_dont_affects_registry(self):
+ class MyTask(luigi.Task):


why not just externalize luigi.Task in these tests?

I think of two reasons:

Maybe in the future we don't expose the luigi.Task,

While true, I've made externalize have no side effects, but a bug in it could actually affect luigi.Task, and then subsequent tests could start to fail if they are run after this test.

daveFNbuck · 2016-12-28T16:26:24Z

luigi/task_register.py

+ name = cls.get_task_family()
+
+ if name in reg and reg[name] != cls and \
+ reg[name] != cls.AMBIGUOUS_CLASS and \


While we're looking at this code, it seems that if we register the same name three times with different classes, the second registration will turn it into cls.AMBIGUOUS_CLASS and the third registration will hit the else clause here and register itself. That doesn't seem right. Maybe this line should go?

Great catch. I added a test case. for this. You were right, the current behavior is broken (see soon up and coming update to PR) :)

daveFNbuck · 2016-12-28T16:29:17Z

luigi/task_register.py

+
+ if name in reg and reg[name] != cls and \
+ reg[name] != cls.AMBIGUOUS_CLASS and \
+ not issubclass(cls, reg[name]):


Doesn't this cover the reg[name] != cls check?

Yes, I think so. I removed the reg[name] != cls check. Nice catch!

daveFNbuck · 2016-12-28T16:31:10Z

luigi/task_register.py

- reg[name] = cls.AMBIGUOUS_CLASS
- else:
- reg[name] = cls
+ if cls._visible_in_registry:


Why didn't you just add this to the existing conditional? It would simplify the diff a bit and avoid extra nesting.

Actually not equivalent. (if it's not visible in registry, we don't want the else clause).

However, I agree we should avoid extra nesting. Made this into a if not blah: continue clause.

daveFNbuck · 2016-12-28T16:32:05Z

test/util_test.py

+
+ return ParentTask
+
+ def test_requries_has_effect_run_child(self):


requires misspelled

daveFNbuck · 2016-12-28T16:32:12Z

test/util_test.py

+ self.assertTrue(self.run_locally_split('ChildTask --my-parameter actuallyset'))
+ self.assertEqual(ParentTask.class_variable, 'actuallyset')
+
+ def test_requries_has_effect_run_parent(self):


requires again

daveFNbuck · 2016-12-28T16:33:33Z

test/util_test.py

@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-from helpers import LuigiTestCase


It seems like you accidentally pulled some unrelated changes to util_test into this.

Actually not completely unrelated. Much of the machinery here is relying on the Registry that got changed here.

I'll amend the commit message saying I intentionally added these tests for this change set.

Tarrasch

Big thanks for review. I've addressed all your comments. :)

I'll update the code in a few hours I think.

Tarrasch · 2017-01-03T03:41:17Z

test/task_test.py

@@ -206,6 +206,24 @@ def run(self):
 self.assertIsNotNone(MyTask.run) # Check immutability
 self.assertIsNotNone(MyTask().run) # Check immutability

+ def test_dont_affects_registry(self):
+ class MyTask(luigi.Task):


I think of two reasons:

Maybe in the future we don't expose the luigi.Task,

While true, I've made externalize have no side effects, but a bug in it could actually affect luigi.Task, and then subsequent tests could start to fail if they are run after this test.

Tarrasch · 2017-01-03T03:44:06Z

test/task_test.py

+ # This first check is just an assumption rather than assertion
+ self.assertTrue(self.run_locally(['MyTask']))
+ luigi.task.externalize(MyTask)
+ self.assertTrue(self.run_locally(['MyTask']))


Ah... Yea this code is hard to read. I'm adding a comment.

It tests that self.run_locally(['MyTask']) doesn't encounter any "ambiguous task" issues after running the externalize code.

Tarrasch · 2017-01-03T03:48:22Z

luigi/task_register.py

- reg[name] = cls.AMBIGUOUS_CLASS
- else:
- reg[name] = cls
+ if cls._visible_in_registry:


Actually not equivalent. (if it's not visible in registry, we don't want the else clause).

However, I agree we should avoid extra nesting. Made this into a if not blah: continue clause.

Tarrasch · 2017-01-03T03:49:52Z

luigi/task_register.py

+
+ if name in reg and reg[name] != cls and \
+ reg[name] != cls.AMBIGUOUS_CLASS and \
+ not issubclass(cls, reg[name]):


Yes, I think so. I removed the reg[name] != cls check. Nice catch!

Tarrasch · 2017-01-03T04:22:09Z

test/task_test.py

@@ -206,6 +206,24 @@ def run(self):
 self.assertIsNotNone(MyTask.run) # Check immutability
 self.assertIsNotNone(MyTask().run) # Check immutability

+ def test_dont_affects_registry(self):


Tarrasch · 2017-01-03T04:59:43Z

luigi/task_register.py

+ name = cls.get_task_family()
+
+ if name in reg and reg[name] != cls and \
+ reg[name] != cls.AMBIGUOUS_CLASS and \


Great catch. I added a test case. for this. You were right, the current behavior is broken (see soon up and coming update to PR) :)

Tarrasch · 2017-01-03T05:00:16Z

test/util_test.py

+
+ return ParentTask
+
+ def test_requries_has_effect_run_child(self):


Tarrasch · 2017-01-03T05:00:18Z

test/util_test.py

+ self.assertTrue(self.run_locally_split('ChildTask --my-parameter actuallyset'))
+ self.assertEqual(ParentTask.class_variable, 'actuallyset')
+
+ def test_requries_has_effect_run_parent(self):


Tarrasch · 2017-01-03T05:02:27Z

test/util_test.py

@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-from helpers import LuigiTestCase


Actually not completely unrelated. Much of the machinery here is relying on the Registry that got changed here.

I'll amend the commit message saying I intentionally added these tests for this change set.

This fixes an implementation detail, so that luigi.task.externalize actually does what it says it does in the documentation. Hence I hope pretty uncontroversial. This makes it so that calling externalize don't register a new task. A pretty serious bug. Now, there's still a caveat, as we're mostly hiding that we do side affects, if anyone would actually inherit from what is returned by externalize. That subtask wouldn't be registered either. This code is quite easy to get wrong somewhere. Luckily luigi is decently tested. To make me even more confident I also added a couple of tests: * Two tests in task_test.py that didn't pass before. * I created the task_register_test.py file. * I added some util tests too, because they are also all about Copying classes and namespace clashes. And I want to be on the safe side.

daveFNbuck · 2017-01-03T16:35:44Z

luigi/task_register.py

 """
 # We have to do this on-demand in case task names have changed later
- # We return this in a topologically sorted list of inheritance: this is useful in some cases (#822)


Did the use case in #822 go away?

Yes, since #1182, the Parameter class became (more) immutable, and a lots of these random corner-cases bugs went away. There were also test cases added with the fix for #822, and I assume green Travis means they still pass. :)

Tarrasch · 2017-01-05T02:14:20Z

Wohoo, I'm starting to feel ready to do a 2.5 release 😎

This fixes an implementation detail, so that luigi.task.externalize actually does what it says it does in the documentation. Hence I hope pretty uncontroversial. This makes it so that calling externalize don't register a new task. A pretty serious bug. Now, there's still a caveat, as we're mostly hiding that we do side affects, if anyone would actually inherit from what is returned by externalize. That subtask wouldn't be registered either. This code is quite easy to get wrong somewhere. Luckily luigi is decently tested. To make me even more confident I also added a couple of tests: * Two tests in task_test.py that didn't pass before. * I created the task_register_test.py file. * I added some util tests too, because they are also all about Copying classes and namespace clashes. And I want to be on the safe side.

daveFNbuck reviewed Dec 28, 2016

View reviewed changes

Tarrasch commented Jan 3, 2017

View reviewed changes

Tarrasch force-pushed the fix-externalize branch from 00d0769 to 613d804 Compare January 3, 2017 06:58

daveFNbuck reviewed Jan 3, 2017

View reviewed changes

daveFNbuck approved these changes Jan 4, 2017

View reviewed changes

Tarrasch merged commit f7e0b77 into spotify:master Jan 5, 2017

Tarrasch deleted the fix-externalize branch January 5, 2017 02:13

This was referenced Jun 29, 2022

no mo enum 34 #3180

Closed

enum34 be gone #3181

Closed

mdragilev mentioned this pull request Jun 28, 2024

for S3 contrib package move to boto3 Affirm/luigi#26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

externalize: Dont register the copied tasks #1975

externalize: Dont register the copied tasks #1975

Tarrasch commented Dec 28, 2016

mention-bot commented Dec 28, 2016

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

daveFNbuck Dec 28, 2016

Tarrasch Jan 3, 2017

Tarrasch left a comment

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

Tarrasch Jan 3, 2017

daveFNbuck Jan 3, 2017

Tarrasch Jan 4, 2017

Tarrasch commented Jan 5, 2017 •

edited

Loading


		return ParentTask

		def test_requries_has_effect_run_child(self):

externalize: Dont register the copied tasks #1975

externalize: Dont register the copied tasks #1975

Conversation

Tarrasch commented Dec 28, 2016

Have you tested this? If so, how?

mention-bot commented Dec 28, 2016

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

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

Tarrasch 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tarrasch commented Jan 5, 2017 • edited Loading

Tarrasch commented Jan 5, 2017 •

edited

Loading