diff --git a/doc/changes/DM-52898.other.rst b/doc/changes/DM-52898.other.rst new file mode 100644 index 00000000..bd9b6be1 --- /dev/null +++ b/doc/changes/DM-52898.other.rst @@ -0,0 +1 @@ +Modified the BPS report driver so it compiles exit code summary only when necessary, i.e., when ``--return-exit-codes`` option was used with ``bps report``. diff --git a/python/lsst/ctrl/bps/drivers.py b/python/lsst/ctrl/bps/drivers.py index 53da0b8b..8debefdc 100644 --- a/python/lsst/ctrl/bps/drivers.py +++ b/python/lsst/ctrl/bps/drivers.py @@ -55,6 +55,7 @@ from lsst.utils.usage import get_peak_mem_usage from . import BPS_DEFAULTS, BPS_SEARCH_ORDER, DEFAULT_MEM_FMT, DEFAULT_MEM_UNIT, BpsConfig +from .bps_reports import compile_code_summary, compile_job_summary from .bps_utils import _dump_env_info, _dump_pkg_info, _make_id_link from .cancel import cancel from .construct import construct @@ -68,7 +69,7 @@ from .ping import ping from .pre_transform import acquire_quantum_graph, cluster_quanta from .prepare import prepare -from .report import BPS_POSTPROCESSORS, display_report, retrieve_report +from .report import display_report, retrieve_report from .restart import restart from .status import status from .submit import submit @@ -400,20 +401,31 @@ def restart_driver(wms_service, run_id): print("Restart failed: Unknown error") -def report_driver(wms_service, run_id, user, hist_days, pass_thru, is_global=False, return_exit_codes=False): - """Print out summary of jobs submitted for execution. +def report_driver( + wms_service: str | None = None, + run_id: str | None = None, + user: str | None = None, + hist_days: float = 0.0, + pass_thru: str | None = None, + is_global: bool = False, + return_exit_codes: bool = False, +): + """Print out the summary of jobs submitted for execution. Parameters ---------- - wms_service : `str` + wms_service : `str`, optional Name of the class. - run_id : `str` + run_id : `str`, optional A run id the report will be restricted to. - user : `str` + user : `str`, optional A user the report will be restricted to. - hist_days : `float` - Number of days. - pass_thru : `str` + hist_days : `float`, optional + Number of past days to consider while preparing the report. By default, + only the currently running workflows are included in the report. + If the report is restricted to a single run (i.e., ``run_id`` is set), + the history search will be limited by default to two past days. + pass_thru : `str`, optional A string to pass directly to the WMS service class. is_global : `bool`, optional If set, all available job queues will be queried for job information. @@ -430,17 +442,19 @@ def report_driver(wms_service, run_id, user, hist_days, pass_thru, is_global=Fal Only applicable in the context of a WMS with associated handlers to return exit codes from jobs. """ - if wms_service is None: + if not wms_service: default_config = BpsConfig(BPS_DEFAULTS) wms_service = os.environ.get("BPS_WMS_SERVICE_CLASS", default_config["wmsServiceClass"]) - # When reporting on single run: - # * increase history until better mechanism for handling completed jobs is - # available. + # When reporting on a single run: + # * increase history until a better mechanism for handling completed jobs + # is available. # * massage the retrieved reports using BPS report postprocessors. if run_id: hist_days = max(hist_days, 2) - postprocessors = BPS_POSTPROCESSORS + postprocessors = [compile_job_summary] + if return_exit_codes: + postprocessors.append(compile_code_summary) else: postprocessors = None diff --git a/python/lsst/ctrl/bps/report.py b/python/lsst/ctrl/bps/report.py index 79fc4cf2..b9275a0c 100644 --- a/python/lsst/ctrl/bps/report.py +++ b/python/lsst/ctrl/bps/report.py @@ -31,7 +31,7 @@ oriented database tables. """ -__all__ = ["BPS_POSTPROCESSORS", "display_report", "retrieve_report"] +__all__ = ["display_report", "retrieve_report"] import logging import sys @@ -40,20 +40,9 @@ from lsst.utils import doImportType -from .bps_reports import ( - DetailedRunReport, - ExitCodesReport, - SummaryRunReport, - compile_code_summary, - compile_job_summary, -) +from .bps_reports import DetailedRunReport, ExitCodesReport, SummaryRunReport from .wms_service import BaseWmsService, WmsRunReport, WmsStates -BPS_POSTPROCESSORS = (compile_job_summary, compile_code_summary) -"""Postprocessors for massaging run reports -(`tuple` [`Callable` [[`WmsRunReport`], None]). -""" - _LOG = logging.getLogger(__name__) @@ -167,7 +156,7 @@ def retrieve_report( pass_thru: str | None = None, is_global: bool = False, return_exit_codes: bool = False, - postprocessors: Sequence[Callable[[WmsRunReport], None]] | None = None, + postprocessors: Sequence[Callable[[WmsRunReport], list[str]]] | None = None, ) -> tuple[list[WmsRunReport], list[str]]: """Retrieve summary of jobs submitted for execution. diff --git a/tests/test_drivers.py b/tests/test_drivers.py index b94860d5..df851e2d 100644 --- a/tests/test_drivers.py +++ b/tests/test_drivers.py @@ -34,8 +34,9 @@ import yaml -from lsst.ctrl.bps import WmsStates -from lsst.ctrl.bps.drivers import _init_submission_driver, ping_driver, status_driver +from lsst.ctrl.bps import WmsRunReport, WmsStates +from lsst.ctrl.bps.bps_reports import compile_code_summary, compile_job_summary +from lsst.ctrl.bps.drivers import _init_submission_driver, ping_driver, report_driver, status_driver TESTDIR = os.path.abspath(os.path.dirname(__file__)) @@ -142,5 +143,187 @@ def testWmsServiceNone(self): self.assertEqual(retval, WmsStates.RUNNING.value) +class TestReportDriver(unittest.TestCase): + """Test report_driver function.""" + + @unittest.mock.patch( + "lsst.ctrl.bps.drivers.BPS_DEFAULTS", new={"wmsServiceClass": "wms_test_utils.WmsServiceSuccess"} + ) + def testWmsServiceFromDefaults(self): + # Should not raise an exception and use default from BPS_DEFAULTS. + with unittest.mock.patch.dict(os.environ, {}, clear=True): + report_driver( + wms_service=None, + run_id=None, + user=None, + hist_days=0, + pass_thru=None, + ) + + def testWmsServiceFromEnvVar(self): + # Should not raise an exception. + with unittest.mock.patch.dict( + os.environ, {"BPS_WMS_SERVICE_CLASS": "wms_test_utils.WmsServiceSuccess"} + ): + report_driver( + wms_service=None, + run_id=None, + user=None, + hist_days=0.0, + pass_thru=None, + ) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testHistDefault(self, mock_display, mock_retrieve): + mock_retrieve.return_value = ([], []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id="123", + user=None, + hist_days=0.0, + pass_thru=None, + ) + + # Verify retrieve_report was called with the default hist setting. + _, kwargs = mock_retrieve.call_args + self.assertAlmostEqual(kwargs["hist"], 2.0) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testHistCustom(self, mock_display, mock_retrieve): + mock_retrieve.return_value = ([], []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id="123", + user=None, + hist_days=4.0, + pass_thru=None, + ) + + # Verify retrieve_report was called with a custom hist setting. + _, kwargs = mock_retrieve.call_args + self.assertAlmostEqual(kwargs["hist"], 4.0) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testPostprocessorsWithoutExitCodes(self, mock_display, mock_retrieve): + mock_retrieve.return_value = ([], []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id="123", + user=None, + hist_days=0.0, + pass_thru=None, + return_exit_codes=False, + ) + + # Verify the postprocessors list contains only one postprocessor. + args, kwargs = mock_retrieve.call_args + self.assertEqual(len(kwargs["postprocessors"]), 1) + self.assertIn(compile_job_summary, kwargs["postprocessors"]) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testPostprocessorsWithExitCodes(self, mock_display, mock_retrieve): + mock_retrieve.return_value = ([], []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id="123", + user=None, + hist_days=0.0, + pass_thru=None, + return_exit_codes=True, + ) + + # Verify the postprocessors list contains both postprocessors. + _, kwargs = mock_retrieve.call_args + self.assertEqual(len(kwargs["postprocessors"]), 2) + self.assertIn(compile_code_summary, kwargs["postprocessors"]) + self.assertIn(compile_job_summary, kwargs["postprocessors"]) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testPostprocessorsNoRunId(self, mock_display, mock_retrieve): + mock_retrieve.return_value = ([], []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id=None, + user=None, + hist_days=0.0, + pass_thru=None, + ) + + # Verify postprocessors contains compile_job_summary + _, kwargs = mock_retrieve.call_args + self.assertIsNone(kwargs["postprocessors"]) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testDisplayCalledIfRuns(self, mock_display, mock_retrieve): + mock_runs = [WmsRunReport(wms_id="1", state=WmsStates.SUCCEEDED)] + mock_retrieve.return_value = (mock_runs, []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id=None, + user=None, + hist_days=0, + pass_thru=None, + ) + + # Verify display_report was called with the runs + mock_display.assert_called_once() + args, kwargs = mock_display.call_args + self.assertEqual(args[0], mock_runs) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + def testDisplayCalledIfMessages(self, mock_display, mock_retrieve): + mock_messages = ["Warning message 1", "Warning message 2"] + mock_retrieve.return_value = ([], mock_messages) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id=None, + user=None, + hist_days=0, + pass_thru=None, + ) + + # Verify display_report was called with messages + mock_display.assert_called_once() + args, kwargs = mock_display.call_args + self.assertEqual(args[1], mock_messages) + + @unittest.mock.patch("lsst.ctrl.bps.drivers.retrieve_report") + @unittest.mock.patch("lsst.ctrl.bps.drivers.display_report") + @unittest.mock.patch("builtins.print") + def testNoRecordsFoundMessage(self, mock_print, mock_display, mock_retrieve): + mock_retrieve.return_value = ([], []) + + report_driver( + wms_service="wms_test_utils.WmsServiceSuccess", + run_id="123", + user=None, + hist_days=1.5, + pass_thru=None, + ) + + # Verify display_report() was NOT called. + mock_display.assert_not_called() + + # Verify that a helpful message was printed. + mock_print.assert_called_once() + call_args = mock_print.call_args[0][0] + self.assertIn("No records found", call_args) + self.assertIn("123", call_args) + + if __name__ == "__main__": unittest.main()