Test config

Author: rowanwalker96

src/dlstbx/services/trigger_xchem.py

@@ -898,7 +898,7 @@ def trigger_hitidentification(
             return {"success": True}
         comparator_threshold = config.resolve("comparator_threshold", parameters)
         pipedream = config.resolve("pipedream", parameters)
-        pandda = config.resolve("run_pandda", parameters, "pandda")
+        pandda = parameters.pandda
 
         # Industrial proposals never run Pipedream, whatever the visit asks for
         if (
@@ -1078,9 +1078,8 @@ def trigger_xchem_collate(
         - automatic: boolean passed to ProcessingJob.automatic
 
         The visit's config file (see dlstbx.util.xchem_config) supplies
-        `pipedream` if the recipe did not, names the `notify` mail recipients in
-        place of the visit's ISPyB Team Leader, and can stop the visit with
-        `enabled: false`.
+        `pipedream` if the recipe did not, its top-level `notify` names the mail
+        recipient, and `enabled:false` stops processing.
         Example recipe parameters:
         { "target": "xchem_collate",
             "dcid": 123456,
@@ -1256,8 +1255,7 @@ def trigger_xchem_collate(
         if notify_email:
             self.log.info(f"Notifying {notify_email} from the config for {visit}")
         else:
-            notify_email = [get_visit_team_leader_email(visit, session) or ""]
-            notify_email = ["qvu59474@diamond.ac.uk"]
+            notify_email = get_visit_team_leader_email(visit, session) or ""
 
         analysis_dir = self._resolve_analysis_dir(xchem_visit_dir)
         recipe_parameters = {
@@ -1268,7 +1266,7 @@ def trigger_xchem_collate(
             "scaling_id": scaling_id,
             "pipedream": pipedream,
             "overwrite": overwrite,
-            "notify_email": ",".join(notify_email),
+            "notify_email": notify_email,
         }
         # Upsert on max dcid
         self.upsert_proc(rw, max(dcids), "XChem-Collate", recipe_parameters)

src/dlstbx/util/pandda.py

@@ -200,5 +200,5 @@ def get_pandda_settings(visit_dir, logger=None):
 
     A visit need not have a config file, and one that exists may be empty.
     """
-    settings = load_visit_config(visit_dir, logger or log).pandda_args
+    settings = load_visit_config(visit_dir, logger or log).pandda or {}
     return " ".join(f"--{k}={v}" for k, v in settings.items())

src/dlstbx/util/xchem_config.py

@@ -1,18 +1,17 @@
 """Per-visit settings for the XChem autoprocessing pipelines.
 
-A labxchem visit may hold a `.config.yaml` (or the older `.user.yaml`) naming
-its target and steering how it is processed::
+A labxchem visit may hold a `.user.yaml` naming its target and steering how
+it is processed::
 
     data:
       acronym: A71EV2A            # cached by the pipeline
     autoprocessing:
       enabled: true               # process this visit at all?
       comparator_threshold: 150   # datasets PanDDA2 waits for before starting
       pipedream: false            # run Pipedream?
-      pandda:                     # false to skip PanDDA2, or a mapping of
-        high_res_lower_limit: 2.5 # extra --key=value args to run it with
-      notify:                     # who to mail when collate finishes
-        - someone@diamond.ac.uk
+      pandda:                     # extra PanDDA2 --key=value arguments
+        high_res_lower_limit: 2.5
+    notify: someone@diamond.ac.uk   # who to mail when collate finishes
 
 Every key is optional, and leaving one out is not the same as setting it false,
 so the fields below default to None to mean "the user said nothing".
@@ -22,55 +21,39 @@
 pydantic's `model_fields_set`, so a recipe hardcoding a parameter pins it for
 every visit it covers -- leave it out of the recipe for the file to have a say.
 
-The files are hand-edited, so nothing here raises on bad input: settings that
-fail to parse are logged and dropped, keeping the cached acronym either way.
+The files are hand-edited, so nothing here raises on bad input: a setting that
+fails to parse is logged and dropped on its own, leaving the rest of the file
+in force.
 """
 
 from __future__ import annotations
 
 import logging
-import re
 from pathlib import Path
 from typing import Any
 
 import pydantic
 import yaml
 
-CONFIG_FILENAMES = (".config.yaml", ".user.yaml")
+CONFIG_FILENAME = ".user.yaml"
 
 log = logging.getLogger("dlstbx.util.xchem_config")
 
 
 class VisitConfig(pydantic.BaseModel):
     """A visit's config file, flattened: `acronym` comes from the `data`
-    section, the rest from `autoprocessing`."""
+    section, `notify` is top level, the rest come from `autoprocessing`."""
 
-    model_config = pydantic.ConfigDict(extra="allow")
+    # extras are forbidden so a misspelled key is reported rather than
+    # silently ignored; load_visit_config drops it and keeps the rest
+    model_config = pydantic.ConfigDict(extra="forbid")
 
     acronym: str | None = None
     enabled: bool | None = None
     comparator_threshold: int | None = pydantic.Field(default=None, gt=0)
     pipedream: bool | None = None
-    pandda: bool | dict[str, Any] | None = None
-    notify: list[str] = pydantic.Field(default_factory=list)
-
-    @pydantic.field_validator("notify", mode="before")
-    @classmethod
-    def _addresses(cls, value):
-        """Accept one address, or a comma-separated string, as well as a list."""
-        if isinstance(value, str):
-            value = value.split(",")
-        return [str(v).strip() for v in value or [] if str(v).strip()]
-
-    @property
-    def pandda_args(self) -> dict:
-        """PanDDA2 arguments, where `pandda` was given as a mapping."""
-        return self.pandda if isinstance(self.pandda, dict) else {}
-
-    @property
-    def run_pandda(self) -> bool | None:
-        """Whether to run PanDDA2. A mapping says how to run it, not whether."""
-        return self.pandda if isinstance(self.pandda, bool) else None
+    pandda: dict[str, Any] | None = None
+    notify: str | None = None
 
     def resolve(self, key: str, parameters, param_key: str | None = None):
         """The value to use for a setting: the recipe's where it set one
@@ -92,56 +75,57 @@ def _mapping(value) -> dict:
     return value if isinstance(value, dict) else {}
 
 
-def config_path(visit_dir) -> Path:
-    """A visit's config file: whichever name is already there, else the
-    preferred one."""
-    visit_dir = Path(visit_dir)
-    for name in CONFIG_FILENAMES:
-        if (visit_dir / name).is_file():
-            return visit_dir / name
-    return visit_dir / CONFIG_FILENAMES[0]
-
-
-def _read(visit_dir, logger) -> tuple[str, dict]:
-    """A visit config file's text and its parsed contents."""
-    path = config_path(visit_dir)
+def _read(visit_dir, logger) -> tuple[Path, str, dict | None]:
+    """A visit config file's path, text and parsed contents, the last being
+    None if the file is there but could not be read or parsed."""
+    path = Path(visit_dir) / CONFIG_FILENAME
     try:
         text = path.read_text() if path.is_file() else ""
-        return text, _mapping(yaml.safe_load(text))
-    except (OSError, yaml.YAMLError) as e:
+        return path, text, _mapping(yaml.safe_load(text))
+    except (OSError, UnicodeDecodeError, yaml.YAMLError) as e:
         logger.warning(f"Ignoring unreadable visit config {path}: {e}")
-        return "", {}
+        return path, "", None
 
 
 def load_visit_config(visit_dir, logger=log) -> VisitConfig:
-    """Read a visit's config file, or an all-defaults config if it has none."""
-    _, raw = _read(visit_dir, logger)
-    acronym = _mapping(raw.get("data")).get("acronym")
+    """Read a visit's config file, or an all-defaults config if it has none.
+
+    A setting that fails validation is dropped on its own and the rest of the
+    file still applies, so one typo cannot cost a visit its whole config.
+    """
+    path, _, raw = _read(visit_dir, logger)
+    raw = raw or {}
+    values = {str(k): v for k, v in _mapping(raw.get("autoprocessing")).items()}
+    values["acronym"] = _mapping(raw.get("data")).get("acronym")
+    values["notify"] = raw.get("notify")
     try:
-        return VisitConfig(acronym=acronym, **_mapping(raw.get("autoprocessing")))
+        return VisitConfig(**values)
     except pydantic.ValidationError as e:
-        # keep the acronym: losing it would make the visit undiscoverable
-        logger.warning(f"Ignoring invalid settings in {config_path(visit_dir)}: {e}")
-        return VisitConfig(acronym=acronym)
+        # every bad field is reported in one pass, so dropping them all leaves
+        # only settings that validate
+        bad = {str(err["loc"][0]) for err in e.errors() if err["loc"]}
+        logger.warning(f"Ignoring invalid {', '.join(sorted(bad))} in {path}: {e}")
+        return VisitConfig(**{k: v for k, v in values.items() if k not in bad})
 
 
 def cache_acronym(visit_dir, acronym: str, logger=log) -> None:
-    """Add a visit's target acronym to its config file, leaving whatever the
-    user wrote untouched. Does nothing if one is already recorded."""
-    text, raw = _read(visit_dir, logger)
+    """Append a visit's target acronym to its config file, leaving whatever the
+    user wrote untouched. Does nothing if one is already recorded.
+
+    A file that already carries a `data` section gets a second one; YAML takes
+    the last, and `acronym` is the only key the pipeline puts there.
+    """
+    path, text, raw = _read(visit_dir, logger)
+    if raw is None:
+        # the file is there but unreadable; appending would destroy it
+        return
     if _mapping(raw.get("data")).get("acronym") is not None:
         return
-
-    entry = f"data:\n  acronym: {acronym}"
-    empty_section = re.compile(r"^data:[ \t]*$", re.MULTILINE)
-    if empty_section.search(text):
-        updated = empty_section.sub(entry, text, count=1)
-    else:
-        separator = "" if not text or text.endswith("\n") else "\n"
-        updated = f"{text}{separator}{entry}\n"
-
-    path = config_path(visit_dir)
+    separator = "" if not text or text.endswith("\n") else "\n"
+    # dump just the new fragment: appending keeps the user's comments and key
+    # order
+    entry = yaml.dump({"data": {"acronym": acronym}}, default_flow_style=False)
     try:
-        path.write_text(updated)
+        path.write_text(f"{text}{separator}{entry}")
     except OSError as e:
         logger.warning(f"Could not cache acronym {acronym} to {path}: {e}")