Repository navigation
feat: persist variant genomes with a deterministic ID - #255
Conversation
- variant genomes deterministic id calculated by hashing variants and genome config - persist hash components using data manager (1 file / variant genome) - make VariantGenome equality more strict. previously two variant sets on different chromosomes with the same start could compare unequal because the ordering was only by `start` attribute.
| key=attrgetter("chromosome", "start", "ref", "alt")) | ||
|
|
||
| @property | ||
| def id(self) -> str: |
There was a problem hiding this comment.
should this be __hash__ or it should be separate from the magic function?
There was a problem hiding this comment.
Has to stay separate. __hash__ contract requires immutable state. It's safer to leave it intentionally un-hashable.
https://releases.llvm.org/23.1.0/projects/libcxx/docs/ReleaseNotes.html libcxx 23 release notes say: > libc++ has dropped a lot of transitive includes this presents itself as many errors like src/genome_track_io.cpp:639:2: error: no member named 'throw_with_nested' in namespace 'std' 639 | GK_RETHROW("for '{}'", interval); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The last successful build was with libcxx 22.1.8, failed one is with 23.1.2.
| #define __GK_ASSERT_H__ | ||
|
|
||
| #include "defines.h" | ||
| #include <exception> |
There was a problem hiding this comment.
nice catch and sorry about that!
gkaur
left a comment
gkaur
left a comment
There was a problem hiding this comment.
Great work! Thanks @ovesh, this gives us the missing building block for tracking target sites on modified genomes: a deterministic ID for a VariantGenome, plus a way to persist and reload the underlying base genome + variants.
It would be great if we could document the expected lifecycle somewhere in the user/API docs:
- create
VariantGenome(base_genome, variants) - call
save() - store the returned
variant_genome_idalongside downstream records - later, call
VariantGenome.load(id)with access to the same data manager.
The main thing that would also be helpful to mention is that the ID is not self-contained; GK can only reconstruct it if the backing variant_genome.<id>.txt.gz file exists, and the saved variants are canonicalized/normalized.
Another small future ergonomics thought: if downstream systems eventually want a single genome like string, we could expose a convention like mm39:<variant_genome_id>. Not essential for this PR, since storing base genome + variant genome ID separately should work fine.
Aside from the small gzip determinism nit I left inline, this looks great to me!!
| :py:exc:`ValueError` | ||
| The stored file does not match `variant_genome_id`. | ||
| """ | ||
| path = gk_data.data_manager.get_file(cls._FILENAME_FORMAT.format(variant_genome_id)) |
There was a problem hiding this comment.
Small suggestion: should VariantGenome.load() validate that variant_genome_id looks like the expected 64-char hex digest before formatting it into the data-manager filename? Not critical, but it would avoid odd filenames and give a clearer error for bad IDs.
Currently, if some one uses there calls:
VariantGenome.load("abc")
VariantGenome.load("../xyz")
GK would try to fetch files like:
variant_genome.abc.txt.gz
variant_genome.../xyz.txt.gz
And I'm not sure what the error would be!
| lines = [self.genome.config] + [v.as_variant_string() for v in self._canonical_variants()] | ||
| with open(path, "wb") as f: | ||
| # mtime=0 keeps the bytes deterministic so re-saving an equal genome matches the existing upload | ||
| f.write(gzip.compress("".join(line + "\n" for line in lines).encode(), mtime=0)) |
There was a problem hiding this comment.
Small nit: save() assumes gzip output is fully deterministic once we set mtime=0, but (Claude told me) there’s still one gzip header byte (“OS” byte), that can vary when Python/zlib writes the gzip wrapper. Common Unix-like builds often produce the same value, so this probably isn’t affecting us today, but another runtime/platform could produce different raw bytes.
Easiest fix I think would be to normalize that byte after compressing, or write the gzip through gzip.GzipFile(..., filename="", mtime=0) so the header is stable. Not urgent, but cheap to fix :)
3 participants
startattribute.