fix(kg_emb): migrate SampleKGDataset off the removed 1.x dataset API - #1202
fix(kg_emb): migrate SampleKGDataset off the removed 1.x dataset API#1202AxelNoun wants to merge 12 commits into
Conversation
SampleDataset is now a litdata.StreamingDataset that expects schema.pkl. A knowledge-graph task is an in-memory list of triples, so SampleKGDataset subclasses torch.utils.data.Dataset and exposes KGDatasetProtocol for the models. Co-authored-by: Cursor <cursoragent@cursor.com>
KGE models only need entity_num, relation_num and task_spec_param. Annotate that structural contract with KGDatasetProtocol so the model layer no longer imports the removed SampleBaseDataset. Co-authored-by: Cursor <cursoragent@cursor.com>
SampleKGDataset was never exported from pyhealth.datasets. The examples now import it from kg_emb.datasets and build a torch DataLoader with collate_fn_dict_with_padding, because get_dataloader requires litdata.StreamingDataset.set_shuffle(). Co-authored-by: Cursor <cursoragent@cursor.com>
Validate ratios with ValueError so the check survives python -O, and shuffle with a local Generator so the function no longer mutates global NumPy state. Co-authored-by: Cursor <cursoragent@cursor.com>
pandarallel was never declared in pyproject.toml or pixi.lock. The undeclared import in umls.py and base_kg_dataset.py is what produced the ModuleNotFoundError on the kg_emb import path in issue sunlabuiuc#952. initialize() ran in umls.py with no parallel_apply in kg_emb; mimicextract's parallel_apply calls are unreachable on the empty BaseEHRDataset stub. There is no lockfile entry to regenerate. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover construction, split reproducibility, generic collation of variable-length ground truths, set_task on a synthetic graph, and scoring invariants. Tests instantiate SampleKGDataset so a rename-only fix cannot go green. Co-authored-by: Cursor <cursoragent@cursor.com>
Document the map-style SampleKGDataset path in the MedCode API page and add a synthetic TransE example that uses DataLoader instead of get_dataloader. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace double-hyphen asides in five docstrings with periods or commas so they remain readable in a terminal and under Sphinx. Co-authored-by: Cursor <cursoragent@cursor.com>
SampleKGDataset failed to satisfy its own KGDatasetProtocol under mypy: the Protocol declared task_spec_param as a plain attribute (Mapping[str, Any] | None), which Protocol treats as read-write and therefore invariant, while SampleKGDataset declares it as dict[str, Any] | None. Models only ever read task_spec_param, so declare it as a read-only property instead: read-only Protocol members are covariant, and a concrete dict satisfies it.
Each model's __main__ block builds an untyped list of dict literals and then adds a "train" key with a bool value, which mypy rejects because it infers the dict's value type from the first literal. Annotate samples as list[dict[str, Any]] in all four demo blocks.
|
Update: after discussing direction with @jhnwu3, the target is to keep Carrying over regardless of the base class: the |
Architecture & Implementation Plan UpdateJust dropping a quick update here to keep a clear record of the architectural decisions we aligned on via Discord, which will guide the next commits for #1202: 1. Base Architecture (In-Memory) 2. Data Shape & Serialization (The "Tensor Trick")
Next Steps: |
…the Tensor Trick
Per the architecture pivot agreed in the PR discussion: SampleKGDataset moves
back under InMemorySampleDataset instead of standalone torch.utils.data.Dataset,
while keeping its full public surface (entity2id/relation2id, cardinality
validation, split(), stat(), dev/task_spec_param) unchanged.
- Add KGProcessor ("kg_entity_list"), pre-padding ground_truth_head/tail to
each field's own max length and emitting {"value", "mask"} pure tensors
ahead of litdata's pickle-based caching, instead of raw variable-length
Python lists. "triple" goes through the existing "tensor" processor.
- Fix a correctness issue the padding introduces: pad_token_id (0) is not a
reserved sentinel and can collide with a real entity id. kg_base.py's
train_neg_sample_gen and test_neg_sample_filter_bias_gen now reconstruct
the exact unpadded entity list via the mask (_unpad_ground_truth) before
doing set-membership filtering, so negative sampling and filtered ranking
stay correct whenever entity 0 is legitimate.
- Add a nested-dict collation branch to collate_fn_dict_with_padding,
restricted to all-tensor dicts, so {"value","mask"} pairs batch via a
plain stack (shape is already uniform per field) without disturbing the
existing list-of-dicts collation used by heterogeneous per-sample dicts
such as "hyperparameters".
- Update tests/core/test_kg_emb.py for the new shapes: triple is now a
Tensor (not a tuple), ground_truth_* collate to {"value","mask"}, and
set_shuffle is now expected (SampleKGDataset is intentionally back under
the SampleDataset umbrella). Adds TestGroundTruthUnpadding, a regression
test for the padding/entity-0 collision fix above.
24/24 tests in tests/core/test_kg_emb.py pass, plus the sample_kg_dataset.py
and splitter.py doctests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mple) The contribution-rules check (tools/check_pr_rules.py) failed on 5f66ff4, something I hadn't run locally before pushing — only pytest, not ruff or the repo's own gate. It enforces ruff-clean added/modified lines plus a '>>>' doctest on every new/modified top-level public class or function. - kg_processor.py: replace typing.Dict/List/Iterable with PEP 585 builtins and collections.abc.Iterable (ruff UP035/UP006, target-version py313). Entirely new file, so every line was in scope. - datasets/utils.py: add a runnable '>>>' example to collate_fn_dict_with_padding's docstring, since the function body was modified (the nested all-tensor-dict branch) and had none. Verified against the same tooling and base/head SHAs CI used (0a75f99..<new commit>): `python tools/check_pr_rules.py --base --head` now reports "All PR contribution rules passed." tests/core/test_kg_emb.py still 24/24, plus the collate_fn_dict_with_padding doctest. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI/CD Green & Ready for ReviewEverything is fully green! I've completed the implementation of the architectural plan we discussed:
The PR is out of draft and ready for your final review whenever you have time! |
Closes #952. Supersedes #1192. Part of #1201.
What the rename does not reach
#1192 renames
SampleBaseDatasettoSampleDatasetacross 7 files. The renameclears the
ImportError, but the module stays unusable: since 2.0,SampleDatasetis alitdata.StreamingDatasetwhose constructor expects adirectory containing
schema.pkl, whileSampleKGDataset.__init__still passesa list of samples, an argument the rename does not touch.
self.samplesis then never assigned, so__getitem__andstat()raiseAttributeError, andBaseKGDataset.set_task()cannot return anything usable.The failure moves from import time to call time.
To be fair to the author: #952 explicitly asked for the rename across eight
files, and #1192 does exactly that. The gap is in how the issue was framed.
Root cause
SampleKGDatasetinherited fromSampleBaseDatasetto reuse__len__and.samples. It required none of the parent's behavioural contract and used noneof the services the hierarchy exists to provide: a knowledge graph has no
feature schema, no processors and no patient/visit index. The inheritance was
convenience reuse, not an is-a relationship.
The 2.0 migration did not introduce this. It removed the coincidence that made
it harmless, namely the overlap between the interface actually used and the one
the parent exposed. Adapting
SampleKGDatasetto the streaming contract wouldmean satisfying a constraint the module has no use for, so removing the
inheritance is the minimal fix rather than the drastic one.
What this PR does
SampleKGDatasetbecomes a standalonetorch.utils.data.Dataset.Models depend on a structural
typing.Protocol(PEP 544) instead of a concreteclass.
KGEBaseModel.__init__only ever readsentity_num,relation_numandtask_spec_param, so both the old and the new annotation over-specified thecontract, and both were wrong:
SampleDatasethas neither attribute.KGDatasetProtocolstates the capability actually required. The annotationbecomes checkable by mypy, the model layer becomes testable with lightweight
doubles, and
kg_embstops being coupled to changes in the main pipeline. Thatlast point is the durable one: a rename would have held until the next
SampleDatasetrefactor.Also in scope:
split()no longer overwrites NumPy's global random state, and validates itsinput with
ValueErrorrather thanassert, which is stripped underpython -Oand should not be load-bearing for user input validation. Note:for a given
seed=, this returns a different partition than the previousimplementation (different RNG algorithm). There is no prior release of this
module, so no existing caller depends on the old ordering.
__main__examples importedSampleKGDatasetfrompyhealth.datasets, where it has never existed. They now usetorch.utils.data.DataLoaderwithcollate_fn_dict_with_paddingdirectly,since
get_dataloadercallsset_shuffleand is streaming-only.pandarallelimport, of a package never declared inpyproject.toml, isremoved rather than added to the dependencies, since no
parallel_applyexists anywhere in
kg_emb. There is no lockfile entry to regenerate.base_kg_dataset.pyandumls.pyimported their siblings through theabsolute package path, so the package only initialised correctly for one
ordering of the imports in
__init__.py, which nothing enforced. Enablingruff's isort rule would have sorted
base_kg_datasetfirst and broken it.Tests
tests/core/test_kg_emb.py, 22 behavioural tests: construction, indexing,vocabularies, cardinality validation, split partitioning and reproducibility,
generic collation of variable-length ground truths,
BaseKGDataset.set_task()on a synthetic graph, and scoring invariants (DistMult symmetry in head and
tail, TransE margin on an exact triple).
No test asserts on a type annotation: annotations are metadata, and under
PEP 563 such an assertion compares against a string.
Every test was validated by fault injection. Reverting the fix it protects makes
it fail:
test_construction_and_lengthsuper().__init__(samples, dataset_name, task_name)test_is_a_map_style_datasetset_shuffletest_set_task_on_a_synthetic_graph**kwargsfrom theSampleKGDataset(...)calltest_global_numpy_state_is_untouchednp.random.seedplusshuffletest_variable_length_ground_truth_stays_a_python_listground_truth_headin__getitem__These are hand-seeded mutants aimed at this PR's changes, not a systematic
mutation run, so 22/22 is not a mutation score in the usual sense. It supports a
weaker and sufficient claim: a rename-only fix cannot make this suite go green.
Scope and limits
kg_embfrom the 2.0 pipeline; it does not migrate it intothe streaming pipeline. If you would rather see the latter, the dataset layer
here is isolated and tested and would be the starting point.
compared against the original papers. Verified on synthetic graphs only, as
UMLS requires a licence.
get_dataloaderremains streaming-only.kg_embno longer depends on it, sothis is not blocking here; tracked in PyHealth 2.0: several modules still target the 1.x dataset API #1201 together with the other
modules still targeting the 1.x API.
Credit to @userjuma for the original diagnosis in #1192. The import chain
described there is accurate and was the starting point for this work.