Repository navigation
[query] Key liftovers and reference sequences by reference genome value - #15877
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Attachment model and resource lifecycle hail/hail/src/is/hail/io/reference/RgAttachment.scala, hail/hail/src/is/hail/io/reference/RgAttachments.scala, hail/hail/src/is/hail/io/reference/Liftover.scala, hail/hail/src/is/hail/io/reference/ReferenceSequence.scala, hail/hail/src/is/hail/variant/ReferenceGenomeEnv.scala, hail/hail/test/src/is/hail/io/reference/RgAttachmentsSuite.scala |
Adds RgAttachment and RgAttachments, moves liftover and sequence behavior into attachment implementations, and removes the previous standalone implementations. ReferenceGenomeEnv.get looks up user-defined and builtin genomes. Tests cover attachment lookup, duplicate handling, and removal. |
Execution context and driver integration hail/hail/src/is/hail/backend/ExecuteContext.scala, hail/hail/src/is/hail/backend/driver/BatchQueryDriver.scala, hail/hail/src/is/hail/backend/driver/Py4JQueryDriver.scala, hail/hail/test/src/is/hail/HailExtension.scala, hail/hail/test/src/is/hail/backend/driver/BatchQueryDriverSpec.scala |
Execution contexts and query drivers replace separate liftover and sequence maps with RgAttachments. Batch loading skips configured attachments when their genomes are out of scope. Tests cover that loading behavior. |
Compiled attachment delivery and validation hail/hail/src/is/hail/expr/ir/Compile.scala, hail/hail/src/is/hail/expr/ir/EmitClassBuilder.scala, hail/hail/src/is/hail/expr/ir/functions/*, hail/hail/src/is/hail/annotations/*, hail/hail/src/is/hail/expr/ir/CompileAndEvaluate.scala, hail/hail/src/is/hail/rvd/AbstractRVDSpec.scala, hail/hail/test/src/is/hail/variant/ReferenceGenomeSuite.scala, hail/hail/test/src/is/hail/expr/ir/* |
Generated functions register, restore, and receive ordered attachments through one interface. Cache keys include ctx.attachments. Tests cover attachment shipping, identity, deduplication, cache behavior, and array-length validation. The changes also update typed reference-genome lookups and include row-decoding and call-site adjustments. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant CompileImpl
participant EmitModuleBuilder
participant RgAttachments
participant FunctionWithRgAttachments
participant RgAttachment
CompileImpl->>EmitModuleBuilder: compile IR using the execution context
EmitModuleBuilder->>RgAttachments: resolve typed liftover or sequence
EmitModuleBuilder->>FunctionWithRgAttachments: provide ordered attachment array
FunctionWithRgAttachments->>RgAttachment: restore with filesystem and local temporary directory
FunctionWithRgAttachments->>FunctionWithRgAttachments: initialize with addRgAttachments
Possibly related PRs
- hail-is/hail#15875: Both changes update the
ReferenceGenomeEnvcontract used when resolving reference genomes.
Suggested reviewers: ehigham
Merge Risk: 
Minimal · up to 140ed
-
No actionable merge-blocking issue is established; the change is mergeable after normal checks.
Security architecture risk: 
Low · up to 11d53
-
The change strengthens separation between reference genomes and their resources. The examined paths preserve access authority and prevent removed resources from reappearing. No introduced security issue was established, but deployment and runtime coverage remain incomplete.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The examined exposure follows configured resource paths into driver-side loading and generated-function restoration. Its effective filesystem scope remains the authority supplied by the execution filesystem; this review did not establish a broader tenant, service, or environment boundary change.
Trust Boundaries and Controls
- observed — The unified restore contract is sealed to the attachment module. Delivery restores internally registered resources using the execution filesystem and captured temporary directory. Arbitrary same-class positional substitution is not independently validated by the generated initializer, but positional trust also existed in the separate base interfaces; no new external caller was established.
Resilience and Maintainability Implications
- observed — Restoration precedes attachment injection and function return, so a failed restore prevents publication of the generated function. Earlier successful restores are not rolled back and can persist into a retry. The separate restore loops at the merge base had the same persistence characteristic; no new authority or cleanup regression was established.
-
Pre-merge checks: 
4
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
833fef0 to
cbc75d3
Compare
cbc75d3 to
c5d5efb
Compare
c675aa5 to
12439c5
Compare
12439c5 to
e8730c1
Compare
e8730c1 to
828aef5
Compare
| * Two of these are equal if they hold the same attachment objects under equal keys. | ||
| */ | ||
| final case class RgAttachments( | ||
| liftovers: Map[(ReferenceGenome, ReferenceGenome), Liftover], |
There was a problem hiding this comment.
A nice to have, maybe:
type Liftover.Key = (..A session's liftovers and reference sequences are now attached to reference genome values rather than names. A reference genome redefined under a removed one's name gets a different key, so it can never reach a liftover whose chain was checked against the old contigs. - Liftover holds its source and destination reference genomes, and its chain is checked against exactly those. ReferenceSequence holds its reference genome. Both implement a sealed RgAttachment trait with restore(fs, localTmpdir), and live together in RgAttachment.scala. - RgAttachments is the session's immutable set of attachments: at most one liftover per (source, destination) and one sequence per reference genome, compared with reference genome equality. Its equality is structural, with attachments compared by identity. - RgAttachments.removeReference removes a reference genome from the env together with its sequence and every liftover from or to it, so re-adding an identical genome starts with no attachments. Previously liftovers to a removed genome were kept. - RgAttachments.removeLiftover is a no-op when either genome is not in scope. Removing an absent sequence still fails. - ExecuteContext carries `attachments: RgAttachments` in place of the separate liftover and sequence maps. The Py4J driver, the Batch driver and the test setup build it through the same API. - At launch, the Batch driver skips, with a warning, any liftover or sequence whose reference genome is not in scope, instead of failing every RPC. Using it then fails when compiling. - CompileCacheKey includes the session's attachments, and the Py4J driver no longer clears the compile cache when they change. - Emitted code looks up liftovers and sequences by the reference genome values its types hold. The module builder's liftover and sequence registries are keyed by value, in insertion order.
An emitted module now registers, ships and restores its liftovers and reference sequences through one registry instead of one per kind. There is no behavior change. - EmitModuleBuilder keeps its reference genome attachments in a single LinkedHashMap keyed by the attachment itself, which compares by identity. Insertion order gives the one ordering used for both the shipped values and the static fields, replacing the two orders that had to agree. - Each static field keeps its attachment's class (Liftover or ReferenceSequence), so generated code using it needs no cast; the adder casts each element once. - FunctionWithRgAttachments.addRgAttachments replaces addLiftovers and addReferenceSequences and their traits. Like addReferenceGenomes, it fails when given an array of the wrong length. - Compiled restores each shipped attachment through RgAttachment.restore in one loop before setting it. - Reference genomes keep their own registry: types and literal decoding reach them, and they must be set before literals are decoded.
Calls that wrapped when they took a HailStateManager stay wrapped, and UnsafeRow loses its unused no-argument constructor. RegistryHelpers gets safeReadAnyRef and unsafeReadAnyRef, wrappers over SafeRow.readAnyRef and UnsafeRow.readAnyRef. Nothing calls them.
828aef5 to
140eddb
Compare
| aggSigs: IndexedSeq[AggStateSig], | ||
| args: Seq[EmitParamType], | ||
| body: IR, | ||
| attachments: RgAttachments, |
There was a problem hiding this comment.
Is this because the code we generate depends on whether there are rg attachments, whose existence is not encoded in the type-system?
ehigham
left a comment
There was a problem hiding this comment.
missing a key: Liftover.Key = in the liftover class.
Looks great!
A session's liftovers and reference sequences are now attached to
reference genome values rather than names. A reference genome redefined
under a removed one's name gets a different key, so it can never reach a
liftover whose chain was checked against the old contigs. Emitted
modules register, ship and restore these attachments through one
registry instead of one per kind.
chain is checked against exactly those. ReferenceSequence holds its
reference genome. Both implement a sealed RgAttachment trait with
restore(fs, localTmpdir), and live together in RgAttachment.scala.
one liftover per (source, destination) and one sequence per reference
genome, compared with reference genome equality. Its equality is
structural, with attachments compared by identity.
env together with its sequence and every liftover from or to it, so
re-adding an identical genome starts with no attachments. Previously
liftovers to a removed genome were kept.
scope. Removing an absent sequence still fails.
attachments: RgAttachmentsin place of theseparate liftover and sequence maps. The Py4J driver, the Batch
driver and the test setup build it through the same API.
sequence whose reference genome is not in scope, instead of failing
every RPC. Using it then fails when compiling.
driver no longer clears the compile cache when they change.
values its types hold. EmitModuleBuilder keeps them in a single
LinkedHashMap keyed by the attachment itself, which compares by
identity. Insertion order gives the one ordering used for both the
shipped values and the static fields.
ReferenceSequence), so generated code using it needs no cast; the
adder casts each element once.
addReferenceSequences and their traits. Like addReferenceGenomes, it
fails when given an array of the wrong length. Compiled restores each
shipped attachment through RgAttachment.restore in one loop before
setting it.
reach them, and they must be set before literals are decoded.
took a HailStateManager stay wrapped, and UnsafeRow loses its unused
no-argument constructor.
Security Assessment
Stack created with GitHub Stacks CLI • Give Feedback 💬