feat: Add GenBank file format support for biological sequence data#7951
feat: Add GenBank file format support for biological sequence data#7951behroozazarkhalili wants to merge 6 commits into
Conversation
|
|
||
| return location | ||
|
|
||
| def _parse_genbank(self, fp): |
There was a problem hiding this comment.
hi, was this function written by you / an AI / someone else ? if it comes from somewhere else you should mention it
it's too long for me to review for now, please consider using an external / mature library to parse such data instead if it helps simplify the code
There was a problem hiding this comment.
Hi @lhoestq, thanks for the review!
I wrote this parser myself. The reason I chose a custom pure-Python state machine parser over an external library is to stay consistent with the zero-external-dependency pattern used by the other packaged modules in this project.
The only mature library for GenBank parsing is Biopython (Bio.GenBank / Bio.SeqIO), but it's a very heavy dependency (~150 MB installed) and would be disproportionate for what we need here. The same reasoning was applied for the FASTA and FASTQ loaders in this project — they also use custom lightweight parsers (based on Heng Li's readfq.py) rather than pulling in Biopython.
The GenBank flat file format is well-documented by NCBI (spec), so a focused parser that only extracts the fields we need is more practical than adding a large dependency.
That said, I'm happy to refactor the _parse_genbank method to make it shorter and easier to review — for example by breaking it into smaller helper methods per section (LOCUS, FEATURES, ORIGIN, etc.). Would that help?
There was a problem hiding this comment.
Thanks @lhoestq. I've split _parse_genbank into the per-section handlers I mentioned: a small _FeatureAccumulator now owns the multi-line FEATURES/qualifier bookkeeping (it was duplicated in four spots), and the state machine is broken into _handle_header_line / _handle_features_line / _handle_origin_line, so _parse_genbank itself is now a ~40-line dispatch loop. Behavior is unchanged — all existing tests pass. Hope that's easier to review; happy to adjust further.
- Add GenBank packaged module with state machine parser - Support .gb, .gbk, .genbank file extensions - Parse LOCUS, DEFINITION, ACCESSION, VERSION, KEYWORDS, ORGANISM metadata - Parse FEATURES section with structured JSON output - Parse ORIGIN sequence data with automatic compression detection (gzip, bz2, xz) - Implement dual-threshold batching (batch_size + max_batch_bytes) - Use large_string Arrow type for sequences to handle very long data - Add comprehensive test suite with 28 tests
Addresses review feedback that the parser was too long to review. Extracts the multi-line FEATURES bookkeeping into a small _FeatureAccumulator and splits the state machine into per-section handlers (_handle_header_line, _handle_features_line, _handle_origin_line), turning _parse_genbank into a slim dispatch loop. This collapses the qualifier-flush logic that was duplicated four times and shrinks _parse_genbank from ~155 to ~40 lines. Behavior is unchanged: all 28 existing genbank tests pass.
6bec2ff to
f035611
Compare
|
Hi @lhoestq — quick status. The I've also rebased the branch onto the latest |
lhoestq
left a comment
There was a problem hiding this comment.
Great thanks for the changes ! Are there datasets on Hugging Face already I can use to test your implementation ?
| if magic[:2] == b"\x1f\x8b": # gzip magic number | ||
| return gzip.open(filepath, "rt", encoding="utf-8") | ||
| elif magic[:3] == b"BZh": # bzip2 magic number | ||
| return bz2.open(filepath, "rt", encoding="utf-8") | ||
| elif magic[:6] == b"\xfd7zXZ\x00": # xz magic number | ||
| return lzma.open(filepath, "rt", encoding="utf-8") |
There was a problem hiding this comment.
I think you don't need thjis. Indeed you call download_and_extract() so files are already extracted/uncompressed
There was a problem hiding this comment.
Happy to remove the magic-byte decompression in _open_file — just want to confirm one thing first. The unit tests exercise _generate_tables directly (e.g. test_genbank_gzipped/bz2/xz) without going through dl_manager, so they rely on _open_file decompressing. If I drop it and trust download_and_extract() + extract_on_the_fly, those direct-call tests need to route through the dl_manager (or be dropped). Do you want it fully removed (and the tests reworked accordingly), or kept as a lightweight fallback for the direct-call path? I'll follow your preference.
There was a problem hiding this comment.
if you use _generate_tables in the tests directly without the dl_manager, you can replace file.gb.gz with gz://file.gb::file.gb.gz and the regular open() function and it should work
(open() is extended to support chained uris for compression/archives)
There was a problem hiding this comment.
Done in b5aa245. Removed _open_file; the loader now uses plain open() (which becomes xopen under the streaming patch), matching the text/json modules.
One small correction for the tests: the chained-URI protocol is gzip rather than gz — gz:// raises Protocol not known since fsspec registers the codec as gzip (and bz2/xz for the others). So the direct-_generate_tables tests now build the URI as gzip://sequence.gb::/path/sequence.gb.gz. I derive the protocol from _get_extraction_protocol in the fixtures so they track the loader's real behavior rather than hardcoding it. All 27 tests pass with the gzip/bz2/xz coverage preserved through the real xopen path.
| parse_features: Whether to parse FEATURES section into structured JSON. | ||
| If False, stores raw text. |
There was a problem hiding this comment.
You can remove this argument and always parse IMO.
There is a new datasets.Json() type for this kind of data that you can use btw
There was a problem hiding this comment.
Done — removed the parse_features argument; FEATURES is now always parsed. I also switched the features column to the new datasets.Json() type as you suggested, so it decodes to a parsed Python object instead of a JSON string. Verified the round-trip stores a single JSON encoding (no double-encoding) and reads back as a list of feature dicts.
| ] | ||
|
|
||
| def _info(self): | ||
| return datasets.DatasetInfo(features=self.config.features) |
There was a problem hiding this comment.
if the features are always the same you can pass them here to DatasetInfo (and use self.config.features if the user wants to override them for some reason)
There was a problem hiding this comment.
Done. The schema is always the same, so I declared a canonical DEFAULT_FEATURES and pass it to DatasetInfo in _info(). self.config.features still overrides it if a user wants to, and when columns= is set the features are projected to that subset. Column validation now also happens at config time as a side effect (fail-fast).
… features Addresses review feedback on huggingface#7951: - Remove the `parse_features` config flag; FEATURES is now always parsed. The `features` column is typed as `datasets.Json()`, so it decodes to a parsed object instead of a JSON string. The storage column is built as the JSON arrow type directly to avoid double-encoding during the feature cast. - Declare a canonical `DEFAULT_FEATURES` schema and pass it to DatasetInfo in `_info()`, since the schema is always the same. `config.features` still overrides it, and the schema is projected to `columns` when subset. Column validation now happens at config time (fail-fast). Tests updated to match: drop the parse_features cases, read `features` as a parsed object, assert the JSON arrow type, and expect invalid-column errors at construction time. All 27 genbank tests pass.
|
On test datasets: I searched the Hub and couldn't find a public dataset hosting raw GenBank files ( |
|
I think having one on the Hub that we can point to in the documentation would be useful for the community. This way they have an actual example they can try and replicate for their cases |
Addresses review feedback on huggingface#7951: the loader called download_and_extract() with extract_on_the_fly, so files are already decompressed in the real load path; the custom magic-byte _open_file was redundant. Remove it and open files with the plain open(), matching the text/json packaged modules. For the unit tests that call _generate_tables directly without dl_manager, compressed fixtures now return datasets' chained compression URI (<protocol>://<inner>::<outer>), which the streaming-patched open() (xopen) decompresses. The protocol is derived from datasets' own _get_extraction_protocol so the tests track the loader's real behavior rather than hardcoding it. All 27 genbank tests pass; gzip/bz2/xz coverage preserved via the real xopen path.
|
Created one: https://huggingface.co/datasets/ermiaazarkhalili/datasets-genbank-test It has 3 real, complete NCBI records — ds = load_dataset("ermiaazarkhalili/datasets-genbank-test", data_files="*.gb*", split="train")
# 3 rows; FEATURES decode to structured objects (Json()), and the gzipped file loads via xopenHappy to move it under a different org or reference it from the docs page — let me know where you'd like it linked. |
Add a GenBank subsection to the loading guide, following the CSV/JSON/Parquet pattern, and point to a small example dataset of real NCBI records (ermiaazarkhalili/datasets-genbank-test) so users have something to run and replicate, including a gzipped record that loads transparently.
Summary
Add native support for loading GenBank (.gb, .gbk, .genbank) files, a standard format for biological sequence data with annotations maintained by NCBI.
Changes
genbankpackaged module with pure Python state machine parser_PACKAGED_DATASETS_MODULESand_EXTENSION_TO_MODULEFeatures
large_stringArrow type for sequences/featuresUsage
Test plan