search

desihub / desispec

DESI spectral pipeline
BSD 3-Clause "New" or "Revised" License
35 stars 24 forks source link

Group arguments for maintainability of workflow functions #2021

Open marcelo-alvarez opened 1 year ago

marcelo-alvarez commented 1 year ago

The structure of several desispec.workflow functions taken together is such that adding a single new parameter requires modification of multiple functions in identical ways, copying arguments and the corresponding docstrings in a sequence of nested calls. There is also a significant overlap between function arguments and parameters contained in argparse.ArgumentParser objects.

One solution could be to group pipeline parameters together by the processes they are relevant to, and document them once in the, e.g., class that defines their associations (making use of inheritance for parameters that are relevant to multiple pipeline processes).

This can be accomplished without changing the command-line arguments to scripts like desi_proc, desi_proc_tilenight, nor to the calling sequence of desispec.workflow functions such as submit_night --> submit_tilenight_and_redshifts --> submit_tilenight --> create_and_submit --> create_batch_script --> create_desi_proc_tilenight_batch_script. Should this calling sequence need to be simplified and/or refactored at some point in the future, however, having the pipeline parameters grouped into well defined abstract classes with a clear relationship to user-supplied (i.e. command-line) and data-specific (i.e. exposure properties) parameters would make this much easier, enhancing the overall maintainability of desispec.workflow functions.

@sbailey @akremin

akremin commented 1 year ago

I fully agree. I have considered/scoped this out many times in the past and think it is an important thing to do as part of the script merger+refactor that will take place after Perlmutter becomes the only NERSC machine. Until then I think we should not pursue this so as not to introduce new bugs into daily ops. For now, I think we should prioritize stability over changes.

marcelo-alvarez commented 1 year ago

@akremin thanks, this is an excellent point. In addressing #1908, which I am returning to now and needs to happen before the transition, I will limit any changes to argument grouping that I consider to functions that I introduced in #1802 (which likely would have prevented some bugs I'm fixing now). This should minimize the likelihood of introducing new bugs into daily ops while also maximizing stability. I agree with you that more sweeping changes can wait until after the transition, as part of the script merger+refactor.