Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
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.
Attachments
Issue Links
Activity
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.
Adopted as follows:
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.
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.
I see the workflow does not allow an RFC to transition out of Implemented. This was marked as implemented back in June 2016!
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 = Noneinstead of
config.calibrate.detectAndMeasure.measurement.slots.psfFlux = NoneI 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.