Refactor imap_processing.cli.ProcessInstrument (IMAP-Science-Operatio…

…ns-Center#550)

* Refactor ProcessInstrument class to have final processing method that calls three overridable methods as part of the process workflow.

* add test_cli.py which contains test coverage for imap_processing.cli.py

* Apply docstring formatting suggestions from code review

Co-authored-by: Matthew Bourque <[email protected]>

* CI: Add codecov upload token (IMAP-Science-Operations-Center#554)

* Add launch_time parameter to cdf.utils.calc_start_time (IMAP-Science-Operations-Center#552)

* Added optional launch_time parameter to calc_start_time so instruments can define their own launch time
* Expanded calc_start_time test to use new launch_time parameter

* Add a DOI badge to the README (IMAP-Science-Operations-Center#555)

* Modify docstrings to use 80 character standard width

---------

Co-authored-by: Matthew Bourque <[email protected]>
Co-authored-by: Greg Lucas <[email protected]>
Co-authored-by: Veronica Martinez <[email protected]>

imap_processing/cli.py

@@ -16,6 +16,7 @@
 from abc import ABC, abstractmethod
 from json import loads
 from pathlib import Path
+from typing import final
 from urllib.error import HTTPError
 
 import imap_data_access
@@ -31,6 +32,7 @@
 # In code:
 #   call cdf.utils.write_cdf
 from imap_processing.cdf.utils import load_cdf, write_cdf
+from imap_processing.hi.l1a import hi_l1a
 from imap_processing.mag.l1a.mag_l1a import mag_l1a
 from imap_processing.swe.l1a.swe_l1a import swe_l1a
 from imap_processing.swe.l1b.swe_l1b import swe_l1b
@@ -241,67 +243,159 @@ def download_dependencies(self):
             file_list.append(imap_data_access.download(return_query[0]["file_path"]))
         return file_list
 
-    @abstractmethod
+    def upload_products(self, products: list[str]):
+        """
+        Upload data products to the IMAP SDC.
+
+        Attributes
+        ----------
+        products : list[str]
+        A list of file paths to upload to the SDC.
+        """
+        if self.upload_to_sdc:
+            if len(products) == 0:
+                logger.info("No files to upload.")
+            for filename in products:
+                logger.info(f"Uploading file: {filename}")
+                imap_data_access.upload(filename)
+
+    @final
     def process(self):
-        """Perform instrument specific processing."""
+        """
+        Run the processing workflow and cannot be overridden by subclasses.
+
+        Each IMAP processing step consists of three steps:
+        1. Pre-processing actions such as downloading dependencies for processing.
+        2. Do the data processing. The result of this step will usually be a list
+        of new products (files).
+        3. Post-processing actions such as uploading files to the IMAP SDC.
+        """
+        dependencies = self.pre_processing()
+        products = self.do_processing(dependencies)
+        self.post_processing(products)
+
+    def pre_processing(self):
+        """
+        Complete pre-processing.
+
+        For this baseclass, pre-processing consists of downloading dependencies
+        for processing. Child classes can override this method to customize the
+        pre-processing actions.
+
+        Returns
+        -------
+        List of dependencies downloaded from the IMAP SDC.
+        """
+        return self.download_dependencies()
+
+    @abstractmethod
+    def do_processing(self, dependencies: list):
+        """
+        Abstract method that processes the IMAP processing steps.
+
+        All child classes must implement this method. Input and outputs are
+        typically lists of file paths but are free to any list.
+
+        Attributes
+        ----------
+        dependencies : list
+            List of dependencies to process
+
+        Returns
+        -------
+        list
+            List of products produced
+        """
         raise NotImplementedError
 
+    def post_processing(self, products: list[str]):
+        """
+        Complete post-processing.
+
+        Default post-processing consists of uploading newly generated products
+        to the IMAP SDC. Child classes can override this method to customize the
+        post-processing actions.
+
+        Attributes
+        ----------
+        products : list[str]
+            A list of file paths (products) produced by do_processing method.
+        """
+        self.upload_products(products)
+
 
 class Codice(ProcessInstrument):
     """Process CoDICE."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform CoDICE specific processing."""
         print(f"Processing CoDICE {self.data_level}")
 
 
 class Glows(ProcessInstrument):
     """Process GLOWS."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform GLOWS specific processing."""
         print(f"Processing GLOWS {self.data_level}")
 
 
 class Hi(ProcessInstrument):
     """Process IMAP-Hi."""
 
-    def process(self):
-        """Perform IMAP-Hi specific processing."""
+    def do_processing(self, dependencies: list):
+        """
+        Perform IMAP-Hi specific processing.
+
+        Attributes
+        ----------
+        dependencies: list
+        List of dependencies to process
+        """
         print(f"Processing IMAP-Hi {self.data_level}")
 
+        if self.data_level == "l1a":
+            # File path is expected output file path
+            if len(dependencies) > 1:
+                raise ValueError(
+                    f"Unexpected dependencies found for Hi L1A:"
+                    f"{dependencies}. Expected only one dependency."
+                )
+            datasets = hi_l1a.hi_l1a(dependencies[0])
+            products = [write_cdf(dataset) for dataset in datasets]
+            return products
+
 
 class Hit(ProcessInstrument):
     """Process HIT."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform HIT specific processing."""
         print(f"Processing HIT {self.data_level}")
 
 
 class Idex(ProcessInstrument):
     """Process IDEX."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform IDEX specific processing."""
         print(f"Processing IDEX {self.data_level}")
 
 
 class Lo(ProcessInstrument):
     """Process IMAP-Lo."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform IMAP-Lo specific processing."""
         print(f"Processing IMAP-Lo {self.data_level}")
 
 
 class Mag(ProcessInstrument):
     """Process MAG."""
 
-    def process(self):
+    def do_processing(self, file_paths):
         """Perform MAG specific processing."""
         print(f"Processing MAG {self.data_level}")
-        file_paths = self.download_dependencies()
 
         if self.data_level == "l1a":
             # File path is expected output file path
@@ -311,43 +405,35 @@ def process(self):
                     f"{file_paths}. Expected only one dependency."
                 )
             output_files = mag_l1a(file_paths[0], data_version=self.version)
-            if self.upload_to_sdc:
-                if len(output_files) == 0:
-                    print("No files to upload.")
-                for filename in output_files:
-                    print(f"Uploading file: {filename}")
-                    imap_data_access.upload(filename)
+            return output_files
 
 
 class Swapi(ProcessInstrument):
     """Process SWAPI."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform SWAPI specific processing."""
         print(f"Processing SWAPI {self.data_level}")
 
 
 class Swe(ProcessInstrument):
     """Process SWE."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform SWE specific processing."""
         # self.file_path example:
         # imap/swe/l1a/2023/09/imap_swe_l1a_sci_20230927_v001.cdf
-        dependencies = self.download_dependencies()
         print(f"Processing SWE {self.data_level}")
 
         # TODO: currently assumes just the first path returned is the one to use
 
+        products = []
         if self.data_level == "l1a":
             processed_data = swe_l1a(Path(dependencies[0]))
             for data in processed_data:
                 cdf_file_path = write_cdf(data)
                 print(f"processed file path: {cdf_file_path}")
-
-                if self.upload_to_sdc:
-                    imap_data_access.upload(cdf_file_path)
-                    print(f"Uploading file: {cdf_file_path}")
+                products.append(cdf_file_path)
 
         elif self.data_level == "l1b":
             # read CDF file
@@ -356,9 +442,7 @@ def process(self):
             # TODO: Pass in the proper version and descriptor
             cdf_file_path = write_cdf(data=processed_data)
             print(f"processed file path: {cdf_file_path}")
-            if self.upload_to_sdc:
-                imap_data_access.upload(cdf_file_path)
-                print(f"Uploading file: {cdf_file_path}")
+            products.append(cdf_file_path)
 
         else:
             print("No code to process this level")
@@ -367,7 +451,7 @@ def process(self):
 class Ultra(ProcessInstrument):
     """Process IMAP-Ultra."""
 
-    def process(self):
+    def do_processing(self, dependencies):
         """Perform IMAP-Ultra specific processing."""
         print(f"Processing IMAP-Ultra {self.data_level}")
 

imap_processing/tests/test_cli.py

@@ -0,0 +1,89 @@
+"""Tests coverage for imap_processing.cli."""
+
+import sys
+from unittest import mock
+
+import pytest
+
+from imap_processing.cli import Hi, _validate_args, main
+
+
+@mock.patch("imap_processing.cli.Mag")
+def test_main(mock_instrument):
+    """Test imap_processing.cli.main()"""
+    test_args = [
+        "imap_cli",
+        "--instrument",
+        "mag",
+        "--dependency",
+        (
+            '[{"instrument": "mag", '
+            '"data_level": "l0", '
+            '"descriptor": "sci", '
+            '"version": "v001", '
+            '"start_date": "20240430"}]'
+        ),
+        "--data-level",
+        "l1a",
+        "--start-date",
+        "20240430",
+        "--end-date",
+        "20240501",
+        "--version",
+        "v00-01",
+        "--upload-to-sdc",
+    ]
+    with mock.patch.object(sys, "argv", test_args):
+        # Running without raising an exception is a pass.
+        # No asserts needed.
+        main()
+
+
+@pytest.mark.parametrize(
+    "instrument, data_level, raises_value_error",
+    [
+        ("mag", "l0", ""),
+        ("foo", "l0", "foo is not in the supported .*"),
+        ("codice", "l1z", "l1z is not a supported .*"),
+    ],
+)
+def test_validate_args(instrument, data_level, raises_value_error):
+    """Test coverage for imap_processing.cli._validate_args()"""
+    args = mock.Mock
+    args.instrument = instrument
+    args.data_level = data_level
+
+    if raises_value_error:
+        with pytest.raises(ValueError, match=raises_value_error):
+            _validate_args(args)
+    else:
+        _validate_args(args)
+
+
+@mock.patch("imap_processing.cli.imap_data_access.query")
+@mock.patch("imap_processing.cli.imap_data_access.download")
+@mock.patch("imap_processing.cli.imap_data_access.upload")
+@mock.patch("imap_processing.cli.hi_l1a.hi_l1a")
+@mock.patch("imap_processing.cli.write_cdf")
+def test_hi(mock_write_cdf, mock_hi_l1a, mock_upload, mock_download, mock_query):
+    """Test coverage for cli.Hi class"""
+    mock_query.return_value = [{"file_path": "/path/to/file0"}]
+    mock_download.return_value = "file0"
+    mock_hi_l1a.return_value = ["l1a_file0", "l1a_file1"]
+    mock_write_cdf.side_effect = ["/path/to/file0", "/path/to/file1"]
+
+    dependency_str = (
+        "[{"
+        "'instrument': 'lo',"
+        "'data_level': 'l0',"
+        "'descriptor': 'sci',"
+        "'version': 'v00-01',"
+        "'start_date': '20231212'"
+        "}]"
+    )
+    instrument = Hi("l1a", dependency_str, "20231212", "20231213", "v005", True)
+    instrument.process()
+    assert mock_query.call_count == 1
+    assert mock_download.call_count == 1
+    assert mock_hi_l1a.call_count == 1
+    assert mock_upload.call_count == 2