Conversation
Replaces stub sentinels with real drift probes for gevir:metrics, insider:variants and insider:interfaces, and closes the defects an adversarial review found in them. Issue #177 had recorded both sources as unprobeable. Re-checking showed GeVIR is served as the article's MOESM3 object on Springer's CDN (Content-Length + an MD5 ETag), and that INSIDER's two products are at stable direct paths despite a link-free download page. Review fixes folded in: - run_builder no longer aborts a build when a probe cannot reach its source; provenance records a self-describing PROBE_UNAVAILABLE_TOKEN so offline builds keep working on manual-acquisition plugins. - normalize_etag added to core: str.strip('"') mangles weak and transform-suffixed validators. - Signals moved under headers so drift is not auto-batched as routine; checksums left empty per _conventions §12. - INSIDER split into two probes with two baselines: the products are independently versioned, and a shared baseline meant the bot could only ledger the anchor dataset while a fault on one file masked drift in the other. - ETag compared rather than demoted for INSIDER, since it is hex(size)-hex(mtime) and catches equal-size edits Content-Length cannot. - Docs corrected: the gevirank repo ships analysis code, not data. Datasets with a live drift comparator: 18 -> 21 of 26. Refs #177
…gins (#320) Completes the sweep #319 began; together they close #177. Replaces the last five stub sentinels (dbnsfp, gnomad_metrics, cosmic_cgc, pqtl, alphagenome) with real probes, and closes the defects an adversarial review found in them. gnomad_metrics HEADs the public GCS constraint objects (MD5 ETag + Content-Length + x-goog-generation). The other four watch a release stream, because their data is gated or gone: dbNSFP's advertised S3 archives all 404 (NoSuchBucket, tracked as #321), COSMIC's census is login-gated, pqtl ships as preprint supplementary material, and alphagenome is a credentialed service. Each probe documents what it cannot see. Review fixes folded in: - dbNSFP's pattern missed every v-prefixed archive, so the entire 2.x/3.x generation was invisible (30 releases recorded, 59 actual) and a dbNSFPv5.0a.zip release would have reported clean. - COSMIC matched prose and picked up Actionability-product versions; it now reads the page's id=v<N> release anchors. - Signals moved under headers with checksums empty, per _conventions §12; the redundant sha256 dropped. - gnomad_metrics: normalize_etag, both validators required, non-identity Content-Encoding refused, timeout sized against the runner's 60s SIGALRM, failures collected across objects rather than aborting before v4.0 is probed. - pqtl selects by version rather than list position and fails closed on a missing version; alphagenome no longer hard-requires PyPI's deprecated releases key. - All five use request_with_retry; both HTML scrapes decode explicitly; COSMIC rejects a redirected response. - All five SKILL.md files updated - they still described the deleted stubs. - New suite validates every committed baseline at rest (placeholder_baseline_reason, a real fetched_at, a non-empty compared surface). Committed fingerprints 21 -> 26 of 26; live comparators 18 -> 26 of 26. No stub sentinels remain. Refs #177, #321
…ster (#323) * docs(hpc): fix the container build path so it actually works on a cluster The Apptainer/Singularity section of the HPC migration guide documented a build that cannot succeed. Four independent blockers, each verified on the cluster: * Base image had no Java 11. The guide pinned `python:3.10-slim-bookworm`, but Debian 12 ships no `openjdk-11` package at all -- apt reports it is replaced by `openjdk-17-jre-headless`, and Hail 0.2.x / Spark 3.5 support Java 8 or 11 only. Repinned to `python:3.10-slim-bullseye` (openjdk 11.0.32.1). * `--fakeroot` is not universally available. It needs an `/etc/subuid` entry; with none, the build fails with `no valid mapping entry found` and an unprivileged build is refused outright. Documented the check and the static `proot` fallback that SingularityCE accepts instead. * Extras omitted `expression`, producing an image with no scanpy in which `hvantk expression ...` cannot run. Checked the full list against [project.optional-dependencies]; `ml` and `interactive` are genuinely redundant. * The container cannot initialise Hail without `SPARK_LOCAL_DIRS` on writable node-local scratch, and the failure is misdiagnosed: the visible error is `[Errno 111] Connection refused` from py4j, which reads as a network fault. The real cause is one line above it, `DiskBlockManager: Failed to create any local dir`. The definition file now lives at `containers/hvantk.def` rather than as a prose snippet, so the base image and extras cannot drift from what was last built, and `containers/hvantk_run.sh` sets up and binds Spark scratch so the container is invoked correctly by default. Verified end to end: image builds (2.2 GB) and `hvantk utils check-install` passes on a compute node -- Hail 0.2.137 initialises and the balding_nichols_model smoke test succeeds. * docs(hpc): do not link to containers/hvantk.def from the guide mkdocs --strict rejects a link whose target is not a documentation file: WARNING - Doc file 'guide/hpc-migration.md' contains a link '../../containers/hvantk.def', but the target '../containers/hvantk.def' is not found among documentation files. Reference the path as a code span, matching how the rest of the guide names repo paths. Verified with the same command CI runs: mkdocs build --strict now exits 0.
* docs: fold the README's duplicated sections into the docs site Project structure, Optional extras, and the plugin contract each existed twice: once in README.md and once in docs_site/, where the copy was already a strict superset. The README is 272 -> 174 lines, and each section keeps a short orienting summary plus a link to the canonical page. Nothing is lost. installation.md already carried all twelve extras plus the scanpy/Intel-macOS caveat the README omitted, and architecture.md already carried a richer tree, the build sequence diagram, an annotated plugin.yaml, the two-pass loader and the streamer placement rule. The one thing only the README had -- a real builder rather than a synthetic my_source one -- is added to architecture.md, along with a note that build_clinvar is a sanctioned name: _conventions/SKILL.md cites it explicitly, and the rule is "whatever plugin.yaml's builder.function declares", so both the short and the per-dataset form are correct. Two guards asserted on the README copies and are retargeted: - test_pyproject_extras.py parsed the README extras table; it now guards only installation.md. Its docstring complained the table was "duplicated in THREE places ... and only the first is executable" -- that is now two. - test_docs_claims.py parsed the README tree in both the tree test and its "guard the guard" test; both now read architecture.md. Its docstring described architecture.md accumulating seven dead paths "while the parallel tree in README.md was corrected by hand" -- with one tree left, that divergence cannot recur. Verified: mkdocs build --strict passes, and all six README -> docs_site anchors resolve under GitHub's slug rules (each space becomes its own hyphen, so the em-dash heading is #3-plugin-contract--adding-a-data-source). The plugin-contract prose was checked against the code before being condensed: build_clinvar, its schema_id, AnnotationTable.from_hail's keyword-only provenance, run_builder_for_spec's artifact_type/schema_id validation, the sidecar .provenance.json, and the count and names of the 21 plugins are all accurate as written. * docs: the manifest downloader is optional, not mandatory The condensed plugin contract said a plugin.yaml names "its builder, drift probe, and downloader", which overstates the contract. Verified against the tree: builder and drift_probe appear in 21/21 manifests, but lifecycle.download appears in only 12 -- alphagenome, cosmic-cgc, dbnsfp, gevir, gtex-eqtl, gwas-catalog, insider, msigdb and pqtl ship none, and api.py documents DownloadFn as an "Optional download stage". That is by design, not an omission: CLAUDE.md's downloader strategy routes sources behind a license gate or over ~1 GB to a documented acquisition procedure instead. A contributor reading the old sentence would think a downloader was required and write an unnecessary one. No count is quoted in the prose -- nothing guards it, and the point holds without one. Caught by Qodo on #324. * docs: split the garden-path sentence about optional downloaders "the ones behind a license gate or too large to mirror ship a documented acquisition procedure instead" reads as "mirror ship" before the subject resolves. Two sentences, same meaning. Caught by Copilot on #324.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 34 minor |
| Security | 66 high |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Contributor
PR Summary by QodoAdd live drift probes and a reproducible HPC container path
AI Description
Diagram
High-Level Assessment
Files changed (47)
|
Contributor
Code Review by Qodo
1. Scheduler scratch can be deleted
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+17
to
+20
| SPARK_SCRATCH="${SLURM_TMPDIR:-/tmp/spark-$USER-${SLURM_JOB_ID:-$$}}" | ||
| mkdir -p "$SPARK_SCRATCH" | ||
| cleanup(){ rm -rf "$SPARK_SCRATCH"; } | ||
| trap cleanup EXIT |
Contributor
There was a problem hiding this comment.
1. Scheduler scratch can be deleted 🐞 Bug ☼ Reliability
When SLURM_TMPDIR is set, the wrapper assigns the scheduler-owned directory itself to SPARK_SCRATCH and recursively deletes it from the EXIT trap. Any failure after the trap is installed, including a missing singularity executable, can erase unrelated files stored in the job's shared scratch directory.
Agent Prompt
## Issue description
The wrapper may recursively delete the complete scheduler-provided `SLURM_TMPDIR`, including files it did not create.
## Issue Context
`SLURM_TMPDIR` should be treated as a parent directory. Create a uniquely named child owned by this invocation, bind that child, and remove only that child. Account for the fact that a successful `exec` replaces the shell and does not execute its EXIT trap.
## Fix Focus Areas
- containers/hvantk_run.sh[16-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| BINDS=(-B "$SPARK_SCRATCH:$SPARK_SCRATCH") | ||
| [[ -n "${HVANTK_BIND:-}" ]] && BINDS+=(-B "$HVANTK_BIND") | ||
|
|
||
| exec singularity exec "${BINDS[@]}" \ |
Contributor
There was a problem hiding this comment.
2. Apptainer-only clusters cannot run 🐞 Bug ≡ Correctness
The wrapper advertises Apptainer/Singularity support but unconditionally invokes singularity exec. On clusters exposing only the documented apptainer executable, every wrapper command fails before starting hvantk.
Agent Prompt
## Issue description
The wrapper hardcodes `singularity`, preventing execution on Apptainer-only clusters even though both runtimes are documented as supported.
## Issue Context
Resolve `apptainer` or `singularity` before creating scratch state, allow an explicit override if useful, and emit a clear error if neither exists.
## Fix Focus Areas
- containers/hvantk_run.sh[11-30]
- docs_site/guide/hpc-migration.md[93-104]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+68
to
+70
| releases = sorted((payload.get("releases") or {}).keys()) | ||
| if releases: | ||
| compared["release_count"] = len(releases) |
Contributor
There was a problem hiding this comment.
3. Deprecated field causes false drift 🐞 Bug ≡ Correctness
The AlphaGenome probe places release_count from PyPI's deprecated releases field in the compared headers surface, despite intentionally allowing that field to disappear. When PyPI removes it—or when a yanked/prerelease entry changes—the probe reports drift even though info.version and the installable SDK are unchanged.
Agent Prompt
## Issue description
A deprecated, non-authoritative release count is included in the compared fingerprint and creates false drift.
## Issue Context
The current SDK version is already the authoritative signal. Keep the release list/count under `informational`, and add a regression test proving that removal or alteration of `releases` does not change the compared fingerprint when `info.version` is unchanged.
## Fix Focus Areas
- hvantk/skills/alphagenome/drift_probe.py[56-81]
- hvantk/skills/alphagenome/tests/test_drift_probe.py[60-69]
- hvantk/skills/alphagenome/tests/drift_fingerprint.json[4-12]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+58
to
+64
| # Sized against the runner's budget, not per request. drift_cli defaults | ||
| # --timeout to 60s and enforces it with a single SIGALRM around the whole probe, | ||
| # while requests applies its timeout separately to connect and read. At the | ||
| # previous 30s this loop's worst case was 3 x 60 = 180s, so a merely slow bucket | ||
| # reported probe_failed on a healthy source. A (connect, read) pair keeps the | ||
| # whole loop inside the alarm. | ||
| _TIMEOUT_S = (5.0, 10.0) |
Contributor
There was a problem hiding this comment.
4. Retries exceed probe timeout 🐞 Bug ☼ Reliability
The gnomAD probe's per-request (5, 10) timeout does not keep its three-object loop within the 60-second probe budget because request_with_retry performs four attempts with backoff for each object. One object can consume roughly 54 seconds on repeated read timeouts, so another slow object causes the global alarm to abort an otherwise recoverable probe.
Agent Prompt
## Issue description
Per-attempt request timeouts and default retries can exceed the drift runner's single whole-probe timeout.
## Issue Context
Set an explicit attempt count and smaller request/backoff limits based on the number of objects, or implement a shared deadline so all objects retain a chance to be probed before the 60-second alarm.
## Fix Focus Areas
- hvantk/skills/gnomad_metrics/drift_probe.py[58-83]
- hvantk/core/utils/http.py[30-36]
- hvantk/core/utils/http.py[81-145]
- hvantk/core/plugin/drift_runner.py[218-239]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes
devtomain. Four commits, all green ondev:Bulk of the diff is the drift-probe work:
insider/shared/http_probe.py, per-dataset probes and fingerprints, plus new guards (test_committed_drift_baselines.py,test_probe_normalize_etag.py,test_run_builder_probe_degradation.py).mainruns a wider matrix thandev— it addsPython applicationandDeploy docson top of the 3.10/3.11/3.12 build, the two Hail jobs,build-linuxand the smoke test. That wider set is the point of the gate, so nothing here is merged until it is green.Note the docs change in #324 retargets two doc-guard tests from
README.mdtodocs_site/, since the README copies they parsed are gone. Both were verified green locally and on the #324 PR run.