externalize: Dont register the copied tasks

luigi/task.py

@@ -239,6 +239,8 @@ def task_module(self):
         # TODO(erikbern): we should think about a language-agnostic mechanism
         return self.__class__.__module__
 
+    _visible_in_registry = True  # TODO: Consider using in luigi.util as well
+
     __not_user_specified = '__not_user_specified'
 
     task_namespace = __not_user_specified
@@ -724,7 +726,7 @@ class RequiringTask(luigi.Task):
         @_task_wraps(clazz)
         class _CopyOfClass(clazz):
             # How to copy a class: http://stackoverflow.com/a/9541120/621449
-            pass
+            _visible_in_registry = False
         _CopyOfClass.run = None
         return _CopyOfClass
     else:

luigi/task_register.py

@@ -19,7 +19,6 @@
 """
 
 import abc
-from collections import OrderedDict
 
 from luigi import six
 import logging
@@ -129,23 +128,24 @@ def task_family(cls):
     def _get_reg(cls):
         """Return all of the registered classes.
 
-        :return:  an ``collections.OrderedDict`` of task_family -> class
+        :return:  an ``dict`` of task_family -> class
         """
         # 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)
-        reg = OrderedDict()
-        for cls in cls._reg:
-            name = cls.task_family
-
-            if name in reg and reg[name] != cls and \
-                    reg[name] != cls.AMBIGUOUS_CLASS and \
-                    not issubclass(cls, reg[name]):
+        reg = dict()
+        for task_cls in cls._reg:
+            if not task_cls._visible_in_registry:
+                continue
+
+            name = task_cls.get_task_family()
+            if name in reg and \
+                    (reg[name] == Register.AMBIGUOUS_CLASS or  # Check so issubclass doesn't crash
+                     not issubclass(task_cls, reg[name])):
                 # Registering two different classes - this means we can't instantiate them by name
                 # The only exception is if one class is a subclass of the other. In that case, we
                 # instantiate the most-derived class (this fixes some issues with decorator wrappers).
-                reg[name] = cls.AMBIGUOUS_CLASS
+                reg[name] = Register.AMBIGUOUS_CLASS
             else:
-                reg[name] = cls
+                reg[name] = task_cls
 
         return reg
 

test/task_register_test.py

@@ -0,0 +1,55 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright 2017 VNG Corporation
+#
+# 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
+
+import luigi
+from luigi.task_register import (Register,
+                                 TaskClassNotFoundException,
+                                 TaskClassAmbigiousException,
+                                 )
+
+
+class TaskRegisterTest(LuigiTestCase):
+
+    def test_externalize_taskclass(self):
+        with self.assertRaises(TaskClassNotFoundException):
+            Register.get_task_cls('scooby.Doo')
+
+        class Task1(luigi.Task):
+            @classmethod
+            def get_task_family(cls):
+                return "scooby.Doo"
+
+        self.assertEqual(Task1, Register.get_task_cls('scooby.Doo'))
+
+        class Task2(luigi.Task):
+            @classmethod
+            def get_task_family(cls):
+                return "scooby.Doo"
+
+        with self.assertRaises(TaskClassAmbigiousException):
+            Register.get_task_cls('scooby.Doo')
+
+        class Task3(luigi.Task):
+            @classmethod
+            def get_task_family(cls):
+                return "scooby.Doo"
+
+        # There previously was a rare bug where the third installed class could
+        # "undo" class ambiguity.
+        with self.assertRaises(TaskClassAmbigiousException):
+            Register.get_task_cls('scooby.Doo')

test/task_test.py

@@ -206,6 +206,26 @@ def run(self):
         self.assertIsNotNone(MyTask.run)  # Check immutability
         self.assertIsNotNone(MyTask().run)  # Check immutability
 
+    def test_externalize_doesnt_affect_the_registry(self):
+        class MyTask(luigi.Task):
+            pass
+        reg_orig = luigi.task_register.Register._get_reg()
+        luigi.task.externalize(MyTask)
+        reg_afterwards = luigi.task_register.Register._get_reg()
+        self.assertEqual(reg_orig, reg_afterwards)
+
+    def test_can_uniquely_command_line_parse(self):
+        class MyTask(luigi.Task):
+            pass
+        # This first check is just an assumption rather than assertion
+        self.assertTrue(self.run_locally(['MyTask']))
+        luigi.task.externalize(MyTask)
+        # Now we check we don't encounter "ambiguous task" issues
+        self.assertTrue(self.run_locally(['MyTask']))
+        # We do this once again, is there previously was a bug like this.
+        luigi.task.externalize(MyTask)
+        self.assertTrue(self.run_locally(['MyTask']))
+
 
 class TaskNamespaceTest(LuigiTestCase):
 

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
+from helpers import LuigiTestCase, RunOnceTask
 
 import luigi
 import luigi.task
@@ -23,25 +23,25 @@
 
 class BasicsTest(LuigiTestCase):
 
-    def test_requries(self):
-        class BaseTask(luigi.Task):
+    def test_task_ids_using_requries(self):
+        class ParentTask(luigi.Task):
             my_param = luigi.Parameter()
         luigi.namespace('blah')
 
-        @requires(BaseTask)
+        @requires(ParentTask)
         class ChildTask(luigi.Task):
             pass
         luigi.namespace('')
         child_task = ChildTask(my_param='hello')
         self.assertEqual(str(child_task), 'blah.ChildTask(my_param=hello)')
-        self.assertIn(BaseTask(my_param='hello'), luigi.task.flatten(child_task.requires()))
+        self.assertIn(ParentTask(my_param='hello'), luigi.task.flatten(child_task.requires()))
 
-    def test_requries_weird_way(self):
+    def test_task_ids_using_requries_2(self):
         # Here we use this decorator in a unnormal way.
         # But it should still work.
-        class BaseTask(luigi.Task):
+        class ParentTask(luigi.Task):
             my_param = luigi.Parameter()
-        decorator = requires(BaseTask)
+        decorator = requires(ParentTask)
         luigi.namespace('blah')
 
         class ChildTask(luigi.Task):
@@ -50,4 +50,31 @@ class ChildTask(luigi.Task):
         ChildTask = decorator(ChildTask)
         child_task = ChildTask(my_param='hello')
         self.assertEqual(str(child_task), 'blah.ChildTask(my_param=hello)')
-        self.assertIn(BaseTask(my_param='hello'), luigi.task.flatten(child_task.requires()))
+        self.assertIn(ParentTask(my_param='hello'), luigi.task.flatten(child_task.requires()))
+
+    def _setup_parent_and_child(self):
+        class ParentTask(luigi.Task):
+            my_parameter = luigi.Parameter()
+            class_variable = 'notset'
+
+            def run(self):
+                self.__class__.class_variable = self.my_parameter
+
+            def complete(self):
+                return self.class_variable == 'actuallyset'
+
+        @requires(ParentTask)
+        class ChildTask(RunOnceTask):
+            pass
+
+        return ParentTask
+
+    def test_requires_has_effect_run_child(self):
+        ParentTask = self._setup_parent_and_child()
+        self.assertTrue(self.run_locally_split('ChildTask --my-parameter actuallyset'))
+        self.assertEqual(ParentTask.class_variable, 'actuallyset')
+
+    def test_requires_has_effect_run_parent(self):
+        ParentTask = self._setup_parent_and_child()
+        self.assertTrue(self.run_locally_split('ParentTask --my-parameter actuallyset'))
+        self.assertEqual(ParentTask.class_variable, 'actuallyset')