Updated code documentation (#290)

* Updated installation instructions for Murfey to reflect recent setup modernisation work.
* Incremental addition of comments and descriptions to functions and variables.

Author: tieneupin

README.md

@@ -14,21 +14,22 @@ Murfey, the package, is named after [Eliza Murfey, the inventor](https://nationa
 ### How do I set up a development environment?
 
 We suggest you start with your favourite virtual environment (mamba/conda/python virtualenv/...),
-then install the dependencies listed in `requirements_dev.txt` with eg.
+then install using the following command.
+
+#### From Git
 
 ```bash
 $ git clone [email protected]:DiamondLightSource/python-murfey.git
 $ cd python-murfey
-$ pip install -r requirements_dev.txt
-$ pip install -e .[client,server]
+$ pip install -e .[client,server,developer]
 ```
 
-You will also want to set up pre-commits:
+The packages included under the `[developer]` installation key contain some helpful tools to aid you with developing Murfey further:
 
-```bash
-$ pip install pre-commit
-$ pre-commit install
-```
+- `ipykernel` - Enables interactive code development via Jupyter Notebooks.
+- `pre-commit` - Allows for the installation and running of hooks to help with linting, formatting, and type checking your code.
+- `pytest` - Used in conjunction with test functions to evaluate the reliability of your code.
+- `bump2version` - A nice little script to simplify version control.
 
 Finally, you may want to set up an ISPyB mock database server and a Zocalo
 development environment. The instructions for this are out of scope here.

src/murfey/bootstrap/__main__.py

@@ -10,16 +10,19 @@
 from urllib.parse import urlparse
 from urllib.request import urlopen
 
-# A script to simplify installing Murfey on a network-isolated machine.
-# This could in theory be invoked by
-#   python -m murfey.bootstrap
-# but then you would already have murfey installed, so what is the point.
-# More commonly this file will be run directly from a wheel with
-#   python murfey.whl/murfey/bootstrap
-# In this constellation you can not import any other files from the murfey
-# package. If you absolutely have to do this then look at the pip package
-# how this can be achieved. Also note that only standard library imports
-# will be available at that installation stage.
+"""
+A script to simplify installing Murfey on a network-isolated machine.
+This could in theory be invoked by
+  `python -m murfey.bootstrap`
+but then you would already have murfey installed, so what is the point.
+
+More commonly, this file will be run directly from a wheel with
+  `python murfey.whl/murfey/bootstrap`
+In this constellation, you cannot import any other files from the murfey package.
+If you absolutely have to do this then look at the pip package for how this can be
+achieved. Also note that only standard library imports will be available at that
+installation stage.
+"""
 
 
 def _download_to_file(url: str, outfile: str):
@@ -48,6 +51,7 @@ def _download_to_file(url: str, outfile: str):
         )
 
 
+# Main script block
 if __name__ == "__main__":
     parser = argparse.ArgumentParser("murfey.bootstrap")
     parser.add_argument("-?", action="help", help=argparse.SUPPRESS)
@@ -56,26 +60,31 @@ def _download_to_file(url: str, outfile: str):
     )
     args = parser.parse_args()
 
-    # Validate the passed server address and construct a minimal base path string and
-    # extract the host name for pip installation purposes
+    # Validate the passed server address
+    # Construct a minimal base path string
+    # Extract the host name for pip installation purposes
     try:
         murfey_url = urlparse(args.server)
     except Exception:
         exit(f"{args.server} is not a valid URL")
     murfey_base = f"{murfey_url.scheme}://{murfey_url.netloc}"
     murfey_hostname = murfey_url.netloc.split(":")[0]
 
+    # Check that Python version is supported
     print(f"Python version: {sys.version_info.major}.{sys.version_info.minor}")
-    if sys.hexversion < 0x3080000:
+    # if sys.hexversion < 0x3080000:
+    if sys.version_info >= (3, 9):  # Use version_info tuple instead
         exit(
             "Your python version is too old to support Murfey. "
-            "You need at least Python 3.8"
+            "You need at least Python 3.9"
         )
 
+    # Step 1: Download pip wheel
     print()
     print(f"1/4 -- Connecting to murfey server on {murfey_base}...")
     _download_to_file(f"{murfey_base}/bootstrap/pip.whl", "pip.whl")
 
+    # Step 2: Get pip to install itself
     print()
     print("2/4 -- Bootstrapping pip")
     python = sys.executable
@@ -95,6 +104,7 @@ def _download_to_file(url: str, outfile: str):
         exit("Could not bootstrap pip")
     os.remove("pip.whl")
 
