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-33254: Remove Gen 2 support from ap_pipe #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, only minor comments.
Please do build the docs and make sure that the changed files don't have any dangling references, broken formatting, etc., though I didn't see anything worrying when looking at the source.
doc/lsst.ap.pipe/apdb.rst
Outdated
.. |pipetask| replace:: :command:`pipetask` | ||
|
||
|
||
In its default configuration, the Alert Production Pipeline, as represented by :lsst-task:`lsst.ap.pipe.ApPipeTask` (Gen 2) or :file:`pipelines/ApPipe.yaml` (Gen 3), relies on a database to save and load DIASources and DIAObjects. | ||
In its default configuration, the Alert Production Pipeline, as represented by :file:`pipelines/ApPipe.yaml` (Gen 3), relies on a database to save and load DIASources and DIAObjects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove the parenthetical (Gen 3) as well... especially since some new people may get confused and not realize that it's the name of what they're using.
doc/lsst.ap.pipe/apdb.rst
Outdated
Databases can also be set up using :ref:`config files <command-line-task-config-howto-configfile>`: | ||
|
||
.. code-block:: py | ||
:caption: myApdbConfig.py | ||
|
||
config.db_url = "sqlite:///databases/apdb.db" | ||
|
||
.. prompt:: bash | ||
|
||
make_apdb.py -C myApdbConfig.py | ||
ap_pipe.py repo --calib repo/calibs --rerun myrun -C myApPipeConfig.py --id [optional ID to process] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is still relevant; I would keep everything the same except replacing the ap_pipe.py
line with a pipetask run
line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I was a bit confused because of the command-line-task reference, but this isn't actually a command-line task. So this link may break in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but right now those are the only docs at https://pipelines.lsst.io/v/daily/modules/lsst.pipe.base/ that explain how task configs work.
At present, the alert production pipeline is implemented using two separate frameworks that store and retrieve data from "Butler" repositories in incompatible ways: | ||
At present, the alert production pipeline is implemented using the Gen 3 framework: | ||
|
||
- `ApPipeTask` is an `lsst.pipe.base.CmdLineTask` that reads and writes data using the :ref:`lsst.daf.persistence` package. | ||
This is the established "Gen 2" framework. | ||
- ``ApPipe`` is an `lsst.pipe.base.Pipeline` that reads and writes data using the :ref:`lsst.daf.butler` package. | ||
This "Gen 3" framework is expected to be the only implementation in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole intro ought to be rewritten to be less Gen 2-centric, but I'm fine with deferring that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issued as DM-35646.
getting-started-gen2 | ||
pipeline-tutorial-gen2 | ||
apdb | ||
|
||
.. _lsst.ap.pipe-using: | ||
|
||
.. _lsst.ap.pipe-using-gen3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest also removing the "Command Line Task Reference" section starting on new line 64. If this package is now Gen 3 only, then it will never be needed.
30dc20b
to
26de890
Compare
from lsst.ap.pipe import ApPipeTask | ||
|
||
if __name__ == '__main__': | ||
ApPipeTask.parseAndRun() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Overlooked this one. 🙂
Thanks for the doc review! Yes, I had run |
doc/lsst.ap.pipe/apdb.rst
Outdated
config.db_url = "sqlite:///databases/apdb.db" | ||
|
||
.. prompt:: bash | ||
|
||
make_apdb.py -C myApdbConfig.py | ||
ap_pipe.py repo --calib repo/calibs --rerun myrun -C myApPipeConfig.py --id [optional ID to process] | ||
pipetask run -p ApPipe.yaml -C myApPipeConfig.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pipetask run -p ApPipe.yaml -C myApPipeConfig.py | |
pipetask run -p ApPipe.yaml -C myApPipeConfig.py -b repo -o myrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
e7bdd02
to
4f3ed1b
Compare
No description provided.