cd98d642c99cbe1e5648734f7735452b69c01c23 mspeir Fri Aug 21 13:05:22 2026 -0700 G2P otto: a lock, an atomic install, and separate stderr, refs #38142 Four hardening items from the same review, none of which change the track data. install() claimed to be atomic but did rm followed by ln, leaving a window in which /gbdb/<db>/g2p/g2p.bb did not exist. It now builds the symlink under a temp name and renames it over the live one. bash() folded stderr into stdout, and loadCoordinates parses that return value as bigBed rows, so a single warning from bigBedToBed would have arrived looking like data. The two streams are separate now, with stderr passed through to ours so the otto mail still shows it. Two runs would share a build directory and race on the move of AllG2P.csv over prevAllG2P.csv, which decides whether the next run thinks anything changed. A non-blocking flock means the second run says so and stops. The hgnc_id field is stripped, as the value used to join on it already was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> diff --git src/hg/utils/otto/g2p/doG2p.py src/hg/utils/otto/g2p/doG2p.py index dc6b089ee56..b279fc774fc 100755 --- src/hg/utils/otto/g2p/doG2p.py +++ src/hg/utils/otto/g2p/doG2p.py @@ -11,61 +11,88 @@ What it does, once a month: 1. Download the full G2P panel CSV. 2. No-op (silent) if the download is byte-identical to last run's copy. 3. Sanity check: required columns must all be present, else abort loudly. 4. For hg19 and hg38: join G2P records to gene coords from the HGNC bigBed track and build a bed9+20 bigBed in a dated working directory. 5. Guard: abort if item count moved >10% vs the live track (unless --force). 6. Atomically repoint /gbdb/<db>/g2p/g2p.bb at the new dated bigBed. The dated working directories double as the archive of past builds. """ import argparse import csv +import fcntl +import os import subprocess import sys from datetime import datetime from pathlib import Path WORKDIR = "/hive/data/outside/otto/g2p" +LOCK_FILE = WORKDIR + "/doG2p.lock" DBS = ["hg19", "hg38"] DOWNLOAD_URL = "https://www.ebi.ac.uk/gene2phenotype/api/panel/all/download" AS_FILE = WORKDIR + "/g2p.as" EXPECTED_COLUMNS_FILE = WORKDIR + "/expectedColumns.txt" NEW_CSV = WORKDIR + "/AllG2P.csv" PREV_CSV = WORKDIR + "/prevAllG2P.csv" GBDB_BB = "/gbdb/%s/g2p/g2p.bb" # live symlink, per-db COUNT_TOLERANCE = 0.10 # 10% item-count change requires --force parser = argparse.ArgumentParser(description="Build and update the G2P track.") parser.add_argument("--force", action="store_true", help="Rebuild even if the download is unchanged, and bypass " "the >10%% item-count safety check.") args = parser.parse_args() def bash(cmd): - """Run cmd in a bash subprocess, returning stdout; raise on non-zero exit.""" + """Run cmd in a bash subprocess, returning stdout; raise on non-zero exit. + + stdout is kept apart from stderr on purpose. loadCoordinates parses this return + value as data, so folding the two together means one warning from the underlying + tool arrives looking like a row of a bigBed. Whatever the command puts on stderr + is passed through to ours, so the otto mail still carries it. + """ try: out = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, - universal_newlines=True, stderr=subprocess.STDOUT) - return out.stdout + stderr=subprocess.PIPE, universal_newlines=True) except subprocess.CalledProcessError as e: raise RuntimeError("command '{}' returned error (code {}): {}".format( - e.cmd, e.returncode, e.output)) + e.cmd, e.returncode, (e.stderr or "") + (e.output or ""))) + if out.stderr: + sys.stderr.write(out.stderr) + return out.stdout + + +def acquireLock(): + """Take an exclusive lock so two runs cannot interleave. + + Two runs share one build directory and both finish by moving AllG2P.csv over + prevAllG2P.csv, so an overlap can leave the "has the download changed" check + comparing against a file the other run wrote. The lock is held until this + process exits; the returned handle only needs to stay referenced. + """ + fh = open(LOCK_FILE, "w") + try: + fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + sys.exit("another doG2p.py still holds %s; not starting a second run" % LOCK_FILE) + return fh def download(url, outFile): """Download the G2P panel CSV.""" bash("curl -sSf -L -o %s '%s'" % (outFile, url)) def md5(path): return bash("md5sum %s" % path).split()[0] def updateNeeded(): """Download the CSV; return True if it differs from last run (or --force).""" download(DOWNLOAD_URL, NEW_CSV) if args.force: @@ -185,31 +212,31 @@ # can carry several coordinate rows, which would inflate the tally. rgb = confidenceToColor(row["confidence"]) if rgb is None: # Tally on the folded value so case and stray whitespace do not split # one unknown value into several, but keep a raw example alongside it: # the folded form is not what is in the CSV, so it is not what someone # reading the log would grep for. key = normalizeConfidence(row["confidence"]) count, example = unknownConfidence.get(key, (0, row["confidence"])) unknownConfidence[key] = (count + 1, example) rgb = DEFAULT_COLOR # G2P 20 fields g2pId = row["g2p id"] geneMim = row["gene mim"] - hgncIdVal = row["hgnc id"] + hgncIdVal = row["hgnc id"].strip() # stripped, as the join key is prevSymbols = row["previous gene symbols"].replace(";", ",") diseaseName = row["disease name"] diseaseMim = row["disease mim"] diseaseMondo = row["disease MONDO"] allelicReq = row["allelic requirement"] crossMod = row["cross cutting modifier"] confidence = row["confidence"] varConseq = row["variant consequence"] varTypes = row["variant types"] molMech = row["molecular mechanism"] molMechCat = row["molecular mechanism categorisation"] molMechEv = row["molecular mechanism evidence"] phenotypes = row["phenotypes"].replace(";", ",") publications = row["publications"].replace(";", ",") panel = row["panel"] @@ -249,39 +276,46 @@ print("%s: no live bigBed yet, skipping item-count check" % db) return old = itemCount(liveBb) new = itemCount(newBb) print("%s item count: live=%d new=%d" % (db, old, new)) if abs(new - old) > COUNT_TOLERANCE * max(new, old): msg = "WARNING: %s item count changed >%.0f%% (live=%d new=%d)" % ( db, COUNT_TOLERANCE * 100, old, new) if args.force: print(msg + " (continuing due to --force)") else: sys.exit(msg + "\nRun ./doG2p.py --force if you approve this change.") def install(db, newBb): - """Atomically repoint /gbdb/<db>/g2p/g2p.bb at the freshly built bigBed.""" + """Repoint /gbdb/<db>/g2p/g2p.bb at the freshly built bigBed, atomically. + + Build the new symlink under a temp name and rename it over the live one. The + rm + ln this replaces left a window, short but real, in which the live path did + not exist at all -- and the browser reads that path. + """ liveBb = GBDB_BB % db bash("mkdir -p %s" % str(Path(liveBb).parent)) - bash("rm -f %s" % liveBb) - bash("ln -s %s %s" % (newBb, liveBb)) + tmpLink = "%s.tmp%d" % (liveBb, os.getpid()) + bash("ln -sfn %s %s" % (newBb, tmpLink)) + bash("mv -T %s %s" % (tmpLink, liveBb)) print("Installed %s -> %s" % (liveBb, newBb)) def main(): + lock = acquireLock() # held until the process exits if not updateNeeded(): # Silent no-op: nothing new from G2P this run. return validateColumns(NEW_CSV) date = str(datetime.now()).split(" ")[0] buildDir = "%s/%s" % (WORKDIR, date) bash("mkdir -p %s" % buildDir) bash("cp %s %s/AllG2P.csv" % (NEW_CSV, buildDir)) g2pData = loadG2p(NEW_CSV) hgncIds = list(g2pData.keys()) print("Number of HGNC IDs found: %s" % len(hgncIds))