# Variant tasks should have a registry and an abstract base class

XMLWordPrintable

#### Details

• Type: RFC
• Status: Implemented
• Resolution: Done
• Component/s:
• Labels:
None
• Location:
here

#### Description

When there are different versions of a given task, I propose that we require those tasks to have an abstract base class that defines the interface.

I further propose that the same file that defines the abstract base class also define a registry, and all implementations register themselves. This has several advantages:

• Finding variant subtasks is easier
• Switching between variant subtasks can be done from the command line (by comparison, retargeting requires a config override file)
• Switching between variant subtasks is safer because they all have the same API
• Config override files or users can configure more than one variant, again improving the ease and safety of using variants

Details of the proposal:

• Abstract base classes should use the abc package
• A task should have an abstract base class if it has multiple implementations or is likely to have them. Otherwise I suggest not bothering, in order to avoid clutter.
• Use lsst.pex.config.RegistryField for the registry. It is simple and easy to use. Note that lsst.pipe.base.Task.makeSubtask will need minor work to support this.

Once this is implemented I anticipate essentially all retargeting of subtasks to be replaced by use of the registry.

#### Activity

Hide
Jim Bosch added a comment -

I'd be happy to comment on an explicit proposal for which places to use RegistryField; the ones you've listed sound reasonable.

What I'm trying to avoid is config-setter lines that look like this:

 config.calibrate["DefaultCalibrate"].detectAndMeasure["DefaultDetectAndMeasure"].measurement["SingleFrameMeasurement"].slots.psfFlux = None 

 config.calibrate.detectAndMeasure.measurement.slots.psfFlux = None 

I know you're not proposing we add registries for all of these, but I'd like to be explicit about that.

The sort of hybrid field I have in mind is one in which the RegistryField dict-like syntax is always allowed, but one can also modify the active field the same way one would modify a regular ConfigurableField.

One more thought: we should think about defining interfaces (perhaps not as ABCs) for the config classes as well. For most of these retargetable slots, I believe there are some config values that are likely common to all subclasses and others that are unique.

Show
Jim Bosch added a comment - I'd be happy to comment on an explicit proposal for which places to use RegistryField; the ones you've listed sound reasonable. What I'm trying to avoid is config-setter lines that look like this: config.calibrate["DefaultCalibrate"].detectAndMeasure["DefaultDetectAndMeasure"].measurement["SingleFrameMeasurement"].slots.psfFlux = None instead of config.calibrate.detectAndMeasure.measurement.slots.psfFlux = None I know you're not proposing we add registries for all of these, but I'd like to be explicit about that. The sort of hybrid field I have in mind is one in which the RegistryField dict-like syntax is always allowed, but one can also modify the active field the same way one would modify a regular ConfigurableField. One more thought: we should think about defining interfaces (perhaps not as ABCs) for the config classes as well. For most of these retargetable slots, I believe there are some config values that are likely common to all subclasses and others that are unique.
Hide
Russell Owen added a comment -

Jim Bosch thank you. That is clear and helpful and I agree that we should not over-use registries.

Also, I hope RegistryField can be modified to support direct (non-dict) access, rather than adding a variant field type. But even so, it's unlikely to ever be as easy to use as ConfigurableField, in which case the latter would continue to be preferred if there are only a few variant tasks to choose from and one is usually the clear best choice.

Show
Russell Owen added a comment - Jim Bosch thank you. That is clear and helpful and I agree that we should not over-use registries. Also, I hope RegistryField can be modified to support direct (non-dict) access, rather than adding a variant field type. But even so, it's unlikely to ever be as easy to use as ConfigurableField, in which case the latter would continue to be preferred if there are only a few variant tasks to choose from and one is usually the clear best choice.
Hide
Russell Owen added a comment -

When there are different versions of a given task in our stack, those tasks must have an abstract base class that defines the interface. This restriction does not apply to one-off and 3rd party code. Such abstract classes must use the abc package.

If there are many versions of a given task (e.g. star selectors) then the same file that defines the abstract base class should also define a registry, using lsst.pex.config.RegistryField, and all implementations should register themselves with that registry.

If a task uses a subtask that has a registry, and it is likely users will want to be able to choose variants of that subtask, then the task should use the registry in its config, instead of lsst.pex.config.ConfigurableField. This makes the variant subtasks easier to find and specify, though it also makes them a bit more painful to configure, due to limitations in RegistryField.

Note that being able to use RegistryField in a task config to specify a subtask requires a minor change to pipe_base.

Show
Hide
John Swinbank added a comment -

I note that this RFC is incorrectly marked as implemented, since DM-6075 is not complete. However, I don't have permission to change its status.

Show
John Swinbank added a comment - I note that this RFC is incorrectly marked as implemented, since DM-6075 is not complete. However, I don't have permission to change its status.
Hide
Tim Jenness added a comment -

I see the workflow does not allow an RFC to transition out of Implemented. This was marked as implemented back in June 2016!

Show
Tim Jenness added a comment - I see the workflow does not allow an RFC to transition out of Implemented. This was marked as implemented back in June 2016!

#### People

Assignee:
Russell Owen
Reporter:
Russell Owen
Watchers:
Jim Bosch, John Swinbank, Kian-Tat Lim, Paul Price, Robert Lupton, Russell Owen, Tim Jenness
0 Vote for this issue
Watchers:
7 Start watching this issue

#### Dates

Created:
Updated:
Resolved:
Planned End:

#### Jenkins

No builds found.