diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index b0dbd4b..f6c554f 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -774,6 +774,28 @@ jobs: name: benchmark-openswath-${{ inputs.runtime_source == 'package' && needs.package-runtime.outputs.openms_sha || needs.build-openms.outputs.openms_sha }} path: results-openswath if-no-files-found: error + + # A failed "Run OpenSwath benchmark" step skips every later step (they + # default to success()), so the normalization and the results upload + # above would both be skipped and the evidence the failure needs would be + # lost with the runner. Capture it here instead. `failure() && !cancelled()` + # keeps the upload off cancelled runs; the explicit paths plus + # if-no-files-found: ignore mean a run that died before writing anything + # cannot turn one failure into two. + - name: Upload OpenSwath diagnostics on failure + if: ${{ failure() && !cancelled() }} + uses: actions/upload-artifact@v7 + with: + name: benchmark-openswath-diagnostics-${{ inputs.runtime_source == 'package' && needs.package-runtime.outputs.openms_sha || needs.build-openms.outputs.openms_sha }} + path: | + results-openswath/openswath.json + results-openswath/output.featureXML + results-openswath/output.json + results-openswath/summary.txt + results-openswath/stages.tsv + results-openswath/meta.txt + results-openswath/logs/** + if-no-files-found: ignore # --------------------------------------------------------------------------- # ProSE + PeptDeep ONNX pipeline validation. # diff --git a/benchmark/openswath_diagnostics.py b/benchmark/openswath_diagnostics.py new file mode 100644 index 0000000..8d6f8a2 --- /dev/null +++ b/benchmark/openswath_diagnostics.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +"""Explain a failed OpenSwath benchmark report from openswath.json. + +The runner used to print only:: + + library join failed - see library_error in openswath.json + +which names the field but not its value, so a CI failure did not say *why* the +TraML <-> featureXML join failed. This module renders that value, or states +precisely why there is none (missing file, unreadable file, malformed JSON, +absent or empty ``correctness.library_error``), and lists the non-passing +stages and the artifacts the join consumed. + +Everything here is best-effort: the caller is already on a failure path, so +``describe_failure`` never raises for bad input and ``main`` never changes the +benchmark's exit status. Output is bounded - it never dumps the report itself. +""" + +import json +import os +import sys + +#: A library_error is a short message from openswath_metrics, but clip anyway: +#: a future error could embed a whole transition list, and the log is not a +#: place for that. +MAX_ERROR_CHARS = 2000 + +#: Stages are already summarised in stages.tsv; the log only needs the ones +#: that did not pass. +MAX_STAGE_LINES = 8 + +#: Files the join consumes or produces, reported as present/absent with size. +ARTIFACTS = ("output.featureXML", "output.chrom.mzML", "output.json") + + +def _clip(value): + if len(value) <= MAX_ERROR_CHARS: + return value + return "%s ... [%d more characters]" % ( + value[:MAX_ERROR_CHARS], + len(value) - MAX_ERROR_CHARS, + ) + + +def _stage_lines(report): + """One line per stage that did not pass, bounded, required stages first.""" + stages = report.get("stages") + if not isinstance(stages, list): + return [] + + failing = [stage for stage in stages if isinstance(stage, dict) + and stage.get("status") != "pass"] + failing.sort(key=lambda stage: stage.get("required") is not True) + + lines = [] + for stage in failing[:MAX_STAGE_LINES]: + lines.append( + "stage %s: status=%s exit_code=%s reason=%s" + % ( + stage.get("name", "(unnamed)"), + stage.get("status", "(missing)"), + stage.get("exit_code", "(missing)"), + stage.get("reason") or "(none)", + ) + ) + if len(failing) > MAX_STAGE_LINES: + lines.append("... %d more non-passing stages" % (len(failing) - MAX_STAGE_LINES)) + return lines + + +def _artifact_lines(directory): + """Size of each result file, so a missing featureXML is visible at a glance.""" + lines = [] + for name in ARTIFACTS: + path = os.path.join(directory, name) + try: + size = os.path.getsize(path) + except OSError: + lines.append("artifact %s: missing" % name) + else: + lines.append("artifact %s: %d bytes" % (name, size)) + return lines + + +def describe_failure(json_path): + """Return bounded, human-readable lines describing why the run did not pass.""" + try: + with open(json_path, "rb") as handle: + raw = handle.read() + except FileNotFoundError: + return ["openswath.json is missing: %s" % json_path] + except OSError as exc: + return ["openswath.json could not be read: %s" % exc] + + try: + report = json.loads(raw) + except ValueError as exc: + # json.JSONDecodeError and UnicodeDecodeError are both ValueErrors. + return ["openswath.json is not valid JSON (%d bytes): %s" % (len(raw), exc)] + + if not isinstance(report, dict): + return ["openswath.json is not a JSON object (got %s)" % type(report).__name__] + + lines = [] + verdict = report.get("verdict") + lines.append("verdict: %s" % (verdict if isinstance(verdict, str) else "(missing)")) + + correctness = report.get("correctness") + if not isinstance(correctness, dict): + lines.append("library_error: (correctness section missing or not an object)") + elif "library_error" not in correctness: + lines.append("library_error: (key absent - the join did not report an error)") + else: + error = correctness["library_error"] + if not isinstance(error, str): + lines.append("library_error: (not a string: %s)" % type(error).__name__) + elif not error.strip(): + lines.append("library_error: (empty string)") + else: + clipped = _clip(error) + if "\n" in clipped: + lines.append("library_error:") + lines.extend(" %s" % part for part in clipped.splitlines()) + else: + lines.append("library_error: %s" % clipped) + + lines.extend(_stage_lines(report)) + lines.extend(_artifact_lines(os.path.dirname(os.path.abspath(json_path)))) + return lines + + +def main(argv=None): + argv = list(sys.argv[1:] if argv is None else argv) + if len(argv) != 1: + print("usage: openswath_diagnostics.py ", file=sys.stderr) + return 2 + + for line in describe_failure(argv[0]): + print(line) + # Diagnostics never change the benchmark's exit status. + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/benchmark/run_openswath_benchmark.sh b/benchmark/run_openswath_benchmark.sh index a676c34..3ed14b4 100644 --- a/benchmark/run_openswath_benchmark.sh +++ b/benchmark/run_openswath_benchmark.sh @@ -118,6 +118,14 @@ run_stage() { record_stage "${name}" "${required}" "${rc}" "${wall_s}" "${cpu_s}" "${peak_kb}" "${status}" "${reason}" } +# Print why openswath.json did not pass, without changing the exit status. +# Best effort: a missing interpreter, a missing file or malformed JSON must not +# replace the benchmark failure with a diagnostic error. +report_openswath_failure() { + python3 "${SCRIPT_DIR}/openswath_diagnostics.py" "${RESULTS_DIR}/openswath.json" 2>&1 \ + | while IFS= read -r line; do log " ${line}"; done +} + # --- checks ------------------------------------------------------------------ echo "=== OpenMS benchmark: OpenSwath DIA ===" | tee "${RESULTS_DIR}/summary.txt" @@ -304,7 +312,10 @@ if python3 -c 'import json,sys; sys.exit(0 if json.load(open(sys.argv[1]))["verd : # openswath.json verdict already combines stage + library-join outcomes else verdict="fail" - log "library join failed - see library_error in openswath.json" + log "openswath.json verdict is not pass - diagnostics:" + if ! report_openswath_failure; then + log " (openswath diagnostics unavailable - see openswath.json)" + fi fi log "" diff --git a/benchmark/test_openswath_diagnostics.py b/benchmark/test_openswath_diagnostics.py new file mode 100644 index 0000000..bd6d4bf --- /dev/null +++ b/benchmark/test_openswath_diagnostics.py @@ -0,0 +1,165 @@ +#!/usr/bin/env python3 +"""Tests for openswath_diagnostics.py (OpenSwath DIA failure diagnostics). + +Covers the cases the runner has to survive once openswath.json says "not pass": +a real library_error, an empty one, a missing key, malformed JSON, a JSON value +that is not an object, and a missing file. Also checks that the rendered output +stays bounded, because it is written to a CI log. + +Run: python3 benchmark/test_openswath_diagnostics.py +""" + +import json +import os +import sys +import tempfile + +HERE = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, HERE) + +import openswath_diagnostics as od # noqa: E402 + + +def _write(tmp, report, name="openswath.json"): + path = os.path.join(tmp, name) + with open(path, "w") as handle: + if isinstance(report, str): + handle.write(report) + else: + json.dump(report, handle) + return path + + +def _report(**correctness): + return { + "verdict": "fail", + "stages": [ + {"name": "openswath_workflow", "required": True, "exit_code": 0, + "status": "pass", "reason": ""}, + {"name": "join", "required": True, "exit_code": 1, + "status": "fail", "reason": "required stage failed"}, + ], + "correctness": correctness, + } + + +def _joined(lines): + return "\n".join(lines) + + +def test_valid_library_error_is_reported(): + with tempfile.TemporaryDirectory() as tmp: + message = ( + "feature 1 references transition native_id 'group_Precursor_i0', " + "which the library does not define" + ) + path = _write(tmp, _report(library_error=message)) + out = _joined(od.describe_failure(path)) + + assert "verdict: fail" in out, out + assert ("library_error: " + message) in out, out + # the failing stage is listed, the passing one is not + assert "stage join:" in out, out + assert "stage openswath_workflow" not in out, out + # artifacts are reported even though none exist here + assert "artifact output.featureXML: missing" in out, out + print("PASS: a valid library_error is printed verbatim") + + +def test_empty_library_error_is_named(): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, _report(library_error=" ")) + out = _joined(od.describe_failure(path)) + + assert "library_error: (empty string)" in out, out + print("PASS: an empty library_error is named instead of printed as blank") + + +def test_missing_library_error_key_is_distinguished(): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, _report(actual_features=6)) + out = _joined(od.describe_failure(path)) + + assert "library_error: (key absent" in out, out + # the non-passing stage is still reported, so a pure stage failure is + # not mistaken for a silent join failure + assert "stage join:" in out, out + print("PASS: an absent library_error key does not look like an empty one") + + +def test_malformed_json_does_not_raise(): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, '{"verdict": "fail", "correctness": {') + lines = od.describe_failure(path) + + assert len(lines) == 1, lines + assert "not valid JSON" in lines[0], lines + # no fragment of the file's content leaks into the log + assert "correctness" not in lines[0], lines + print("PASS: malformed JSON is reported without echoing its content") + + +def test_non_object_json_is_reported(): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, json.dumps(["fail"])) + lines = od.describe_failure(path) + + assert "not a JSON object" in lines[0], lines + assert "list" in lines[0], lines + print("PASS: a JSON value that is not an object is reported") + + +def test_missing_file_is_reported(): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "absent.json") + lines = od.describe_failure(path) + + assert len(lines) == 1, lines + assert "is missing" in lines[0], lines + print("PASS: a missing openswath.json is reported") + + +def test_long_error_is_clipped(): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, _report(library_error="x" * (od.MAX_ERROR_CHARS * 3))) + out = _joined(od.describe_failure(path)) + + assert "more characters]" in out, out + for line in od.describe_failure(path): + assert len(line) <= od.MAX_ERROR_CHARS + 200, len(line) + print("PASS: a long library_error is clipped to a bounded log line") + + +def test_non_string_library_error_is_reported(): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, _report(library_error={"detail": "nested"})) + out = _joined(od.describe_failure(path)) + + assert "library_error: (not a string: dict)" in out, out + print("PASS: a non-string library_error is reported by type, not dumped") + + +def test_main_never_changes_exit_status(): + with tempfile.TemporaryDirectory() as tmp: + missing = os.path.join(tmp, "absent.json") + assert od.main([missing]) == 0 + # wrong usage is the only non-zero case, and it is not the failure path + assert od.main([]) == 2 + print("PASS: main() returns 0 on a missing report (diagnostics never fail the run)") + + +def main(): + test_valid_library_error_is_reported() + test_empty_library_error_is_named() + test_missing_library_error_key_is_distinguished() + test_malformed_json_does_not_raise() + test_non_object_json_is_reported() + test_missing_file_is_reported() + test_long_error_is_clipped() + test_non_string_library_error_is_reported() + test_main_never_changes_exit_status() + print("\nAll openswath_diagnostics tests passed.") + + +if __name__ == "__main__": + main() diff --git a/benchmark/test_openswath_diagnostics.sh b/benchmark/test_openswath_diagnostics.sh new file mode 100644 index 0000000..aab687a --- /dev/null +++ b/benchmark/test_openswath_diagnostics.sh @@ -0,0 +1,109 @@ +#!/usr/bin/env bash +# test_openswath_diagnostics.sh - exercise the runner's failure diagnostics. +# +# The diagnostic path only runs after a failed benchmark, so it cannot be +# covered by running the benchmark itself. This test extracts `log` and +# `report_openswath_failure` from run_openswath_benchmark.sh (so the shipped +# code is the tested code) and drives them with synthetic openswath.json files. +# +# Run: bash benchmark/test_openswath_diagnostics.sh +set -euo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +RUNNER="${HERE}/run_openswath_benchmark.sh" +TMP="$(mktemp -d)" +trap 'rm -rf "${TMP}"' EXIT + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +# --- interpreter ------------------------------------------------------------- +# The runner calls `python3`; on hosts where only `python` works (Git Bash on +# Windows), put a shim first on PATH so the extracted function is unchanged. +if ! python3 -c 'import sys' >/dev/null 2>&1; then + real_python="$(command -v python || true)" + [[ -n "${real_python}" ]] || fail "no usable Python interpreter found" + mkdir -p "${TMP}/bin" + printf '#!/usr/bin/env bash\nexec "%s" "$@"\n' "${real_python}" > "${TMP}/bin/python3" + chmod +x "${TMP}/bin/python3" + PATH="${TMP}/bin:${PATH}" +fi + +# --- extract the functions under test --------------------------------------- +extract_fn() { + awk -v fn="$1" ' + index($0, fn "() {") == 1 { + print + if ($0 ~ /}$/) exit + inside = 1 + next + } + inside { + print + if ($0 == "}") exit + } + ' "${RUNNER}" +} + +log_src="$(extract_fn log)" +report_src="$(extract_fn report_openswath_failure)" +[[ -n "${log_src}" ]] || fail "could not extract log() from ${RUNNER}" +[[ -n "${report_src}" ]] || fail "could not extract report_openswath_failure() from ${RUNNER}" +eval "${log_src}" +eval "${report_src}" + +# --- fixtures ---------------------------------------------------------------- +RESULTS_DIR="${TMP}/results" +SCRIPT_DIR="${HERE}" +mkdir -p "${RESULTS_DIR}" +: > "${RESULTS_DIR}/summary.txt" + +write_report() { printf '%s' "$1" > "${RESULTS_DIR}/openswath.json"; } + +check() { + local label="$1" expected="$2" out + out="$(report_openswath_failure)" + if ! grep -Fq -- "${expected}" <<< "${out}"; then + printf '%s\n' "${out}" >&2 + fail "${label}: expected ${expected@Q} in diagnostics" + fi + # the same text must reach summary.txt, otherwise the uploaded log loses it + grep -Fq -- "${expected}" "${RESULTS_DIR}/summary.txt" \ + || fail "${label}: diagnostics were not written to summary.txt" + echo "PASS: ${label}" +} + +# --- valid library_error ----------------------------------------------------- +write_report '{"verdict":"fail","stages":[],"correctness":{"library_error":"feature 7 references transition native_id '"'"'grp_Precursor_i0'"'"', which the library does not define"}}' +check "valid library_error" "library_error: feature 7 references transition native_id 'grp_Precursor_i0', which the library does not define" +check "verdict is printed" "verdict: fail" +check "missing featureXML is visible" "artifact output.featureXML: missing" + +# --- empty library_error ----------------------------------------------------- +write_report '{"verdict":"fail","correctness":{"library_error":" "}}' +check "empty library_error" "library_error: (empty string)" + +# --- absent library_error key ------------------------------------------------ +write_report '{"verdict":"fail","stages":[{"name":"openswath_workflow","required":true,"status":"fail","exit_code":1,"reason":"required stage failed (see logs/openswath_workflow.log)"}],"correctness":{}}' +check "absent library_error" "library_error: (key absent" +check "failing stage is named" "stage openswath_workflow: status=fail exit_code=1" + +# --- malformed JSON ---------------------------------------------------------- +write_report '{"verdict": "fail", "correctness": {' +check "malformed JSON" "not valid JSON" + +# --- absent openswath.json --------------------------------------------------- +rm -f "${RESULTS_DIR}/openswath.json" +check "missing openswath.json" "openswath.json is missing" + +# --- a broken diagnostics call must be reported, not swallowed -------------- +SCRIPT_DIR="${TMP}" # openswath_diagnostics.py is not here +if report_openswath_failure >/dev/null 2>&1; then + fail "a failing diagnostic script must return non-zero so the caller can say so" +fi +echo "PASS: a broken diagnostics call is detectable by the caller" + +echo +echo "All openswath_diagnostics shell tests passed."