Milab-6720: variantkey modality - #2
Merged
Merged
Conversation
Four independent things stopped an imported set from Import VDJ Data, and they fail at four different points — so fixing only what a user reported would fix one of them. MODEL, inputOptions. The dropdown filtered candidates down to those with a discoverable per-record pl7.app/vdj/chain column. An imported set has none, so it was never offered: the failure was invisible, with no error to report. Its locus is a property of the whole set and lives on the key axis, so that is accepted as an alternative — pl7.app/vdj/chain for a single mapped chain, pl7.app/vdj/receptor for a paired one. MAIN.TPL, alphabet. Read from pl7.app/alphabet on the key axis, else from the <key>/structure domain key. An imported set carries neither: the structure key belongs to the scClonotypeKey vocabulary its axis no longer uses, and no variantKey producer emits one. The run died on "Cannot determine CDR3 alphabet". Every sequence and region column an imported set emits is pl7.app/alphabet: aminoacid, so that is the answer. SCORE.TPL, locus. addUnit required a per-row chain column and returned early without one, leaving units empty — an empty p-frame and a null progress resource, which the model reads as not-running. So the block sat there having silently scored nothing. A unit whose locus is constant now carries it as "=LOCUS". SCORE.TPL, arrangement. isSingleCell keyed off the axis being scClonotypeKey. A paired imported set carries both chains in one frame under the scClonotypeChain column domain but on a variantKey axis, so it took the bulk path, where firstWithDomain returns the first CDR3 column and the second chain is dropped with the result still looking complete. It now widens from the column domain, gated on the axis being variantKey, testing /index because that is what the scSlot domains match on. SOFTWARE. --chain-column accepts "=LOCUS" and materialises it as a constant column before anything else runs, so group_by, the join and the unsupported-chain report are untouched. Args is a frozen dataclass, so the resolved names travel as a local rather than being assigned back. Verified against real polars: literals become constant String columns, plain column names pass through with nothing added, and the two forms mix. The light chain is deliberately not scored. OLGA needs IGK and IGL as separate models and an imported set records only IGLight — CHAINS in bare-set-specs.lib.tengo has no kappa or lambda — so the locus is unknown, not merely unstated. It is passed through as IGLight, which is not in CHAIN_MODELS, so the scorer skips it, leaves Pgen null and names it in the "skipped: " line the model's skippedChains output already reads. No new warning machinery. IGHeavy, TCRAlpha and TCRBeta resolve to IGH, TRA and TRB and are scored normally. A NEW table rather than reuse of SC_CHAIN_LABELS. Its two TCR rows are inverted against MiXCR's convention, which fixes A as the more diverse chain and so orders TCRAB as TCRBeta/TCRAlpha (mixcr-clonotyping/workflow/src/process.tpl.tengo:43,695). Today that is only a labelling bug, because the locus comes from the per-row column; reusing the table here would have picked the wrong OLGA model and produced wrong numbers. Imported units take their label from the resolved chain for the same reason, so their labels agree with their loci. The existing inversion is left alone — correcting it changes user-visible labels on single-cell TCR datasets and belongs in its own change.
build.yaml and mark-stable.yaml, matching import-vdj-data (the producer this block now reads) and the other consumers adapted under this ticket. prepublishOnly checked and left alone: it is byte-identical to import-vdj-data, antibody-tcr-lead-selection, clonotype-space, rarefaction, repertoire-score and immune-assay-data, and carries no redundant pack step. No changeset: a build-time concern that changes nothing in the published packages.
The shared v4 CI gates publication on block-tools being at latest (node-simple-pnpm.yaml, "Check infrastructure requirements for publication"), and it passes only block-tools to require-latest — model and ui-vue are free to lag. Version bump only, no structure refresh. Both build paths verified green at the new version, software included, so the 2.13 build script rename and the local-software regression do not bite here. Note: 'block-tools structure check' now hard-fails this block for having no sibling kind/ package. That gate is not wired into any script or into CI (build.yaml runs build:dev-local and test), so it does not block publication — but the block does now formally owe a kind, whose BlockParams contract is an author decision. No changeset: a build-time concern that changes nothing in the published packages.
Review point: the locus for a unit with no per-row chain column can be materialised inside the block, so the scoring script does not need to know about constants at all. Replaces the '=LOCUS' marker and its resolve_chain_literals() handler in main.py with a pt step that adds chain_<suffix> as a literal over the built parquet. software/src/main.py is now byte-identical to main again, which drops the software package from the changeset: no version bump, and no image rebuild for a change that is only about where the locus comes from. Given how much friction rebuilding and republishing software just cost, that is the larger win here. The step is gated on a literal actually being needed, so bulk and single-cell inputs keep exactly the graph they had — no extra pt run, no extra parquet pass. Only an imported set pays the extra read/write, and it is the case that could not run at all before. The locus derivation itself is unchanged: bareChain() plus CHAIN_LOCUS still resolve IGHeavy/TCRAlpha/TCRBeta to IGH/TRA/TRB, and IGLight still passes through unresolved so the scorer skips it and names it in the skipped-chains report. My earlier claim that no Tengo-only route existed was wrong — I checked the xsv/parquet file builder, which takes PColumns only, and never checked pt.
MiXCR fixes "A" as the more diverse chain, so TCRAB orders TCRBeta/TCRAlpha and TCRGD orders TCRDelta/TCRGamma (mixcr-clonotyping/workflow/src/process.tpl.tengo:43,695). SC_CHAIN_LABELS read A as Alpha and B as Beta, and gamma/delta the same way, so a single-cell TCR alpha/beta dataset labelled its alpha column "(Beta)" and its beta column "(Alpha)". The values were never wrong — the locus is read from the per-record pl7.app/vdj/chain column, not from the A/B letter — but a mislabelled column reads as the other chain's result, which is worse than a missing label. Fixed by deleting SC_CHAIN_LABELS rather than correcting it. The same A/B-to-chain knowledge was written twice, in that table and in RECEPTOR_CHAINS, and having two copies is what let them drift apart; one of them was bound to be wrong. chainLabel now resolves the slot through RECEPTOR_CHAINS and names the result with CHAIN_LABELS, so the label and the OLGA model come from one source and cannot disagree. That also removes the reason the imported-set branch sets its own label, but it is kept: it takes the label from the very chain value the locus was derived from, which is a stronger guarantee than deriving both from the same table separately.
kevindetry-milaboratories
requested changes
Aug 27, 2026
| // that is the answer rather than a failure. | ||
| if is_undefined(alphabet) && keySpec.name == "pl7.app/variantKey" && | ||
| !is_undefined(keyDomain["pl7.app/vdj/clonotypingRunId"]) { | ||
| alphabet = "aminoacid" |
Collaborator
There was a problem hiding this comment.
I still can't see how we can simply assume that alphabet is always aminoacid. Why can't the producer properly annotate their column?
If we can't patch a producer now, then I suggest asking slopus whether Olga can detect alphabet on its own and use that functionality (just make sure it's reasonably efficient). And then make alphabet param optional in Python.
| if !isSingleCell && keySpec.name == "pl7.app/variantKey" { | ||
| for col in cdr3Cols { | ||
| colDomain := is_undefined(col.spec.domain) ? {} : col.spec.domain | ||
| if !is_undefined(colDomain["pl7.app/vdj/scClonotypeChain/index"]) { |
Collaborator
There was a problem hiding this comment.
ask slopus if there's null chaining in tengo
Contributor
Author
There was a problem hiding this comment.
no, only though !is_undefined()
| // report, which the model already surfaces. | ||
| CHAIN_LOCUS := { | ||
| "IGHeavy": "IGH", | ||
| "IGLight": "IGLight", |
Collaborator
There was a problem hiding this comment.
maybe instead of inventing non-existent locus we can simply skip the column?
| } | ||
| } | ||
| if len(literalUnits) > 0 { | ||
| ptWf := pt.workflow().mem("4GiB").cpu(1) |
Collaborator
There was a problem hiding this comment.
Can we just use the same pt workflow from above instead of creating a new one?
| } | ||
|
|
||
| // Preferred: the per-row locus column. An imported set has none, so fall back to the | ||
| // unit's single mapped chain and materialise it as a constant column below. Without a |
Collaborator
There was a problem hiding this comment.
please no British English
…name
CHAIN_LOCUS claimed 'IGLight' was a locus so that the scorer would skip it. It is not a
locus, and a table of loci that contains a non-locus invites a reader to trust it.
The map now holds only names OLGA could recognise, and a chain with no entry sends its own
name instead. The scorer has no model under that name either, so the outcome is unchanged —
null score, and the name in the 'skipped: ' line that MainPage.vue renders as a banner
('no recombination model for: IGLight'). The behaviour is identical; only the lie is gone.
Falling through rather than dropping the unit is deliberate: the banner exists only because
the scorer sees the chain. Returning early in tengo would leave the light chain absent with
nothing on screen to say it was there, which is worse than an empty column.
Only IGLight is affected. Every other imported chain has a real locus, including TCRGamma
and TCRDelta: TRG and TRD are genuine loci that OLGA ships no model for, so they belong in
the map and are skipped one level further down.
Also drops a temp map in the arrangement scan in favour of the short-circuit the SDK uses
itself (pframes/xsv-builder.lib.tengo:123). Tengo has no optional chaining and maps.lib
exports no safe-get, so this is as short as it goes.
The loop above tableBuilder already visits every unit, so gathering the pt expressions there removes a second pass over units and the literalUnits list it existed to produce. It also states the invariant in the code rather than in a comment: a unit either has a chain column, which goes into the table, or a constant locus, which is queued as a literal — the if/else makes that exclusive by construction, where two separate loops left it implicit. No behaviour change; the pt pass still runs only when a literal exists.
The pt pass repeated tableBuilder's cpu(1)/mem("4GiB"), so a change to one would silently
leave the other behind. Both are column shuffles over the same table, which is the reason
the numbers match — now stated once, where a reader sees it, instead of twice as bare
literals.
parquetFileBuilder assembled the table and a second pt pass appended the constant chain column, so the whole table was written, read and rewritten for the sake of one literal. pt can source the columns itself — frameFromColumnBundle joins bundle columns on their common axes — so the builder is gone and there is one step, one resource budget, one pass. The renames that setAxisHeader and the per-add headers used to do are now a select(): frameFromColumnBundle names the axis by pSpec.getAxisId and each column by its bundle key, so the select maps those to clonotypeKey and cdr3_/chain_<suffix>. The literals are appended with withColumns afterwards, still only when a unit has no chain column. TABLE_CPU/TABLE_MEM are dropped with the second builder; a single call needs no constant. Two things this deliberately gives up, both accepted on request: The pt path is no longer gated. It previously ran only for imported sets, leaving bulk and single-cell on the long-proven parquetFileBuilder; now every input shape is assembled by pt. That is the price of one pass, and it widens what a pt-side mistake can break. frameFromColumnBundle has no other consumer in the workspace — no block uses it — so this is its first use here. pSpec.getAxisId producing exactly the name the frame carries for the axis is the part most worth checking on the first real run.
This reverts commit d9f22a2.
Second attempt. The first (d9f22a2, reverted in f963a97) used frameFromColumnBundle, which json.decodes each pool key — and an addMulti result is keyed "<resultKey>.<canonical ref>" (pframes/bundle.lib.tengo:399), which is not JSON. The decode returned an error, indexing it raised "invalid index on error" at pt:770, and the block failed at graph construction. This names the columns itself with the layer underneath: pt.p.column(name, {spec, data}) per bundle entry, joined with pt.p.full, the key axis aliased through pt.sc.axis(keySpec). The json.decode step is never reached, because no pool key is passed to pt at all. parquetFileBuilder and the second pt pass are gone, so the table is written once rather than written, read and rewritten to append one constant. The literals still go on with withColumns, and still only when a unit has no chain column. Note this puts every input shape through pt, where the previous version left bulk and single-cell on parquetFileBuilder and used pt only for imported sets. That is inherent to one pass, and it is why the two-pass version was worth running first: the adaptation is now known good against real data, so anything that breaks here is the assembly. Also merges the two changesets into one, and fixes a real error in the one being merged: the label fix named '@platforma-open/milaboratories.generation-probability', a package that does not exist. The facade is '.block'. Left alone it would have bumped nothing.
They existed to share one budget between parquetFileBuilder and the pt pass. The single-pass assembly has one call site, so they were left declared and unreferenced by the splice that replaced their only users. The budget is inline on the pt workflow now; a lone call needs no constant.
…mption The imported-set branch hardcoded "aminoacid" on the grounds that such a set only carries amino-acid sequences. True today, and an assumption where a fact was available: the key axis does not state the alphabet but the sequence columns do, in pl7.app/alphabet. main.tpl keeps reading the axis and its structure key, and now passes "" rather than panicking when neither says anything. score.tpl finishes the job, because that is where the columns are in hand, taking the alphabet off the CDR3 columns and refusing on either no column carrying one or columns that disagree. The axis stays authoritative wherever it speaks, which is the invariant that matters: a MiXCR set emits both nt and aa CDR3 columns, and the axis is what says which defines the clonotype. Deriving from the columns there would be ambiguous, so it happens only on the silent path. Also adds the ll import score.tpl was missing before it could panic.
…entinel The alphabet was resolved in main.tpl from the key axis and passed to score.tpl, so once the columns became a third source the value had to cross a template boundary half-resolved — and "" stood for "the axis did not say", a sentinel that meant nothing to either side alone. score.tpl already receives keySpec, so it can do the whole thing: axis domain, then the structure key, then the columns, then refuse. main.tpl no longer mentions the alphabet at all and alphabetFromStructure moves with the logic that uses it. is_undefined carries the "not yet known" state the way tengo already does it, so there is no sentinel value. Behaviour is unchanged for every input: the axis still wins wherever it speaks, which is what keeps a MiXCR set scoring the CDR3 its clonotype key was built from rather than whichever alphabet a column happened to carry. The refusal is now one message naming both sources instead of two naming one each, since at that point neither has produced anything.
The scorability check read axesSpec[1]. Which index the key axis sits at is a property of the producer, not something a consumer can assume — main.tpl.tengo searches its ENTITY_KEY_NAMES for it rather than indexing, and the model now does the same through keyAxisOf. The selector list is derived from that same array, so the three axis names are written once. They were previously spelled out in the selectors and again in the check, which is how the two would have drifted the first time a fourth key axis appeared.
oxlint dropped 'as const' and the readonly cast it forced. Without the literal tuple the array is string[], which is all .includes() and the selector map need, so the cast was work the type was creating for itself. Applied by the build's check step, not by hand.
kevindetry-milaboratories
approved these changes
Aug 27, 2026
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.
Greptile Summary
The PR adds generation-probability scoring for imported receptor sets keyed by
pl7.app/variantKey, including paired-chain selection, constant-locus arguments, and amino-acid inference. It also updates the build environment and block tooling dependency.variantKey— Platforma key axis used by imported receptor records. The model now accepts eligiblevariantKeydatasets, and the workflow recognizes their metadata and sequence namespace.IGH,TRA, orTRB. The workflow maps imported receptor-chain identities to loci and passes them as=LOCUS.scClonotypeChaindomain.variantKeysets with clonotyping metadata now default to amino acid when no explicit alphabet source exists.IGLightremains unscored because its metadata cannot distinguish IGK from IGL and is reported through the existing skipped-chain mechanism.@platforma-sdk/block-tools— Workspace build and packaging tooling. It is upgraded from 2.12.9 to 2.14.3, with corresponding lockfile changes.Confidence Score: 5/5
The PR appears safe to merge based on the reviewed changes, with no concrete changed-code defect established.
The model, workflow, process template, and Python runtime consistently carry imported receptor loci from key metadata through constant chain columns, while unsupported light chains retain the existing nullable-score and reporting behavior.
Important Files Changed
=LOCUSargument for each scoring unit.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Imported variantKey dataset] --> B[Model eligibility check] B --> C[Resolve amino-acid alphabet] C --> D[Discover CDR3 chain slots] D --> E[Resolve locus from key metadata] E --> F[Pass sequence and =LOCUS arguments] F --> G[Materialize constant chain columns] G --> H[Calculate Pgen with OLGA] H --> I[Publish typed score and export frames]Reviews (1): Last reviewed commit: "MILAB-6720: bump block-tools to 2.14.3" | Re-trigger Greptile
Context used (5)