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

DM-30148: PipelineTasks use wrong label as name #123

Merged
merged 3 commits into from
May 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions COPYRIGHT
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Copyright 2016 University of Illinois Champaign-Urbana
Copyright 2016-2018 The Board of Trustees of the Leland Stanford Junior University, through SLAC National Accelerator Laboratory
Copyright 2018 The Trustees of Princeton University
Copyright 2016 University of Illinois Board of Trustees
Copyright 2016-2021 The Board of Trustees of the Leland Stanford Junior University, through SLAC National Accelerator Laboratory
Copyright 2018-2021 The Trustees of Princeton University
Copyright 2019-2021 Association of Universities for Research in Astronomy, Inc. (AURA)
Copyright 2021 University of Washington
8 changes: 4 additions & 4 deletions python/lsst/ctrl/mpexec/taskFactory.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@
class TaskFactory(BaseTaskFactory):
"""Class instantiating PipelineTasks.
"""
def makeTask(self, taskClass, name, config, overrides, butler):
def makeTask(self, taskClass, label, config, overrides, butler):
"""Create new PipelineTask instance from its class.

Parameters
----------
taskClass : type
PipelineTask class.
name : `str` or `None`
The name of the new task; if `None` then use
label : `str` or `None`
The label of the new task; if `None` then use
``taskClass._DefaultName``.
config : `pex.Config` or None
Configuration object, if ``None`` then use task-defined
Expand Down Expand Up @@ -93,6 +93,6 @@ def makeTask(self, taskClass, name, config, overrides, butler):
config.freeze()

# make task instance
task = taskClass(config=config, initInputs=initInputs, name=name)
task = taskClass(config=config, initInputs=initInputs, name=label)

return task
162 changes: 162 additions & 0 deletions tests/test_taskFactory.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
# This file is part of ctrl_mpexec.
#
# Developed for the LSST Data Management System.
# This product includes software developed by the LSST Project
# (https://www.lsst.org).
# See the COPYRIGHT file at the top-level directory of this distribution
# for details of code ownership.
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <https://www.gnu.org/licenses/>.

import shutil
import tempfile
import unittest

import lsst.afw.table as afwTable
import lsst.daf.butler.tests as butlerTests
import lsst.pex.config as pexConfig
from lsst.pipe.base import PipelineTaskConfig, PipelineTaskConnections
from lsst.pipe.base.configOverrides import ConfigOverrides
from lsst.pipe.base import connectionTypes

from lsst.ctrl.mpexec import TaskFactory


class FakeConnections(PipelineTaskConnections, dimensions=set()):
initInput = connectionTypes.InitInput(name="fakeInitInput", doc="", storageClass="SourceCatalog")
initOutput = connectionTypes.InitOutput(name="fakeInitOutput", doc="", storageClass="SourceCatalog")
input = connectionTypes.Input(name="fakeInput", doc="", storageClass="SourceCatalog", dimensions=set())
output = connectionTypes.Output(name="fakeOutput", doc="", storageClass="SourceCatalog", dimensions=set())


class FakeConfig(PipelineTaskConfig, pipelineConnections=FakeConnections):
widget = pexConfig.Field(dtype=float, doc="")


def mockTaskClass():
"""A class placeholder that records calls to __call__.
"""
mock = unittest.mock.Mock(__name__="_TaskMock", _DefaultName="FakeTask", ConfigClass=FakeConfig)
return mock


class TaskFactoryTestCase(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a docstring: it's not obvious to me what a "TaskFactory" test would be testing. I think it's testing TaskFactory.makeTask() with a variety of combinations of optional arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

It tests the functionality of TaskFactory (which happens to consist of one method)? I'm not sure what I could say that wouldn't be obvious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, maybe the fact that TaskFactory itself is short on documentation is really my problem? I was looking at the test without knowing anything about TaskFactory: I didn't know it only had one method.


@classmethod
def setUpClass(cls):
super().setUpClass()

tmp = tempfile.mkdtemp()
cls.addClassCleanup(shutil.rmtree, tmp, ignore_errors=True)
cls.repo = butlerTests.makeTestRepo(tmp)
butlerTests.addDatasetType(cls.repo, "fakeInitInput", set(), "SourceCatalog")
butlerTests.addDatasetType(cls.repo, "fakeInitOutput", set(), "SourceCatalog")
butlerTests.addDatasetType(cls.repo, "fakeInput", set(), "SourceCatalog")
butlerTests.addDatasetType(cls.repo, "fakeOutput", set(), "SourceCatalog")

def setUp(self):
super().setUp()

self.factory = TaskFactory()
self.constructor = mockTaskClass()

@staticmethod
def _alteredConfig():
config = FakeConfig()
config.widget = 42.0
return config

@staticmethod
def _overrides():
overrides = ConfigOverrides()
overrides.addValueOverride("widget", -1.0)
return overrides

@staticmethod
def _dummyCatalog():
schema = afwTable.SourceTable.makeMinimalSchema()
return afwTable.SourceCatalog(schema)

def _tempButler(self):
butler = butlerTests.makeTestCollection(self.repo, uniqueId=self.id())
catalog = self._dummyCatalog()
butler.put(catalog, "fakeInitInput")
butler.put(catalog, "fakeInitOutput")
butler.put(catalog, "fakeInput")
butler.put(catalog, "fakeOutput")
return butler

def testOnlyMandatoryArg(self):
self.factory.makeTask(taskClass=self.constructor, label=None, config=None,
overrides=None, butler=None)
self.constructor.assert_called_with(config=FakeConfig(), initInputs=None, name=None)

def testAllArgs(self):
butler = self._tempButler()
self.factory.makeTask(taskClass=self.constructor, label="no-name", config=self._alteredConfig(),
overrides=self._overrides(), butler=butler)
catalog = butler.get("fakeInitInput") # Copies of _dummyCatalog are identical but not equal
# When config passed in, overrides ignored
self.constructor.assert_called_with(config=self._alteredConfig(),
initInputs={'initInput': catalog},
name="no-name")

# Can't test all 14 remaining combinations, but the 6 pairs should be enough coverage

def testNameConfig(self):
self.factory.makeTask(taskClass=self.constructor, label="no-name", config=self._alteredConfig(),
overrides=None, butler=None)
self.constructor.assert_called_with(config=self._alteredConfig(), initInputs=None, name="no-name")

def testNameOverrides(self):
self.factory.makeTask(taskClass=self.constructor, label="no-name", config=None,
overrides=self._overrides(), butler=None)
config = FakeConfig()
self._overrides().applyTo(config)
self.constructor.assert_called_with(config=config, initInputs=None, name="no-name")

def testNameButler(self):
butler = self._tempButler()
self.factory.makeTask(taskClass=self.constructor, label="no-name", config=None,
overrides=None, butler=butler)
catalog = butler.get("fakeInitInput") # Copies of _dummyCatalog are identical but not equal
self.constructor.assert_called_with(config=FakeConfig(),
initInputs={'initInput': catalog},
name="no-name")

def testConfigOverrides(self):
self.factory.makeTask(taskClass=self.constructor, label=None, config=self._alteredConfig(),
overrides=self._overrides(), butler=None)
# When config passed in, overrides ignored
self.constructor.assert_called_with(config=self._alteredConfig(), initInputs=None, name=None)

def testConfigButler(self):
butler = self._tempButler()
self.factory.makeTask(taskClass=self.constructor, label=None, config=self._alteredConfig(),
overrides=None, butler=butler)
catalog = butler.get("fakeInitInput") # Copies of _dummyCatalog are identical but not equal
self.constructor.assert_called_with(config=self._alteredConfig(),
initInputs={'initInput': catalog},
name=None)

def testOverridesButler(self):
butler = self._tempButler()
self.factory.makeTask(taskClass=self.constructor, label=None, config=None,
overrides=self._overrides(), butler=butler)
config = FakeConfig()
self._overrides().applyTo(config)
catalog = butler.get("fakeInitInput") # Copies of _dummyCatalog are identical but not equal
self.constructor.assert_called_with(config=config,
initInputs={'initInput': catalog},
name=None)