DM-23436: apdb creation instructions outdated #51
Conversation
doc/lsst.ap.pipe/apdb.rst
Outdated
| The Alert Production Pipeline, as represented by :lsst-task:`lsst.ap.pipe.ApPipeTask` and executed by |ap_pipe|, delegates saving and loading DIASources and DIAObjects to its ``diaPipe`` subtask. | ||
| In principle, different implementations of ``diaPipe`` can be selected in defined in `ApPipeTask`'s configuration (by setting :lsst-config-field:`lsst.ap.pipe.ap_pipe.ApPipeConfig.diaPipe`), but the default choice --- :lsst-task:`lsst.ap.association.DiaPipelineTask` --- is expected to be appropriate for most uses. | ||
| `~lsst.ap.association.DiaPipelineTask` uses a database --- known as the Alert Production Database, or APDB --- to store its results. |
There was a problem hiding this comment.
I think talking about diaPipe may be too much detail for the context paragraph. The target audience here is people who see the AP pipeline as a single unit and want to know how to run it from (almost) scratch. Why not delay mention of diaPipe or DiaPipelineTask to the instructions in the next section?
Or are you actually forseeing a future where, depending on the config, the AP pipeline might not need any sort of APDB?
There was a problem hiding this comment.
This is a good point, and I wondered about it when writing the text.
My foresight is not as good as you imply it might be. However, given that the APDB usage is isolated in a retargetable subtask, I'm a bit leery of talking about it as being fundamental to the design of the pipeline. Given your comments, though, maybe that's a worthwhile simplification. I'll do some redrafting.
There was a problem hiding this comment.
That's a good point. We don't want DiaPipelineTask to be a subtask; we wrote it so that it would be a standalone PipelineTask in Gen 3, so there's not even an implicit contract that any alternative has to provide database support.
However, I don't think we'll be able to write a truly generic description of how this should work until DM-22663, so I think the simplification is worthwhile for now.
doc/lsst.ap.pipe/apdb.rst
Outdated
| |ap_pipe| includes information about the database location and schema in its `ApPipeConfig`, and most users pass this information to the script through the :option:`--config <ap_pipe.py --config>` and :option:`--configfile <ap_pipe.py --configfile>` command-line options. | ||
| The database is configured using an instance of `~lsst.dax.apdb.ApdbConfig`, which is made accessible through `ApPipeConfig`'s ``diaPipe.apdb`` option. | ||
| |ap_pipe| command line users can pass configuration information to the script through the :option:`--config <ap_pipe.py --config>` and :option:`--configfile <ap_pipe.py --configfile>` command-line options. |
There was a problem hiding this comment.
I think it's important to make it clear from the first sentence that this paragraph is talking about how the database is configured in the context of ap_pipe.py (which the second paragraph then extends to make_apdb.py). The fact that these two scripts share a config is very useful but potentially very confusing.
How about simply removing the passive voice? "|ap_Pipe| configures the database using an instance..."
There was a problem hiding this comment.
I'm confused about the point here. Surely ApPipeTask is the fundamental construct? Under what circumstances would I be using ApPipeTask but not configuring the database through ApPipeConfig.diaPipe.apdb? I'm worried that I'm being slow to catch on! :-)
There was a problem hiding this comment.
ApPipeTask is the fundamental construct, but this page is specifically about how to use make_apdb.py, not ApPipeTask. Mentioning the latter at all, while necessary to explain the shared UI, introduces some mental friction, or at least it has in the past for other AP developers.
Because: - It's not obvious why this was here rather than in the doc/ dir. - The fact it has been outdated for some time implies nobody reads it anyway!
jdswinbank
force-pushed
the
tickets/DM-23436
branch
from
f949b9e
to
90f6593
Compare
To facilitate some later author adding content.
jdswinbank
force-pushed
the
tickets/DM-23436
branch
from
90f6593
to
9d7043f
Compare
No description provided.