+    # Step 3: Update pip
     print()
     print("3/4 -- Updating pip")
     python = sys.executable
@@ -114,6 +124,7 @@ def _download_to_file(url: str, outfile: str):
     if result.returncode:
         exit("Could not update pip")
 
+    # Step 4: pip install murfey
     print()
     print("4/4 -- Installing murfey client")
     result = subprocess.run(
@@ -131,6 +142,7 @@ def _download_to_file(url: str, outfile: str):
     if result.returncode:
         exit("Could not install murfey client")
 
+    # Write config file
     print()
     print("Installation completed.")
     config = configparser.ConfigParser()

src/murfey/client/analyser.py

@@ -71,6 +71,9 @@ def __init__(
         )
 
     def _find_extension(self, file_path: Path):
+        """
+        Identifies the file extension and stores that information in the class.
+        """
         if (
             required_substrings := self._murfey_config.get(
                 "data_required_substrings", {}
@@ -81,28 +84,40 @@ def _find_extension(self, file_path: Path):
             if not any(r in file_path.name for r in required_substrings):
                 return []
 
+        # Checks for MRC, TIFF, TIF, and EER files if no extension has been defined
         if (
             file_path.suffix in (".mrc", ".tiff", ".tif", ".eer")
             and not self._extension
         ):
             logger.info(f"File extension determined: {file_path.suffix}")
             self._extension = file_path.suffix
+        # Check for TIFF, TIF, or EER if the file's already been assigned an extension
         elif (
             file_path.suffix in (".tiff", ".tif", ".eer")
             and self._extension != file_path.suffix
         ):
             logger.info(f"File extension re-evaluated: {file_path.suffix}")
             self._extension = file_path.suffix
+        # Check for LIF files separately
         elif file_path.suffix == ".lif":
             self._extension = file_path.suffix
 
     def _find_context(self, file_path: Path) -> bool:
+        """
+        Using various conditionals, identifies what workflow the file is part of, and
+        assigns the necessary context class to it for subsequent stages of processing
+        """
+
+        # CLEM workflow check
+        # Look for LIF files
         if file_path.suffix == ".lif":
             self._role = "detector"
             self._context = CLEMContext("leica", self._basepath)
             return True
+
         split_file_name = file_path.name.split("_")
         if split_file_name:
+            # Files starting with "FoilHole" belong to the SPA workflow
             if split_file_name[0].startswith("FoilHole"):
                 if not self._context:
                     logger.info("Acquisition software: EPU")
@@ -123,9 +138,12 @@ def _find_context(self, file_path: Path) -> bool:
                         else SPAContext("epu", self._basepath)
                     )
                 self.parameters_model = ProcessingParametersSPA
+                # Assign it the detector attribute if not already present
                 if not self._role:
                     self._role = "detector"
                 return True
+
+            # Files starting with "Position" belong to the standard tomography workflow
             if (
                 split_file_name[0] == "Position"
                 or "[" in file_path.name
@@ -136,25 +154,34 @@ def _find_context(self, file_path: Path) -> bool:
                     logger.info("Acquisition software: tomo")
                     self._context = TomographyContext("tomo", self._basepath)
                     self.parameters_model = PreprocessingParametersTomo
+                # Assign role if not already present
                 if not self._role:
+                    # Fractions files attributed to the detector
                     if (
                         "Fractions" in split_file_name[-1]
                         or "fractions" in split_file_name[-1]
                     ):
                         self._role = "detector"
+                    # MDOC files attributed to the microscope
                     elif (
                         file_path.suffix == ".mdoc"
                         or file_path.with_suffix(".mdoc").is_file()
                     ):
                         self._role = "microscope"
+                    # Attribute all other files to the detector
                     else:
                         self._role = "detector"
                 return True
+
+            # Files with these suffixes belong to the serial EM tomography workflow
             if file_path.suffix in (".mrc", ".tiff", ".tif", ".eer"):
+                # Ignore batch files and search maps
                 if any(p in file_path.parts for p in ("Batch", "SearchMaps")):
                     return False
+                # Ignore JPG files
                 if file_path.with_suffix(".jpg").is_file():
                     return False
+                # Ignore the averaged movies written out by the Falcon
                 if (
                     len(
                         list(
@@ -165,7 +192,6 @@ def _find_context(self, file_path: Path) -> bool:
                     )
                     > 1
                 ):
-                    # This covers the case of ignoring the averaged movies written out by the Falcon
                     return False
                 self._context = TomographyContext("serialem", self._basepath)
                 self.parameters_model = PreprocessingParametersTomo

src/murfey/client/contexts/clem.py

@@ -23,6 +23,9 @@
 def _file_transferred_to(
     environment: MurfeyInstanceEnvironment, source: Path, file_path: Path
 ) -> Optional[Path]:
+    """
+    Returns the Path of the transferred file on the DLS file system
+    """
     machine_config = get_machine_config(
         str(environment.url.geturl()), demo=environment.demo
     )
@@ -38,6 +41,9 @@ def _file_transferred_to(
 def _get_source(
     file_path: Path, environment: MurfeyInstanceEnvironment
 ) -> Optional[Path]:
+    """
+    Returns the Path of the file on the client PC
+    """
     for s in environment.sources:
         if file_path.is_relative_to(s):
             return s
@@ -87,16 +93,13 @@ def post_transfer(
                 file_path=transferred_file,
             )
 
-            # Get the file size and timestamp from transferred_file
-            # Client PC cannot see file_path; that is for server PC
-
             # Post the message and logs it if there's an error
             capture_post(
                 url,
                 json={
                     "name": str(file_path),
-                    "size": transferred_file.stat().st_size,
-                    "timestamp": transferred_file.stat().st_ctime,
+                    "size": transferred_file.stat().st_size,  # File size, in bytes
+                    "timestamp": transferred_file.stat().st_ctime,  # For Unix systems, shows last metadata change
                     "description": "",
                 },
             )

src/murfey/client/tui/screens.py

@@ -766,20 +766,27 @@ def on_button_pressed(self, event: Button.Pressed):
             f"{self.app._environment.url.geturl()}/machine/"
         ).json()
         if machine_data.get("upstream_data_download_directory"):
+            # Create the directory locally to save files to
             download_dir = Path(machine_data["upstream_data_download_directory"]) / str(
                 event.button.label
             )
             download_dir.mkdir(exist_ok=True)
+
+            # Get the paths to the TIFF files generated previously under the same session ID
             upstream_tiff_paths_response = requests.get(
                 f"{self.app._environment.url.geturl()}/visits/{event.button.label}/upstream_tiff_paths"
             )
             upstream_tiff_paths = upstream_tiff_paths_response.json() or []
+
+            # Request to download the TIFF files found
             for tp in upstream_tiff_paths:
                 (download_dir / tp).parent.mkdir(exist_ok=True, parents=True)
+                # Write TIFF to the specified file path
                 stream_response = requests.get(
                     f"{self.app._environment.url.geturl()}/visits/{event.button.label}/upstream_tiff/{tp}",
                     stream=True,
                 )
+                # Write the file chunk-by-chunk to avoid hogging memory
                 with open(download_dir / tp, "wb") as utiff:
                     for chunk in stream_response.iter_content(chunk_size=32 * 1024**2):
                         utiff.write(chunk)

src/murfey/server/api.py

@@ -1597,7 +1597,9 @@ def failed_client_post(post_info: PostInfo):
 @router.get("/visits/{visit_name}/upstream_visits")
 async def find_upstream_visits(visit_name: str):
     upstream_visits = {}
+    # Iterates through provided upstream directories
     for p in machine_config.upstream_data_directories:
+        # Looks for visit name in file path
         for v in Path(p).glob(f"{visit_name.split('-')[0]}-*"):
             upstream_visits[v.name] = v / machine_config.processed_directory_name
     return upstream_visits
@@ -1620,6 +1622,10 @@ def _get_upstream_tiff_dirs(visit_name: str) -> List[Path]:
 
 @router.get("/visits/{visit_name}/upstream_tiff_paths")
 async def gather_upstream_tiffs(visit_name: str):
+    """
+    Looks for TIFF files associated with the current session in the permitted storage
+    servers, and returns their relative file paths as a list.
+    """
     upstream_tiff_paths = []
     tiff_dirs = _get_upstream_tiff_dirs(visit_name)
     if not tiff_dirs:

src/murfey/server/config.py

@@ -41,9 +41,11 @@ class MachineConfig(BaseModel):
     processed_extra_directory: str = ""
     plugin_packages: Dict[str, Path] = {}
     software_settings_output_directories: Dict[str, List[str]] = {}
-    upstream_data_directories: List[Path] = []
-    upstream_data_download_directory: Optional[Path] = None
-    upstream_data_tiff_locations: List[str] = ["processed"]
+
+    # Find and download upstream directories
+    upstream_data_directories: List[Path] = []  # Previous sessions
+    upstream_data_download_directory: Optional[Path] = None  # Set by microscope config
+    upstream_data_tiff_locations: List[str] = ["processed"]  # Location of CLEM TIFFs
     failure_queue: str = ""