Repository navigation
Restore the guards dropped in the interface refactor — Closes #6 - #8
Merged
Merged
Conversation
nvictus
approved these changes
Sep 17, 2026
nvictus
left a comment
nvictus
left a comment
Member
There was a problem hiding this comment.
lgtm besides one docstring comment I don't understand
| Frozen. The `coordinate_system` cached on it is only sound because the | ||
| field it reads cannot change underneath it. | ||
|
|
||
| Derive a variant with ``model_copy(update=...)``, bearing in mind that it |
Member
There was a problem hiding this comment.
Not following. Is this a todo suggestion?
Contributor
Author
There was a problem hiding this comment.
This is just usage guidance. I can make it more clear.
conradbzura
force-pushed
the
6-restore-dropped-guards
branch
from
September 25, 2026 14:05
bdd7a41 to
200c751
Compare
conradbzura
force-pushed
the
6-restore-dropped-guards
branch
from
September 25, 2026 18:07
cc8c596 to
d727f52
Compare
The facade listed Canvas, a name no submodule binds, so a star import raised AttributeError, and it omitted TileCanvas, which it imports. Nothing caught either: pyproject switches pyflakes' unused-import check off for __init__.py, which is the one rule that would have noticed. It also never exported TileKind, AnnotationRecord, Annotation2DRecord or ErrorTilePayload, although Tileset.tiles and ProvidesRegions.regions are annotated with them, so a third party implementing an exported protocol could not name its own return type.
A negative zoom or position reached Chromsizes.invert and raised a plain ValueError, which is not a TilesetError, so the server boundary could not render it and one malformed id took the whole batch down. Coordinates are now ASCII decimal only. Both of the obvious spellings are wrong in opposite directions: int accepts +5, 1_0 and fullwidth digits, each of which denotes one tile under several distinct request strings, while str.isdigit admits superscripts that int then rejects with the same unrenderable ValueError. A declared arity below one is reported as TilesetUnavailable rather than MalformedTileId. The arity is the tileset's own declaration and not part of the request, so a client told its well-formed id was malformed retries forever against a fault only the server can fix.
TileCanvas is re-exported from clodius.core and can be constructed directly, and its arguments have no valid degenerate case: a binsize or tile size of zero divides by zero one property later. The binsize check is written as a negated comparison so a NaN is refused rather than passed. The tile position is checked in tile_span as well as in invert. A tileset working in absolute coordinates may reach only the former, and the second axis of a 2D tile is screened nowhere else, so an off-lattice position yielded a well-formed range far past the genome, zero rows, and a tile a client cannot tell apart from one that is genuinely empty.
take_most_important implements its limit as ranked[-cap:], which at zero is the whole list, so a server configured to serve no records emitted an unbounded tile: the precise failure the cap exists to prevent. It now also accepts None, which is what TilePolicy supplies when max_records is unset. TilePolicy validated nothing, and a negative max_records meant four different things at the four sites that read it. Two read it as serve nothing; polars top_k raised OverflowError, which is not a TilesetError and so escaped the per-tile boundary and failed the batch. Validating in the constructor puts the rule in the one place all four pass through. Selection uses a heap rather than a full sort, for the reason bed.py already gives for using top_k: the working set is bounded by the cap instead of by how many records the tile covers. On tied importances the heap keeps the earlier record where the sort kept the later one. stable_importance passes usedforsecurity=False so the thinning path keeps working on a FIPS-enforcing build, where md5 is otherwise refused before the flag downstream gets a chance to help.
clodius/core/errors.py documents a TileError as caught by tiles() and returned as that tile's payload, leaving the rest of the batch intact. Three tilesets implemented that and eight raised straight through. The base class now owns it, so a tileset inherits the contract instead of restating it, and the two that override tiles() do so for a reason rather than by duplication. A tileset inheriting the default reads no batch options, so a non-empty options mapping is refused rather than discarded. The protocol makes a malformed option a whole-batch failure because options arrive once for every tile at once, and serving tiles that ignore what was asked for is the worse of the two answers. TilesetInfo and DatasetInfo are frozen: the coordinate system cached on an info is only sound while the field it derives from cannot be rebound. The freeze does not reach inside the list, so mutating chromsizes in place is unsupported rather than prevented. with_ derives a variant by re-running the constructor, which model_copy does not, and rebuilds the cache instead of carrying a stale one across. A misspelled key is not caught by either route, since the models allow extra fields. ndim is annotated without being assigned. An inherited default would let a subclass that forgot to declare its arity serve tiles at someone else's; the absence is reported from parse_tile_id as the server-side misdeclaration it is.
Three tilesets carried the same two-line comment over near-identical expressions testing whether a row names a contig the chromsizes carry. One helper covers all three, bedpe's two-column conjunction included. The second helper answers a different question that no code was asking: a tabix index lists only the contigs it saw records on, which is not the set a query region is derived from. Both live in tiles_v2 rather than beside Chromsizes in core, because nothing under core imports polars or pysam and making the protocol layer cost those imports is the worse trade.
…g it Three tilesets carried their own copy of the loop that converts a TileError into that tile's payload, and two of them carried a version with no try at all, so a refusal propagated and failed the batch. The base class owns the boundary now, so the overrides go. Deleting them also brings the option refusal with it: these three declare that they read no batch options and previously accepted and discarded them.
The precomputed-tile schema answers a tile from a stored assignment rather than a range query, and that arm reached no bounds-checked accessor at all. Its tile id is 2**z - 1 + x, the breadth-first index of a node in a complete binary tree, which is not injective over a position that does not exist: an off-lattice x landed on a different node and served that node's intervals. The result is populated, well formed and wrong, which is worse than an empty tile because nothing about it looks like a failure. The zoom was unchecked on the same line, so 2**z ran on a request-controlled exponent and raised a bare ValueError past Python's integer-conversion limit. Not a TileError, so one bad id failed the batch. Hoisting the canvas lookup above the branch gives both arms the bound and makes the range arm's own call redundant rather than load-bearing. The local tiles() override goes too, since the base class now owns the per-tile boundary.
Four guards, shared by the BED, BEDPE and VCF tilesets. A record whose contig the chromsizes omit has no position on the genome-spanning axis. The converters dropped it through a bare KeyError, which is not a TileError and so reached a client as a 500; they now return None and say so in their return type, rather than three call sites each remembering to test membership first. A tile lying past the end of the genome is routine, since the quadtree extent always exceeds the genome length. oxbow reads an empty region list as no region filter at all, so the indexed path served the entire file for exactly the tile that should serve nothing, while the scanning path's pushed-down predicate correctly matched nothing. The two paths agreed on compressed and uncompressed copies of the same file only by accident. The scan ceiling refuses an unindexed file too large to read end to end. It was applied to indexed BEDPE files as well, which read only their own byte ranges, so a properly indexed file was refused with a message saying it must be indexed. Query regions are derived from the chromsizes, but a tabix index lists only the contigs it saw records on. A file whose records sit on one chromosome, served against a whole assembly, asked its index for contigs it had never heard of and the reader raised through polars as a ComputeError, failing the batch on ordinary input. Screening the regions removes only queries guaranteed to match nothing. Paging through regions filters before slicing, so the offset, the limit and the next-page probe all range over the same population; filtering afterwards let one unplaceable row consume the probe and made consecutive pages overlap.
A cap of zero was enforced at the end of the pipeline, after every record had been fetched and digested: seven seconds of work and 3.59 million md5 digests to serve an empty list. It is refused before the read now, with the downstream guard kept as the backstop. to_bedlike took a pre-resolved chromosome offset and left the lookup at the call site, where a contig outside the chromsizes raised KeyError past the per-tile boundary. It takes the offsets map and returns None, matching to_interaction in the same file. The info for each alternate chromosome ordering is built once at construction rather than per tile. The alternates are fixed when the tileset is created, so the memo is bounded by configuration and cannot grow with request volume; rebuilding ran a full quadtree validation and handed back a cold coordinate system for canvas() to rebuild. Record digests pass usedforsecurity=False for the same reason stable_importance does.
Thinning counted genes by the id the dialect reads off the row, but a GFF3 gene row is not required to carry an ID attribute. For a file whose gene rows have none the ledger stayed empty, the under-the-cap comparison passed every row through, and gene assembly then synthesized an id for those same rows and emitted them all. Every positive cap was bypassed: five hundred records came back under a cap of five. Thinning the linked genes instead makes the cap range over exactly the objects that become records, so the two stages agree by construction rather than by keeping two id derivations in step. The transcript and child walk goes with it, since the genes already own their children. A non-positive cap is refused before the read rather than after the tile has been scanned and linked.
The constructor collapsed a falsy policy to None, the same construct the tile options slot was fixed for. The Tileset protocol declares policy as a TilePolicy, not an optional one, so the runtime handed back a value contradicting the published protocol while the runtime-checkable isinstance still passed, since it tests only that the attribute is present. Every read of it was then an AttributeError, which is not a TilesetError and escapes the boundary as a 500. Row metadata is passed to the info constructor rather than assigned afterwards. The model is frozen, and assigning to it raises a pydantic error that is not a TilesetError either; that failure lands in info(), which tiles() calls on its first line, so it would take every tile with it. This tileset keeps its own tiles() override, because it parses a batch-wide row aggregation once rather than per tile, so it carries its own copy of the per-tile boundary and its own refusal of option keys it does not read.
…atch Tiles are batched per zoom level and transform, since those decide which cooler and which weights a tile is read from. A zoom past the ladder or a transform the file does not carry raised for the whole batch, although both are properties of one requested id rather than of the file; they now land in the payload slot of the tiles that asked for them, and a batch with nothing servable opens no reader at all. A record on a contig the chromsizes omit yields NaN rather than raising, so an absent contig does not disturb its neighbours in the same tile. This tileset overrides tiles() to share one reader across a batch, so it restates the option refusal the base class would otherwise have given it.
Every guard restored in clodius/core ships with a test verified to fail against the unfixed source rather than merely to pass. The error taxonomy is asserted directly. BaseTileset.tiles catches exactly TileError and converts it into that tile's payload; everything else propagates and fails the request. So a class's parent decides whether a failure costs one tile or all sixteen, and reparenting one would change that with nothing else noticing. Zoom and position zero are pinned because every other success case uses a non-zero zoom: writing either negative guard as less-than-or-equal would pass the whole suite while refusing the top tile of every tileset. The unnormalized id spellings are pinned for the same reason, since the parser admits a negative zero deliberately. The runtime-checkable protocol is exercised, because it is what justifies annotating ndim without assigning it: two tilesets alike but for that attribute must answer differently. Fixtures are synthesized into a temp directory, so the suite runs on a checkout with no git-LFS payload.
Sixteen test modules over eleven tilesets, where four modules existed before. Each guard is pinned by a test confirmed to fail against the unfixed source, and the controls that keep those tests from passing vacuously are named as such in their docstrings. Two are worth calling out. The bigInteract fixture emitted thirteen tail fields where the interact schema has fifteen, so every record was two short of what the converter accepts and the entire interaction half of the bbi module had never been executed by a test; repairing it is what makes the link policies testable at all, and a tile is chosen where all three policies give different answers. And the beddb suite covers three schema versions that reach one answer by three different queries, where only the fourth variant had any coverage. Fixtures are synthesized with pybigtools, pysam, h5py and sqlite3 into a temp directory rather than committed, so the suite runs on a checkout with no git-LFS payload.
One entry per guard, under Unreleased. The entries state what each guard refuses and why refusing it matters, rather than naming the symbol that changed, because the shared cause is not visible from any one of them: a failure that is not a TilesetError escapes the per-tile boundary and fails the whole request.
conradbzura
force-pushed
the
6-restore-dropped-guards
branch
from
September 26, 2026 23:19
840da67 to
6639612
Compare
conradbzura
marked this pull request as ready for review
September 28, 2026 18:00
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
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.
Description
Restore the guards that the
tiles_v2interface refactor dropped, and close the gaps that reviewing them turned up.The refactor moved eleven tilesets onto a shared interface and lost a set of defensive checks on the way. The pattern behind every one of them is the same: a failure that used to be refused now escapes as something the server boundary cannot render.
clodius/core/errors.pystates the contract — aTileErroris caught bytiles()and returned as that tile's payload, while anything else propagates and poisons the whole response — so aValueError, aKeyErroror anOverflowErrorraised anywhere under_tileturns one bad tile into a failed request.Fixes #6
Serve the per-tile boundary from one place
Three tilesets implemented the boundary and eight raised straight through the batch.
BaseTileset.tilesnow owns it, and nine tilesets inherit it. The two that overridetiles()do so for real reasons — cooler batches per zoom and transform behind a shared reader, multivec hoists row-aggregation parsing out of the loop — and both now carry the boundary themselves.Batch options are refused rather than discarded. A tileset that reads none used to accept an
optionsbody and serve tiles that ignored it; options arrive once for every tile at once, so a malformed one fails the request.Restore the guards
str.isdigitadmits superscripts thatintthen rejects andintitself admits+5,1_0and fullwidth digits — each of which denotes one tile under several request strings. Report a misdeclared arity asTilesetUnavailable, because the arity is the tileset's own declaration and a client told its well-formed id is malformed retries forever against a fault only the server can fix.TileCanvasconstruction, and check the tile position intile_spanas well asinvert. A tileset working in absolute coordinates may reach only the former, and the 2D bigInteract tile'syaxis is screened nowhere else.take_most_importanttotal over the integers — a cap of zero readranked[-0:], which is the whole list, so a server configured to serve nothing emitted an unbounded tile. Apply the cap in the variant tileset, which never readmax_recordsat all, and range it over linked genes in the gxf tileset, where counting rows missed every gene whose row carries noID. Refuse a zero cap before the read rather than after it.TilePolicya validating constructor. A negativemax_recordsmeant four different things at the four sites that read it, and one of them raisedOverflowErrorpast the boundary.Screen query regions against the index
A tile's query regions come from the tileset's chromsizes, but a tabix index lists only the contigs it saw records on. A BED, BEDPE or VCF whose records sit on one chromosome, served against a whole assembly, asked its index for contigs it had never heard of, and the reader raised through polars as a
ComputeError— failing the batch on entirely ordinary input, while the scanning path served the same tile correctly.clodius/tiles_v2/_index.pyreads what the index carries and drops the regions it cannot answer, which removes only queries guaranteed to match nothing.Publish a coherent info model
Freeze
TilesetInfoandDatasetInfo, since the cached coordinate system is sound only while the field it derives from cannot be reassigned, and addwith_as the derive path that re-validates and rebuilds the cache wheremodel_copydoes neither. Export the payload types the published protocols name, so a third party implementingTilesetcan annotate its own return type.Test the layer
test/coreandtest/tiles_v2go from four test modules to nineteen, and 365 tests now cover them. Every restored guard ships with a test verified to fail against the unfixed source rather than merely to pass.Two findings worth naming. The
interact_recordfixture emitted thirteen tail fields where the interact schema has fifteen, so every bigInteract record was two fields short of what the converter accepts and the entire interaction half ofbbi.pyhad never executed — repairing it is what makes the link policies testable at all. Andtest/core/test_errors.pyasserts the error taxonomy directly, because a class's parent decides whether a failure costs one tile or the whole batch and nothing pinned that.