Conversation
Everything relating to the synthesized proband is renamed from "dummy" to
"synthetic" for consistency with the metamist analysis type Ed added
(synthetic_gvcf) and for professionalism, since this repo is public.
- File renames:
scripts/synthesize_dummy_probands.py -> scripts/create_synthetic_proband_gvcf.py
dummy_gvcf_workflow.py -> synthetic_proband_gvcf_workflow.py
- File deleted:
scripts/query_dummy_site.py (ad-hoc debug script for a GQ bug that has since been fixed)
- Stage classes:
GenerateDummyProbands -> GenerateSyntheticProbandGvcfs
GenerateDummyProbandsCombinerInputs -> GenerateSyntheticProbandCombinerInputs
- Config key:
combiner.dummy_gvcf_text_file -> combiner.synthetic_gvcf_text_file
- Function:
synthesize_dummy_gvcf -> create_synthetic_gvcf
- Output filenames, log messages, docstrings, and comments follow the same rename.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds the first of the two pre-workflow stages that Ed stubbed. Stage 1 now
enumerates duo-only families from the multicohort, spawns one synthesis job
per family invoking scripts/create_synthetic_proband_gvcf.py, then spawns
one metamist registration job per family (depending on its synthesis job).
- utils.get_families_for_synthetic_probands: cached helper that groups the
multicohort's SGs by family_id and keeps only clean parental duos (2
members, one male + one female, no proband, both with gVCFs).
- utils.SyntheticProbandFamily: dataclass carrying the single source of
truth for the synthetic sample name (embedded in the gVCF, the PED, and
the seqr MT - MUST match across all three or inheritance filters silently
fail).
- jobs/GenerateSyntheticProbandGvcfs.create_synthesis_jobs: one BashJob per
family invoking the synthesis script; storage/memory overridable via
synthetic_proband.* config keys. Returns dict[family_id -> job] so
registration can pair up via depends_on.
- jobs/GenerateSyntheticProbandGvcfs.create_analysis_registration_jobs: one
BashJob per family invoking the new registration script. Tolerates
missing upstream synthesis jobs (framework-skipped when output exists)
by running with no dependency, letting the script's own idempotency
handle it.
- scripts/register_synthetic_gvcf_analysis.py: idempotent + stale-aware
registration. Queries metamist for an active synthetic_gvcf analysis
linked to exactly the two parental SGs, compares meta.source_mother_gvcf
/ meta.source_father_gvcf against the current parental paths; skips if
matching, deactivates + recreates if stale.
Metamist analysis type ('synthetic_gvcf') has already been added upstream
by Ed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ombiner run The second pre-workflow stage produces the two files the combiner needs to run a trio-shaped joint call for ravenscroft-rpl (and any future duo-only cohort) - a pedigree with the synthetic probands inserted, and a gVCF manifest. - utils.build_gvcf_manifest_content / write_gvcf_manifest: pure content builder plus a driver-side writer. The writer will also replace the inlined manifest-writing pattern currently in CombineGvcfsIntoVds (upcoming Task 6). - utils.build_synthetic_pedigree_content: three PED rows per qualifying duo family (mother, father, synthetic proband). Individual IDs are chosen to match the sample names embedded in each gVCF - a mismatch would silently disable trio inheritance filtering in seqr. - jobs/GenerateSyntheticProbandCombinerInputs: two Batch job factories. Content is computed in the driver and dropped into the Batch job via a quoted heredoc, with write_output uploading to the durable output path once the job succeeds. - Manifest composition is deliberately inclusive: every real SG in the multicohort with a gVCF is listed, whether or not their family qualified for synthetic proband synthesis. Excluding non-qualifying SGs would silently drop them from the seqr load, which is worse than including them without a trio row in the PED. - stages.GenerateSyntheticProbandCombinerInputs: required_stages set to the Stage 1 class so the combiner-inputs jobs wait for the synthesis + registration jobs. Outputs written to self.prefix so seqr sync can reuse them across loads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…manifest
Restructure the three cases as a proper if/elif/else so the pre-baked
synthetic-gvcf manifest isn't wiped by the fall-through `gvcf_add_arg = ''`
line, and delete the "# figure this out" placeholder. Replace the inlined
manifest-writing loop with utils.write_gvcf_manifest so both this stage and
the synthetic-proband pre-workflow build manifests through the same helper.
Behavioural summary:
- combiner.synthetic_gvcf_text_file set -> use it verbatim (feeds the
synthetic-trio combiner run for ravenscroft-rpl)
- new_sg_gvcfs available -> write a manifest from them via
utils.write_gvcf_manifest (existing seqr-loader behaviour)
- neither -> no --gvcf_add_file argument
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…hesis The ravenscroft-rpl cohort has at least two family shapes that our filter needs to reject cleanly: a multi-child family with one affected member, and a single affected individual with no parents. The existing len/parents-set checks cover both, but they don't handle a 2-sibling family whose members happen to have no .dad/.mom set - the previous filter would treat them as a parental duo and (wrongly) synthesize a proband. An affected member (phenotype=2) is a strong signal that a real proband is already present in the family, whether or not the pedigree's parents-set fields are populated. Skip when we see one. Phenotype=0 (unknown / not set) is left permissive so legitimate parental duos with incomplete metadata still qualify. The two-sibling case with both phenotype=0 remains an unaddressable metadata gap - documented in a comment so future maintainers know why. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…and synthesis" This reverts commit 20726dd.
Two related fixes surfaced by the first live run: 1. Registration failed at test access level because register_synthetic_gvcf_analysis.py called the raw AnalysisApi().create_analysis(project=<bare-dataset-name>). At test access the service account only has write permission on the `-test`-suffixed metamist project, so the request 403'd. Switched to cpg_flow.metamist.get_metamist().create_analysis(), which applies the `-test` suffix internally via get_metamist_proj() and also inherits the framework's retry + logging wrappers. The bare dataset name passed through from stages.py is now correct. 2. utils.build_synthetic_pedigree_content was hand-rolling PED rows for the two parents in each qualifying family. It now iterates the whole multicohort using cpg-flow's sg.pedigree.get_ped_dict() primitive (the same source of truth as multicohort.write_ped_file) and then appends a synthetic-proband row per qualifying family. Side benefit: real SGs from non-qualifying families now appear in the PED too, so seqr can do trio filtering on any real trio whose pedigree metadata was correctly set upstream. Non-qualifying-family samples are already in the combiner manifest for the same reason. Signature of create_pedigree_job now takes multicohort; stages.py updated to pass it through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Second live run exposed a design flaw: cpg_flow's stage-level [REUSE] optimisation skipped Stage 1 wholesale when all expected outputs (the gVCFs) already existed on disk. That meant queue_jobs was never called, so the metamist registration jobs never got submitted - leaving the bucket with synthetic gVCFs but no metamist analysis records. The fix threads a per-family sentinel through the whole pipeline: - register_synthetic_gvcf_analysis.py accepts --marker_path and writes an empty file at that path as its final line. Any earlier exception skips the touch, so the marker's existence is a real success signal. - Stage 1's expected_outputs now declares two keys per family: <family_id>_gvcf and <family_id>_registered. cpg_flow only marks the stage [REUSE] when *both* files exist - so when gVCFs are present but registration didn't complete, the stage QUEUEs correctly. - create_synthesis_jobs and create_analysis_registration_jobs each gained a per-family skip check. If this specific family's gVCF (or marker) already exists, no job is queued for it. This means the next re-run after this fix will submit only 4 registration jobs (one per qualifying family) and zero synthesis jobs. - Stage 2 filters Stage 1's outputs down to just the _gvcf entries before building the manifest. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborators only have access to the external family ID (F000045871,
HP2613, etc.) - not the metamist internal ID (18958). Renaming
synthetic-proband artefacts to use external IDs makes the pipeline's
outputs recognisable to them.
- utils.get_family_external_id_map(dataset_name): new helper backed by
metamist.apis.FamilyApi().get_families(); returns
{internal_family_id_str: external_family_id} for a project. Uses the
existing cpg-flow get_metamist_proj wrapper so access-level suffixing
is handled centrally. Cached.
- SyntheticProbandFamily gains an `external_family_id` field; the
internal `family_id` is kept for stable dict keys and metamist meta.
- synthetic_sample_name (gVCF SM tag / PED individual ID / seqr sample
ID) now uses the external family ID.
- Stage 1's expected_outputs, output filenames, Batch job labels, and
the --family_id CLI arg passed to the registration script all use
the external ID.
- build_synthetic_pedigree_content remaps Family.ID on real SG rows
from internal to external too, so seqr groups them into the same
family as the synthetic proband row.
- Families with no external ID recorded in metamist are logged and
skipped rather than silently mixing internal IDs into the artefacts.
Note: existing artefacts from the previous run use internal-ID names
and will be orphaned on the next run. Wipe the bucket + deactivate the
old metamist analyses before re-running so the state stays consistent.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The FamilyApi.get_families REST client silently dropped external IDs
recorded under an empty-string source key ({"": "20150"}), which is the
shape create_test_subset.py produces when copying families to a -test
project. That caused get_family_external_id_map to return an empty map,
which in turn caused get_families_for_synthetic_probands to reject every
otherwise-qualifying family with "no external family ID recorded".
Switched to the same GraphQL query pattern the rest of utils.py already
uses (LATEST_ANALYSIS_QUERY, SPECIFIC_VDS_QUERY). The GraphQL externalId
scalar returns the value regardless of the source-key shape, so the map
is populated correctly. Same signature; only the internals of
get_family_external_id_map change.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Ruff and mypy issues surfaced when pre-commit ran on the branch. Fixes touch code style / static typing only, not runtime behaviour. - chmod +x synthetic_proband_gvcf_workflow.py to match sibling entrypoints (EXE001; the git mv rename didn't preserve the exec bit). - Reorder import statements in synthetic_proband_gvcf_workflow.py and register_synthetic_gvcf_analysis.py to match ruff's isort layout (I001) - cpg_flow / cpg_utils grouped, cpg_seqr_loader in its own block below. - Suppress UP037 on the two variable annotations in GenerateSyntheticProbandGvcfs.py that reference the TYPE_CHECKING-only BashJob import. Ruff wants the quotes gone, but removing them would NameError at module load. - Replace `assert mother is not None and father is not None` with an explicit `if ... raise RuntimeError(...)` in merge_contig (S101). Same type narrowing for mypy, but survives `python -O` which strips asserts. - Widen the local `maternal_alt` / `paternal_alt` annotations to `str | None` before the branching assignment so mypy accepts the fallback path (union-attr on the previously-inferred `str`). - Rewrite `_next_contig`'s heads filter so the None-narrowing applies to the actual value used, not just to iterators. - Add `# noqa: PLR0915` on `merge_contig` - the two-pointer sweep is legitimately long-for-good-reason and splitting hurts readability. - Auto-format changes from ruff-format across several files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Stage 2's PED and manifest paths are fixed (downstream tooling expects stable locations), but their *content* is a function of the multicohort's SG set + qualifying family set. Relying on the fixed paths alone for cpg_flow's REUSE check meant Stage 2 would silently ship stale content whenever families were added, removed, or newly qualified. - expected_outputs now returns a third key `family_set_marker` whose filename encodes a SHA256 hash of `<sorted SG IDs> ++ <sorted qualifying family IDs>`. Any change to who's in the multicohort or which families qualify invalidates the marker path and forces the stage to QUEUE. - New jobs.GenerateSyntheticProbandCombinerInputs.create_family_set_marker_job is a tiny Batch job that touches the marker only *after* the PED and manifest jobs have both succeeded, so the marker's existence really does mean "the fixed-path outputs are fresh". Scope: only affects the synthetic proband pre-workflow. Neither first_workflow nor full_workflow references Stage 2, so their runs are unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Johnnyassaf
approved these changes
Aug 19, 2026
Johnnyassaf
left a comment
Johnnyassaf
left a comment
Contributor
There was a problem hiding this comment.
Its looking good and the test cases have passed:
https://batch.hail.populationgenomics.org.au/batches/1135760
I guess the next step here is your merge gvcfs into vdfs, any chance we can use the already available CombineGvcfsIntoVds step and then the CreateDenseMtFromVdsWithHail steps?
Replace the old duo-only rule (which required exactly 2 SGs and no
pedigree-parent references) with tiered selection that no longer consults
affected/phenotype status:
1. Skip families opted out via `synthetic_proband_couples` config
(`skip = true`).
2. Auto-qualify any family with exactly one MALE + one FEMALE SG.
3. Skip singleton families.
4. For every other composition (>2 SGs, or same-sex pairs), require an
explicit `{family_id, mother_sg, father_sg}` entry in
`workflow.<dataset>.synthetic_proband_couples`; warn+skip if absent
so the family surfaces in the log.
Affected status is unreliable as a selection signal because collaborators
may re-label it in metamist after the fact. This also lets us handle
families with additional non-parent members (e.g. a 4-person family with
the affected female's mother and sister) by naming the parental pair
explicitly in config.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Split get_families_for_synthetic_probands' tier-1-to-4 selection out of the outer function into _select_couple_for_family, which further delegates the two distinct paths (auto-qualify a 1M+1F duo vs. resolve an explicit config entry) into _couple_from_auto_duo and _couple_from_config. Same behaviour, same log lines - PLR0915 (too many statements) and PLR0911 (too many returns) both satisfied structurally rather than via # noqa. Also drop the local variable annotations on two BashJob collections in GenerateSyntheticProbandGvcfs so UP037 stops firing on them; the container types are still pinned by the function return annotations, and mypy infers the locals from usage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
MattWellie
reviewed
Aug 28, 2026
EddieLF
commented
Aug 28, 2026
EddieLF
left a comment
EddieLF
left a comment
Collaborator
Author
There was a problem hiding this comment.
Mostly looks good Sam, a few important things to address re dataset names and other identifiers first.
Co-authored-by: EddieLF <34049565+EddieLF@users.noreply.github.com>
MattWellie
reviewed
Aug 28, 2026
Six related changes from the PR review, mapped to the reviewers' points:
- Split the two synthetic-proband stages out of stages.py into a new
synthetic_proband_stages.py so the pre-workflow's file doesn't have to
share stages.py with the main workflow's stages (Matt).
- Hash-scope Stage 2's PED and gVCF-manifest into `families-<hash>/`
subdirectories of self.prefix, driven by the SG set + qualifying family
set. The family_set_marker sentinel and its Batch job go away - existing
hash-scoped output paths are now what cpg-flow's REUSE check keys off
(Matt).
- Collapse each Stage's queue_jobs onto a single public factory call, and
make the per-artifact builders module-private with a leading underscore.
Matches the "one factory call per Stage" pattern used everywhere else
in the repo, and makes the stage-scheduling / stage-doing separation
cleaner if this ever gets ported to another orchestration language
(Matt).
- Rewrite Stage 2's per-artifact builders as driver-side writes via
Path.open('w'), gated on workflow.dry_run. Deletes the two thin Bash
jobs that only ran `cat > $output <<HEREDOC` and the heredoc-quoting
hazard that went with them (Eddie + Matt).
- Rewrite scripts/register_synthetic_gvcf_analysis.py to call
cpg_utils.metamist_registration.create_new instead of hand-rolling the
AnalysisApi calls. Uses cpg_utils.config.dataset_for_access_level to
resolve the metamist project name against access level, drops the
bespoke find_existing_registration + deactivate_analysis idempotency
layer (cpg-flow's REUSE + our per-family sentinel is enough - matches
what every other analysis_type-decorated stage in the repo does), and
picks up the newer `outputs` block shape for free (Matt, Eddie).
- Remove the last inline `ravenscroft-rpl` reference from utils.py
(Eddie's steer earlier in the review).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Yep, i think so. We use those stages then create a new stage after that replaces AnnotateCohort which will AnnotateFromGlobalCallset, bascially replacing all the cohort level metrics with real data, untainted by the synthetic samples. |
MattWellie
self-requested a review
September 2, 2026 00:44
6 tasks
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
4 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.
Purpose
Proposed Changes
Checklist