I'm not sure how a run method with a Task-specific list of arguments is "uniform", but I generally agree with the thinking behind this. It seems based on a quick sample that most plain Task s are OK; CmdLineTask s may need to change from run/other name to runDataRef/run, and pipe.base.TaskRunner needs to change as you mention (along with the documentation).

I agree about CmdLineTask taking runDataRef.

I disagree every that Task must have a run method. When a task performs one primary job on one set of data types (one argument list), then I agree that method should be called run}. However, {{Task is a very useful building block: as a configurable object with a log. As such, there are some uses which do not fit into those narrow confines, and in that case I think the methods should be named for clarity of what they do. There are two reasons to violate this, as suggested above:

• A task that can perform one job, but on more than one type of data. Since Python does not lend itself to method overloading, it is more natural to have multiple methods, instead of using dispatching based on data type.
• A task may perform a related set of different operations.

Russell, If tasks are small building blocks, wouldn't it be better to have many small tasks, that have smaller configurations and smaller code bases? If they are related then the could share a common base class, or just use a set of free functions in a module all of the smaller tasks could import?

I think I agree with Nate Lust on this. It seems like knowing what method to call on a Task is a win. Breaking tasks that do multiple things into multiple tasks involves duplicating a little boilerplate, but improves clarity.

One entry point per Task will help with docs too. It'll be easier to design standardized Task documentation pages when there's a 1:1 correspondence between a task and its functionality as opposed to having potentially a few useful and distinct entry points. Having secondary entry points like runDataRef are still easy to document since they can be understood as convenience wrappers for the main run method.

Look at afw.math.Warper for an example where the code does the same thing for different sorts of arguments. One run method won't cut it. It's not a task, but only to avoid afw depending on pipe_base. It really ought to be a task – it's configurable and meant to be called many times with the same configuration.

I'm surprised this is such an issue. Tasks almost always need good documentation to be used effectively (or at all) because the arguments and configuration are so different from one task to the next. I think insisting on a run method will backfire in some cases, resulting in a run method that does dispatching based on the types of its arguments (not a Pythonic thing to do) or breaking up a self-contained piece of code into smaller units than seem called for. I agree it is a good recommendation that will apply to most tasks, but strongly disagree that it should be mandated.

As to one entry point per task: many of our tasks have multiple useful entry points. Even when they have a "run" method they aren't always used by calling that method. The astrometry solvers are a good example. In fact it is strongly recommended to break a task into multiple public methods, so that a variant task can override methods as appropriate. Those won't necessarily be called by other tasks, but they are part of the public API and might be called.

I have looked through many of the tasks. I agree that most have one dominant entry point, which could be called run. An example of a task that has no dominant entry point is LoadReferenceObjects (and its catalog-specific subclasses). It offers loadSkyCircle and loadPixelBox. Both are important. Both are used. Both belong together in the same task. I feel it would be a huge mistake to rename one to run.

To reiterate, I think there are two cases where run is unacceptable:

• When a task offers multiple entry points. That is rare but there are sometimes good reasons for it.
• When a task can process different kinds of data (e.g. Warper, which can warp images and exposures). That can be supported with a run method, but only by using ugly and non-pythonic code.

I'm willing to go with run and one entry point as the standard recommendation, but not as a *must".

I did not take this proposal to mean that standardizing on an interface does not mean there must not be any other entry points, but that the standard one must be available and useful. Am I wrong in this?

Also as for function overloading through dispatch being non pythonic, I must push back on that a bit. The standard library even includes decorators for doing just that, see: https://www.python.org/dev/peps/pep-0443/

My assertion is that LoadReferenceObjects has no natural dominant entry point. It offers multiple region shapes that you can load, with the potential to add more if wanted. To make code readable it is important that the shape being loaded be explicitly specified, rather than using a vague term like run and forcing readers (and writers) of the code to look up what shape that happens to load.

Again...most of the time a task will have one dominant entry point and calling it run is fine. I just think there are natural exceptions that we should allow. Especially since Task is a very generally useful class.

In addition, I feel we lose some readability by mandating run for the primary method I prefer a more descriptive verb, if there is one.

I want our code to be as readable as possible. Readability trumps writability because code that is hard to read is hard to understand and hard to maintain. In addition, a coder has to read the task docs in order to call a task. Finding the desired method should never take more than a few seconds (we have a standard doc section describing how to call a task); the hard part is understanding the arguments.

Note that I am in full agreement with using runDataRef as a required method name for CmdLineTask. By definition command-line tasks must have a method that is called when the task is run from the command line, and it should have a standard name for many reasons.

I think I want to back off on my idea that there may still be multiple entry points. Sometimes there may seem to be a natural insertion point other than run, but might this is an indication that we might need more smaller tasks? If we have something (supertasks?) that can build a task tree from a DAG and config, maybe many much smaller tasks would be better?

I am just musing aloud at this point, it is possible multiple entry points would be better. I am imagining a point where you would not have to hack your way into the middle of a task to figure something out on the command line, but simply request that a new object with certain sub-tasks be built for you, inserting your own new stuff somewhere in the tree. In that scenario more small tasks that may share calls to common free functions may be what is required. Taken to the logical extreme though that would just end up with all tasks only having a run method, which would basically just mean everything is itself basically a single function and glue to chain them together. So the question is what constitutes the minimum meaningful unit of work.

I have not used LoadReferenceObjectsTask before, and have only looked at it briefly, but it seems to me that this does not really need to be a task at all. It seems to be just a collection of functions, which could be in a normal python module. Or if desired could be put into a class with normal class init defaults. If configuration is desired those config options can be put in the config class of the task which calls this code. There are plenty of other examples of having default arguments to functions in the stack. Like I said, I may be missing some bigger picture though having not used the code much myself.

Tasks are our unit of reuse, and I think we should think of them this way. We could have called run _call_ but decided that that was too magic, but basically that's what we are doing. Viewed this way, Russell's example of LoadReferenceObjects would have a run method (which loads reference objects) with an argument to specify shape=skyCircle or pixelBox (and yes, I know that the corresponding arguments are different, so maybe shape={skyCircle, centre=..., radius=...} or an object to describe the desired shape).

In particular, they are not a "very useful building block: ... a configurable object with a log".

Robert Lupton's point about LoadReferenceObjects is well taken, but because the arguments differ I feel the current design is cleaner and easier to use. I would rather we give task authors permission to make the simplest possible design, rather than sacrifice simplicity for the purity of always having a run method.

I have wondered about our use of run instead of _call_ myself; it certainly doesn't feel "magic" to me. If we were to standardize on something, I'd suggest we also revisit that.

I have to agree with Russell Owen: LoadReferenceObjects seems much better served by loadSkyCircle(center, radius) and loadPixelBox(bbox, wcs), instead of e.g. run(shape="skyCircle", center=0, radius=1); the latter seems much clumsier to me. However, this discussion does suggest that LoadReferenceObjects is a different kind of Task from most of the others, in some way. It might be good to catalog all the tasks and see which of them clearly could follow the run pattern, and which might not be able to. That could tell us what the optimal approach is (and also how much work it'll take to clean everything up).

Separately, where does jointcal-and other "multi dataRef" Tasks-fit into this system? jointcal has a run that takes a list of dataRefs, and that's the fundamental unit that it operates on. I suspect we'll have (if we don't already) other similar Tasks, e.g. for creating AP templates images.

Thanks, folks, for the thoughtful discussion.

I regret that the decision was made not to define __call__ on Task objects: doing that years ago would have made the thinking behind Task​s clearer and avoided the need to have this discussion today. I'm reluctant to revisit that discussion in the context of this RFC, though.

My first instinct was that the LoadReferenceObjects case should be addressed by subclassing rather than a profusion of method names. Why not have separate subclasses for sky circles and pixel boxes? I guess the issue here is that we run into a complicated inheritance structure, because we're already subclassing for different catalogues. Relying too much on this specific example makes me nervous, though: I reckon John Parejko is getting to the nub of the issue when he talks about LoadReferenceObjects being a "different kind of Task", and I'm worried that hard cases make bad policy. Russell Owen, per the above you've looked through many of our tasks recently; are there other cases you're particularly worried about? (I'm lazy, and would prefer not to have to go and make a catalog as John suggests...!)

John Parejko — I'm not sure that “multi-dataRef” Task​s are qualitatively different from others. We'd still want a run that took concrete arguments, and a runDataRef (or "refs", maybe) that unpacked them (presumably using proxies, to avoid instantiating everything at once).

Extending the end date of this RFC until Monday 2017-06-26 since the discussion hasn't quite converged.

I think LoadReferenceObjects already has a good design. Subclassing for different shapes is inferior. The different classes are for different catalog formats. In many cases support for additional methods can be added in the base class without any changes needed in subclasses.

I feel as if we're spending too much time trying to pound square pegs into round holes.

Clearly most tasks naturally have one entry point (including all command-line tasks) and in that case they should have a run method (or __call__ if you prefer. But there will be a few tasks that do not naturally fit this pattern – LoadReferenceObjects is an example. In that situation I think we would be much better off allowing a simple, clean solution, instead of forcing the task into a mold that is a bad fit.

I looked through all the non-command-line tasks I could easily find (I don't claim it was exhaustive). What I found:

• Only one other instance where there was no obvious single entry point: BaseReferencesTask.
• Several tasks with one obvious entry point, but whose name is not descriptive enough that it's clear what run does. Some examples: CatalogCalculationTask, MockObjectTask. I'm not trying to pick on anybody, I'm just saying that run isn't always descriptive (especially if the task name is a noun, but it's not always easy to find a suitable verb). Of course when a task uses a subtask it doesn't refer to the task by its full class name, it uses a config field name, so it's really that config name that needs to be clear enough that it's roughly clear what name.run does, else the code becomes difficult to read.

So I assert we have an existence proof that in rare but useful cases there is more than one natural entry point for a non-command-line task.

In addition, I worry that in some cases run is too vague, so I recommend allowing a more descriptive verb than run when it significantly enhances readability. I believe code should be as easy to read and self-documenting as we can reasonably manage. However, I am willing to cede this for tasks that have a single natural entry point since I seem to be the only one arguing in favor of this.

Thanks Russell Owen, that's useful.

So here's my proposal: we document and require that the normal entry point to tasks is run. We will permit exceptions to that, by means of RFC — anybody who is (re-)designing or implementing a task, and who doesn't feel that run is appropriate, can file an RFC to explain why and get an exemption.

Implementation tickets for this RFC will including fixing up all old tasks to the new standards. As that happens, we'll file RFCs for those tasks (presumably to include LoadReferenceObjects) where we feel an exemption is appropriate.

Objections?

I've added two implementation tickets to the RFC and intend to adopt it on that basis unless somebody objects today.

Let's try that. I'm uneasy about requiring an RFC, in that it sends the message that we don't trust our developers to do the right thing. But it's much better than having no exceptions allowed, and if makes sense to be especially careful until the new rule becomes ingrained.

Thanks Russell Owen, all.

This RFC was just mentioned on #dm-middleware-dev in the context of a PR against daf_butler.

Seeing that it was still in ADOPTED state, I had a look, and the implementation ticket DM-11098 "Audit existing tasks and update them to comply with RFC-352" is still in the To-Do state. This seems like it has surely been superseded by the Gen3 migration - and in addition all the subtasks of DM-11098 are complete.

Can we close DM-11098 and move this RFC to IMPLEMENTED?