You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 11 Next »

The Gen3 versions of the AP and DRP pipelines are already able to do at least as much as their Gen2 counterparts, and we currently have no reason to believe that they perform any worse at anything.

We also now have the green light to start removing the Gen2 versions of things, at least according to the release stability promised we've made in the past.

At present, however, some important test coverage still relies on Gen2 middleware and CmdLineTasks - this is a problem in its own right, because it means we're testing things under unrealistic, non-production conditions, and reliance on those tests make it harder to change our tasks in Gen3-only ways.  This page attempts to capture all cases in which we use Gen2 as a test harness, and transform this information into a plan for removing different aspects of the Gen2 middleware.

Gen2 removal tickets

Tickets that involve removing Gen2 functionality should be given the label "gen2-removal" in Jira.  That will make them show up here for easier tracking (until they're done).

Key Summary T Created Updated Due Assignee Reporter P Status Resolution
Loading...
Refresh

Ongoing DRP Gen2-Gen3 parity testing

We've found some major bugs in the Gen3 pipelines only by checking for consistency with Gen2.  Are we done with that effort?  If not, what remains?

This is a blocker for removing any Gen2 task run in DRP, and a blocker for removing Gen2 obs package support for any instrument being used for these parity checks.

Everything else on this page assumes this blocker has been removed; it needs to be updated with more specific blockers in later sections if this work will have a long tail, instead of being a blocker for all Gen2 removal work.

Known Gen2 integration tests and multi-task unit tests

ci_hsc_gen2

This:

  1. Runs the full Gen2 DRP pipeline on HSC data.
  2. Tests that the expected output datasets are actually produced and meet very minimal expectations.
  3. Runs and tests the Gen2 → Gen3 repository conversion tools.
  4. Tests the pipe.base.ShimButler transition convenience code.

(1) is already done by ci_hsc_gen3.

(2) is not, and this is probably the single biggest blocker for removing CmdLineTask inheritance from most hybrid PipelineTask/CmdLineTasks; we cannot remove ci_hsc_gen2 until ci_hsc_gen3 includes those checks ( DM-28806 - Getting issue details... STATUS ).

(3) and (4) are worth keeping around as long as long as the rest of ci_hsc_gen2 has to stay around, since they provide coverage for tools that may still be useful while we are converting Gen2 test code to Gen3.

pipe.tasks.mocks and nopytest_test_coadds.py

This test utility subpackage and the (only) test that uses it provide a lot of early coverage for our coaddition and coadd-processing tasks.  They use very simple (i.e. Gaussian stars, little or no noise) simulations, in order to check that the bookkeeping for CoaddPsf is exactly right on a small amount of data, rather than statistically right on a lot of noisy data.  There's a lot of infrastructure here just to mimic a Gen2 CameraMapper, but it's also tied up with the code that does the simulations.

The test script has also attracted a number of miscellaneous test methods that simply want to check something about the outputs of the tasks the test runs.

Porting all of this coverage to Gen3 would be a lot of work, and I'm skeptical that it's all useful:

  • The zero-noise, simple-PSF simulation framework was intended to be useful for more than CoaddPsf, but no other tests made use of that (that I know of, at least).
  • While that test was helpful in getting CoaddPsf up and running initially, CoaddPsf still shipped with a critical bug in weights that the test was too simple to catch, and a much bigger problem with CoaddPsf involving sigma-clipping plagued us for a while, too.  My takeaway from all of this is that for something like CoaddPsf, large-scale integration tests on realistic data (~RC2 or test-med-1, not just ci_hsc or ci_imsim) are necessary; if we have those, they are also sufficient, and a smaller test like this is only useful as a way to catch some  problems early, especially when there is high churn.
  • CoaddPsf isn't experiencing high churn at all, and in fact we hope to drop it in favor of cell-based coadds someday instead of develop it further.
  • The coverage from running the other tasks here is again mostly just coverage that they run at all, and hence it duplicates coverage we get from ci_hsc_gen3 and ci_imsim.  Nobody should be making changes to these tasks without running those ci_ packages before merge, and is probably developing against real data, not this test, so I don't think discovering failures "earlier" from this test is a strong argument.

So, my tentative recommendation here is to port the test methods that perform checks on the processing outputs to ci_hsc_gen3 and/or ci_imsim, and scrap the rest.

pipe_tasks' test_processCcd.py

This runs ProcessCcdTask on data from obs_test.  It seems to mostly check that the tasks run at all, but also has some purely empirical regression tests ("we got this value once; check that the new one isn't worse").

obs_test is a pure-Gen2 thing, and the pipelines_check package runs the exact same tasks already.  Let's move the checks to tests in that package, and drop this test entirely.

obs_decam's nopytest_test_processCcd.py

This runs processCcd and checks that some output files are are gen2 butler gettable and with some very trivial checks on metadata. This is one of the longest runtime portions of building obs_decam. I suspect it should be replaced with some more direct tests of DECam ISR, and otherwise removed. The most important component, I think, is `testWcPostIsr`.

Guidelines for Gen2 content removal

TODO: what conditions need to be met first?

TODO: what are the typical things to remove?

Guidance on using pytest coverage

Our pytest coverage summary information can help us check whether the removal of tests has caused significant chunks of code to be not executed by tests. John P. will sketch out a guide here to how to use our coverage output for this.

Removing CmdLineTask inheritance from hybrid tasks

Blocked if the task is still run by any Gen2 integration or multi-task unit tests.

What to change:

  • Remove CmdLineTask inheritance
  • Remove RunnerClass class attribute if it exists.
  • Remove any custom TaskRunner subclass used by only this task.
  • Remove class methods, if they exist: applyOverrides, parseAndRun, _makeArgumentParser
  • Remove regular methods, if they exist: runDataRef, writeConfig, writeSchemas, writeMetadata, writePackageVersions, _getConfigName, _getMetadataName
  • Modify __init__ signature to match that of PipelineTask.  Most frequently this will involve making config a keyword-only argument and removing any butler or schema arguments that exist (if changes are needed at all).
  • Replace any use of lsst.pipe.base.ShimButler in the run method with native Gen3 butler calls.
  • Search for the string "DataRef" in docs, comments, and type annotations.  Replace it with one of the following:
    • lsst.daf.butler.DeferredDatasetHandle to refer to what the run method is given for a connection with deferLoad=True (this is the Gen3 object that behaves most like a Gen2 DataRef);
    • lsst.daf.butler.DatasetRef to refer to the combination of a data ID and a dataset type.
  • Deprecate (but do not immediately remove) Gen2-only configuration options.
  • Remove the associated script in bin.src.

Removing Gen2 support from obs packages

Blocked if the obs package is still used in a Gen2 integration test.  For DRP, this just means obs_subaruNeed input from AP on what they still need for Gen2.

What to change:

  • Remove the CameraMapper subclass.
  • Remove YAML mapper policy files.
  • Remove Gen2 ingestion tasks (inherits things in from lsst.pipe.tasks.ingest; Gen3 ingest framework is in lsst.obs.base)
  • Inspect all scripts in bin/bin.src and remove any that are Gen2 only.
    • Follow this procedure if Gen3 replacement is unclear.
    • Look for Python module code that exists only to implement these scripts.

Removing Gen2 support from obs_base

Blocked by the existence of Gen2 support in any concrete obs package.

(Not going to work out what to remove in detail here; that's just part of doing the work.)

Removing Gen2 support from pipe_base

Blocked by the existence of concrete CmdLineTasks in downstream packages.

(Not going to work out what to remove in detail here; that's just part of doing the work.)

Removing pipe_drivers and ctrl_pool

Blocked by  DM-29835 - Getting issue details... STATUS : one task needs to be moved from pipe_drivers to pipe_task.

I expect the removal of ctrl_pool to allow us to remove some dependencies from our conda third-party package list (anything MPI related).

Removing daf_persistence

Blocked by the existence of any lsst.daf.persistence import downstream.

  • afw has 4 files that depend on C++ daf_persistence.

Removing other Gen2-only code

Blocked if the code is still run by any Gen2 integration or multi-task unit tests (e.g. if the Gen2 CmdLineTask and Gen3 PipelineTask for a single conceptual step that calls the same underlying subtasks are distinct).

If the task/script's functionality does not have a clear Gen3 counterpart:

  • First file a ticket with label "gen2-functionality-removal" for removing it, but don't start work on it immediately.
  • Some person or group of people (TBD) will look at those tickets, and may create a new ticket to replace the functionality.
  • We will remove the label when we've decided what to do, and may make a replace-functionality ticket a blocker on the removal ticket.
  • If there is no blocker for the ticket and the "gen2-functionality-removal" label has been removed, the removal ticket may proceed.

Key Summary T Created Updated Due Assignee Reporter P Status Resolution
Loading...
Refresh

Post-removal cleanups

Moving configuration from obs_ packages to _pipe packages

When a hybrid task has had its CmdLineTask side removed, we're no longer blocked from moving its configuration content to pipeline files in _pipe packages, as per  RFC-775 - Getting issue details... STATUS .

But I'm not in a hurry to do this unless it solves some other problem, and we should be careful not to move configuration content that does actually belong in the obs_ package.  Most ISR configuration should stay where it is, as well as anything that corresponds to the set of available filters for the instrument (not the set of filters being run together for any particular processing run, which is  pipeline material).

Fixing up reference catalog loaders

There is a lot of duplication between the Gen2 and Gen3 code paths that we'll hopefully be able to remove.  But this code is super fragile and poorly tested in at least some edge cases (padding, padding, padding, in so many places!).  I suspect that changing __init__ signatures and deprecating Gen2-only configs will push us to try to remove/fix this more aggressively than we should.  When there is any doubt about preserving the Gen3 behavior, we should be willing to leave unused bits of Gen2 refcat code in place and flag them for removal in a later, more careful ticket.

Renaming things that say "gen3"

Let's wait until the Gen2 things are fully gone, and then think about whether the disruption is worth it in each case.  I don't currently see a reason to prioritize this.


  • No labels