diff --git a/documentation/Loader.md b/documentation/Loader.md index 7952f33c..7176f4cf 100644 --- a/documentation/Loader.md +++ b/documentation/Loader.md @@ -164,6 +164,11 @@ about driving the loader from a test: `get_config()` reads `config.CONFIG_PATH` at call time, so patch it on `node_normalizer.config`. +To *query* the frontend against data a loader test just wrote (end-to-end, rather +than asserting on raw Redis keys), see `tests/CLAUDE.md` — it covers reusing the +fixture's `redis_config.yaml` for `RedisConnectionFactory`, the process-wide +connection cache, and faking the app. + ### Dependency landmine: requests / docker / testcontainers `requests >= 2.32` changed its `HTTPAdapter`, which breaks the `docker` SDK < 7.1 diff --git a/node_normalizer/loader/CLAUDE.md b/node_normalizer/loader/CLAUDE.md new file mode 100644 index 00000000..2a656914 --- /dev/null +++ b/node_normalizer/loader/CLAUDE.md @@ -0,0 +1,14 @@ +# Loader notes + +## Don't store the clique-level `taxa` field + +Babel compendium lines carry a clique-level `taxa` list *and* a per-identifier +`t` list on each entry in `identifiers`. The clique-level `taxa` is just the +union of the per-identifier `t` values, so persisting it separately would be +redundant. + +`load_compendium` already writes the whole `identifiers` list (including each +`t`) into `id_to_eqids_db`, so taxa are retrievable per-identifier from there. +Do **not** add `taxa` to the db-5 clique-property JSON — read `t` from the +`id_to_eqids_db` blob instead. (See the taxa assertion in +`tests/test_loader_integration.py`.) diff --git a/node_normalizer/loader/loader.py b/node_normalizer/loader/loader.py index 461364c1..e244f5c3 100644 --- a/node_normalizer/loader/loader.py +++ b/node_normalizer/loader/loader.py @@ -162,7 +162,9 @@ def load_compendium(compendium_filename, block_size: int, test_mode: int = 0) -> eq_id_to_id_db: UPPER(equivalent id) -> canonical id id_to_eqids_db: canonical id -> equivalent identifiers (JSON) id_to_type_db: canonical id -> biolink type - info_content_db: canonical id -> information content + info_content_db: canonical id -> clique properties JSON {"preferred_name", "ic"} + (info_content_db is being grown into a general clique-property store; see #306. + It formerly held a bare information-content float, so readers must tolerate both.) Returns the per-type source-prefix counts accumulated from this file. """ source_prefixes: dict = {} @@ -203,8 +205,12 @@ def load_compendium(compendium_filename, block_size: int, test_mode: int = 0) -> term2id_pipeline.set(equivalent_id["i"].upper(), identifier) id2eqids_pipeline.set(identifier, json.dumps(instance["identifiers"])) id2type_pipeline.set(identifier, instance["type"]) - if "ic" in instance and instance["ic"] is not None: - info_content_pipeline.set(identifier, instance["ic"]) + # Clique-level properties, keyed by canonical id. Every clique gets one + # (unlike the old IC-only write, which skipped cliques without an "ic"). + props = {"preferred_name": instance.get("preferred_name", "")} + if instance.get("ic") is not None: + props["ic"] = instance["ic"] + info_content_pipeline.set(identifier, json.dumps(props)) if test_mode != 1 and line_counter % block_size == 0: term2id_pipeline.execute() diff --git a/node_normalizer/normalizer.py b/node_normalizer/normalizer.py index 9a10455e..9403b6c8 100644 --- a/node_normalizer/normalizer.py +++ b/node_normalizer/normalizer.py @@ -31,6 +31,22 @@ config = get_config() +def _clique_props(raw) -> dict: + """ + Parse a value from info_content_db (db 5), which is growing into a general + clique-property store (see #306). New loads store a JSON dict + {"preferred_name", "ic"}; older loads stored a bare information-content float. + Normalize both into a dict so callers can just do .get("ic")/.get("preferred_name"). + """ + if raw is None: + return {} + try: + value = json.loads(raw) + except (ValueError, TypeError): + return {"ic": raw} + return value if isinstance(value, dict) else {"ic": value} + + def sort_identifiers_with_boosted_prefixes(identifiers, prefixes): """ Given a list of identifiers (with `identifier` and `label` keys), sort them using @@ -486,8 +502,12 @@ async def get_info_content( # call redis and get the value info_contents = await app.state.info_content_db.mget(*canonical_nonan, encoding='utf8') - # get this into a list - info_contents = [round(float(ic_ids), 1) if ic_ids is not None else None for ic_ids in info_contents] + # get this into a list (info_content_db now stores a clique-props JSON; _clique_props + # also handles the old bare-float format) + info_contents = [ + round(float(ic), 1) if (ic := _clique_props(raw).get("ic")) is not None else None + for raw in info_contents + ] # zip into an array of dicts info_contents = dict(zip(canonical_nonan, info_contents)) @@ -634,9 +654,26 @@ async def get_normalized_nodes( dereference_ids = dict() dereference_types = dict() + # Look up each clique's Babel-computed preferred_name. The key is the primary + # subclique's canonical id (dereference_ids[c][0]['i']), which under conflation + # differs from the input clique's canonical id — so this is a second, separate + # db-5 read, keyed by node identifier rather than by input canonical. + # ponytail: merging this into the get_info_content read would entangle IC (keyed + # by input canonical) with preferred_name (primary subclique) semantics. + node_ids = {c: dereference_ids[c][0]['i'] for c in canonical_nonan + if dereference_ids.get(c) and dereference_ids[c][0]} + if node_ids: + raw_props = await app.state.info_content_db.mget(*node_ids.values(), encoding='utf8') + by_nodeid = dict(zip(node_ids.values(), + (_clique_props(r).get("preferred_name") for r in raw_props))) + preferred_names = {c: by_nodeid.get(nid) for c, nid in node_ids.items()} + else: + preferred_names = {} + # output the final result normal_nodes = { input_curie: await create_node(app, canonical_id, dereference_ids, dereference_types, info_contents, + preferred_names=preferred_names, include_descriptions=include_descriptions, include_individual_types=include_individual_types, include_taxa=include_taxa, @@ -663,8 +700,9 @@ async def get_info_content_attribute(app, canonical_nonan) -> dict: :param canonical_nonan: :return: """ - # get the information content value - ic_val = await app.state.info_content_db.get(canonical_nonan, encoding='utf8') + # get the information content value (info_content_db now stores a clique-props JSON; + # _clique_props also handles the old bare-float format) + ic_val = _clique_props(await app.state.info_content_db.get(canonical_nonan, encoding='utf8')).get("ic") # did we get a good value if ic_val is not None: @@ -679,8 +717,8 @@ async def get_info_content_attribute(app, canonical_nonan) -> dict: return new_attrib -async def create_node(app, canonical_id, equivalent_ids, types, info_contents, include_descriptions=True, - include_individual_types=False, include_taxa=False, conflations=None): +async def create_node(app, canonical_id, equivalent_ids, types, info_contents, preferred_names=None, + include_descriptions=True, include_individual_types=False, include_taxa=False, conflations=None): """Construct the output format given the compressed redis data""" # It's possible that we didn't find a canonical_id if canonical_id is None: @@ -707,92 +745,107 @@ async def create_node(app, canonical_id, equivalent_ids, types, info_contents, i # OK, now we should have id's in the format [ {"i": "MONDO:12312", "l": "Scrofula"}, {},...] eids = equivalent_ids[canonical_id] - # As per https://github.com/NCATSTranslator/Babel/issues/158, we select the first label from any - # identifier _except_ where one of the types is in preferred_name_boost_prefixes, in which case - # we prefer the prefixes listed there. - # - # This should perfectly replicate NameRes labels for non-conflated cliques, but it WON'T perfectly - # match conflated cliques. To do that, we need to run the preferred label algorithm on ONLY the labels - # for the FIRST clique of the conflated cliques with labels. - any_conflation = any(conflations.values()) - if not any_conflation: - # No conflation. We just use the identifiers we've been given. - identifiers_with_labels = eids + # Babel now computes a preferred_name for every clique and we store it in + # info_content_db (keyed by the primary subclique's canonical id, i.e. eids[0]['i']). + # When present, use it directly (issue #299). + preferred_name = (preferred_names or {}).get(canonical_id) + if preferred_name: + node = {"id": {"identifier": eids[0]['i'], "label": preferred_name}} else: - # We have a conflation going on! To replicate Babel's behavior, we need to run the algorithem - # on the list of labels corresponding to the first - # So we need to run the algorithm on the first set of identifiers that have any - # label whatsoever. - identifiers_with_labels = [] - curies_already_checked = set() - for identifier in eids: - curie = identifier.get('i', '') - if curie in curies_already_checked: - continue - results, _ = await get_eqids_and_types(app, [curie]) - - identifiers_with_labels = results[0] - labels = map(lambda ident: ident.get('l', ''), identifiers_with_labels) - if any(map(lambda l: l != '', labels)): + # ---- DEPRECATED FALLBACK — DO NOT MODIFY OR IMPROVE ---- + # Legacy label-selection, retained only for querying older DB loads that lack a + # stored preferred_name. Babel now computes preferred_name centrally + # (https://github.com/NCATSTranslator/Babel/blob/16e370be4ba1b9f69b1ce091ccbb111941f35016/src/babel_utils.py#L581); + # delete this whole block (and sort_identifiers_with_boosted_prefixes plus the + # preferred_name_boost_prefixes/demote_labels_longer_than config) once all + # deployments load preferred_name. + # + # As per https://github.com/NCATSTranslator/Babel/issues/158, we select the first label from any + # identifier _except_ where one of the types is in preferred_name_boost_prefixes, in which case + # we prefer the prefixes listed there. + # + # This should perfectly replicate NameRes labels for non-conflated cliques, but it WON'T perfectly + # match conflated cliques. To do that, we need to run the preferred label algorithm on ONLY the labels + # for the FIRST clique of the conflated cliques with labels. + any_conflation = any(conflations.values()) + if not any_conflation: + # No conflation. We just use the identifiers we've been given. + identifiers_with_labels = eids + else: + # We have a conflation going on! To replicate Babel's behavior, we need to run the algorithm + # on the list of labels corresponding to the first + # So we need to run the algorithm on the first set of identifiers that have any + # label whatsoever. + identifiers_with_labels = [] + curies_already_checked = set() + for identifier in eids: + curie = identifier.get('i', '') + if curie in curies_already_checked: + continue + results, _ = await get_eqids_and_types(app, [curie]) + + identifiers_with_labels = results[0] + labels = map(lambda ident: ident.get('l', ''), identifiers_with_labels) + if any(map(lambda l: l != '', labels)): + break + + # Since we didn't get any matches here, add it to the list of CURIEs already checked so + # we don't make redundant queries to the database. + curies_already_checked.update(set(map(lambda x: x.get('i', ''), identifiers_with_labels))) + + # We might get here without any labels, which is fine. At least we tried. + + # At this point: + # - eids will be the full list of all identifiers and labels in this clique. + # - identifiers_with_labels is the list of identifiers and labels for the first subclique that has at least + # one label. + + # Note that types[canonical_id] goes from most specific to least specific, so we + # need to reverse it in order to apply preferred_name_boost_prefixes for the most + # specific type. + possible_labels = [] + for typ in types[canonical_id][::-1]: + if typ in config['preferred_name_boost_prefixes']: + # This is the most specific matching type, so we use this and then break. + possible_labels = list(map(lambda ident: ident.get('l', ''), + sort_identifiers_with_boosted_prefixes( + identifiers_with_labels, + config['preferred_name_boost_prefixes'][typ] + ))) + + # Add in all the other labels -- we'd still like to consider them, but at a lower priority. + for eid in identifiers_with_labels: + label = eid.get('l', '') + if label not in possible_labels: + possible_labels.append(label) + + # Since this is the most specific matching type, we shouldn't do other (presumably higher-level) + # categories: so let's break here. break - # Since we didn't get any matches here, add it to the list of CURIEs already checked so - # we don't make redundant queries to the database. - curies_already_checked.update(set(map(lambda x: x.get('i', ''), identifiers_with_labels))) - - # We might get here without any labels, which is fine. At least we tried. - - # At this point: - # - eids will be the full list of all identifiers and labels in this clique. - # - identifiers_with_labels is the list of identifiers and labels for the first subclique that has at least - # one label. - - # Note that types[canonical_id] goes from most specific to least specific, so we - # need to reverse it in order to apply preferred_name_boost_prefixes for the most - # specific type. - possible_labels = [] - for typ in types[canonical_id][::-1]: - if typ in config['preferred_name_boost_prefixes']: - # This is the most specific matching type, so we use this and then break. - possible_labels = list(map(lambda ident: ident.get('l', ''), - sort_identifiers_with_boosted_prefixes( - identifiers_with_labels, - config['preferred_name_boost_prefixes'][typ] - ))) - - # Add in all the other labels -- we'd still like to consider them, but at a lower priority. - for eid in identifiers_with_labels: - label = eid.get('l', '') - if label not in possible_labels: - possible_labels.append(label) - - # Since this is the most specific matching type, we shouldn't do other (presumably higher-level) - # categories: so let's break here. - break - - # Step 1.2. If we didn't have a preferred_name_boost_prefixes, just use the identifiers in their - # Biolink prefix order. - if not possible_labels: - possible_labels = map(lambda eid: eid.get('l', ''), identifiers_with_labels) - - # Step 2. Filter out any suspicious labels. - filtered_possible_labels = [l for l in possible_labels if - l and # Ignore blank or empty names. - not l.startswith('CHEMBL') # Some CHEMBL names are just the identifier again. - ] - - # Step 3. Filter out labels longer than config['demote_labels_longer_than'], but only if there is at - # least one label shorter than this limit. - labels_shorter_than_limit = [l for l in filtered_possible_labels if l and len(l) <= config['demote_labels_longer_than']] - if labels_shorter_than_limit: - filtered_possible_labels = labels_shorter_than_limit - - # Note that the id will be from the equivalent ids, not the canonical_id. This is to handle conflation - if len(filtered_possible_labels) > 0: - node = {"id": {"identifier": eids[0]['i'], "label": filtered_possible_labels[0]}} - else: - # Sometimes, nothing has a label :( - node = {"id": {"identifier": eids[0]['i']}} + # Step 1.2. If we didn't have a preferred_name_boost_prefixes, just use the identifiers in their + # Biolink prefix order. + if not possible_labels: + possible_labels = map(lambda eid: eid.get('l', ''), identifiers_with_labels) + + # Step 2. Filter out any suspicious labels. + filtered_possible_labels = [l for l in possible_labels if + l and # Ignore blank or empty names. + not l.startswith('CHEMBL') # Some CHEMBL names are just the identifier again. + ] + + # Step 3. Filter out labels longer than config['demote_labels_longer_than'], but only if there is at + # least one label shorter than this limit. + labels_shorter_than_limit = [l for l in filtered_possible_labels if l and len(l) <= config['demote_labels_longer_than']] + if labels_shorter_than_limit: + filtered_possible_labels = labels_shorter_than_limit + + # Note that the id will be from the equivalent ids, not the canonical_id. This is to handle conflation + if len(filtered_possible_labels) > 0: + node = {"id": {"identifier": eids[0]['i'], "label": filtered_possible_labels[0]}} + else: + # Sometimes, nothing has a label :( + node = {"id": {"identifier": eids[0]['i']}} # Now that we've determined a label for this clique, we should never use identifiers_with_labels, possible_labels, # or filtered_possible_labels after this point. diff --git a/tests/CLAUDE.md b/tests/CLAUDE.md new file mode 100644 index 00000000..97612da6 --- /dev/null +++ b/tests/CLAUDE.md @@ -0,0 +1,44 @@ +# Test notes + +Non-obvious bits of writing tests here. Marker commands and the general +frontend/loader split are in `documentation/CONTRIBUTOR.md`; loader-*driving* +mechanics (the `redis_connect` `lru_cache`, which module to `monkeypatch`, the +requests/docker/testcontainers landmine) are in `documentation/Loader.md`. + +## The shared-app-singleton pitfall + +`test_norm.py` and `test_setid.py` assign to `app.state.*` at **import time**, +mutating the shared `node_normalizer.server.app` singleton for the rest of the +process. Don't assume that app is pristine in a later test, and don't add more +import-time mutation. New tests should build their own fake app +(`SimpleNamespace(state=SimpleNamespace(...))`) rather than importing the server +app — see the examples below. + +## Querying the frontend against loaded data (end-to-end) + +To assert on what `get_normalized_nodes` *returns* (not just what keys the loader +wrote), drive the real frontend against a testcontainer instead of a hand-built +mock — it avoids reimplementing the loader's canonical-id/db-5 keying. See +`test_query_gene_protein_conflation_uses_gene_preferred_name` in +`test_loader_integration.py`: + +- **Reuse the loader's `redis_config.yaml`.** The loader fixture already writes one + pointing every logical db at the container; the frontend's + `RedisConnectionFactory.create_connection_pool(path)` consumes that same file. Have + the fixture `yield` the config path. +- **`RedisConnectionFactory.connections` is a process-wide class-level cache.** It's + only populated `if not connections`, so a pool left over from another test wins and + you attach to a dead container. Set `RedisConnectionFactory.connections = {}` before + `create_connection_pool`, and in a `finally` close every connection and clear it again. +- **Fake the app, skip the Toolkit.** Build `app = SimpleNamespace(state=SimpleNamespace( + eq_id_to_id_db=..., id_to_eqids_db=..., info_content_db=..., gene_protein_db=..., ...))` + from `get_connection(name)`, and pre-seed `state.ancestor_map` for the types in play + (e.g. `{"biolink:Gene": ["biolink:Gene"], ...}`) so `get_ancestors` never needs a + Biolink `Toolkit` (`state.toolkit=None`). Only the conflation *fallback* in + `create_node` reads Redis beyond these dbs, so the stored-`preferred_name` path needs + nothing more. + +For a pure-unit version that skips Docker entirely, `test_normalizer.py` drives +`get_normalized_nodes` / `create_node` against a tiny `_MgetRedis` mock — cheaper, but +you must construct the redis values (canonical-id keys, db-5 JSON, gene-first conflation +list) by hand, so it's easy to encode an assumption the loader doesn't actually make. diff --git a/tests/resources/Disease.txt b/tests/resources/Disease.txt new file mode 100644 index 00000000..307d6877 --- /dev/null +++ b/tests/resources/Disease.txt @@ -0,0 +1 @@ +{"type": "biolink:Disease", "ic": 100.0, "identifiers": [{"i": "UMLS:C4288892", "l": "Infant Acute Undifferentiated Leukemia", "d": [], "t": []}, {"i": "NCIT:C126110", "l": "Infant Acute Undifferentiated Leukemia", "d": ["An acute undifferentiated leukemia that occurs in infancy."], "t": []}], "preferred_name": "Infant Acute Undifferentiated Leukemia", "taxa": []} diff --git a/tests/resources/Gene.txt b/tests/resources/Gene.txt new file mode 100644 index 00000000..1f84bfcd --- /dev/null +++ b/tests/resources/Gene.txt @@ -0,0 +1 @@ +{"type": "biolink:Gene", "ic": 86.30756602822301, "identifiers": [{"i": "NCBIGene:1017", "l": "CDK2", "d": ["cyclin dependent kinase 2"], "t": ["NCBITaxon:9606"]}, {"i": "ENSEMBL:ENSG00000123374", "l": "", "d": [], "t": []}, {"i": "HGNC:1771", "l": "CDK2", "d": [], "t": []}, {"i": "OMIM:116953", "l": "CDK2", "d": [], "t": []}, {"i": "UMLS:C1332733", "l": "CDK2 gene", "d": [], "t": []}], "preferred_name": "CDK2", "taxa": ["NCBITaxon:9606"]} diff --git a/tests/resources/GeneProtein.txt b/tests/resources/GeneProtein.txt new file mode 100644 index 00000000..8b7f1bd8 --- /dev/null +++ b/tests/resources/GeneProtein.txt @@ -0,0 +1 @@ +["NCBIGene:1017", "UniProtKB:B4DDL9", "UniProtKB:E7ESI2", "UniProtKB:G3V5T9", "UniProtKB:P24941"] diff --git a/tests/resources/PhenotypicFeature.txt b/tests/resources/PhenotypicFeature.txt new file mode 100644 index 00000000..bf17c1e2 --- /dev/null +++ b/tests/resources/PhenotypicFeature.txt @@ -0,0 +1 @@ +{"type": "biolink:PhenotypicFeature", "ic": 100.0, "identifiers": [{"i": "HP:0009278", "l": "Ulnar deviation of the 4th finger", "d": ["Displacement of the 4th finger towards the ulnar side (i.e., towards the 5th finger)."], "t": ["NCBITaxon:9606"]}, {"i": "UMLS:C4024474", "l": "Ulnar deviation of the 4th finger", "d": [], "t": []}], "preferred_name": "Ulnar deviation of the 4th finger", "taxa": ["NCBITaxon:9606"]} diff --git a/tests/resources/Protein.txt b/tests/resources/Protein.txt new file mode 100644 index 00000000..ba6d315c --- /dev/null +++ b/tests/resources/Protein.txt @@ -0,0 +1,4 @@ +{"type": "biolink:Protein", "ic": null, "identifiers": [{"i": "UniProtKB:B4DDL9", "l": "B4DDL9_HUMAN Cyclin-dependent kinase 2 (trembl)", "d": [], "t": ["NCBITaxon:9606"]}], "preferred_name": "B4DDL9_HUMAN Cyclin-dependent kinase 2 (trembl)", "taxa": ["NCBITaxon:9606"]} +{"type": "biolink:Protein", "ic": null, "identifiers": [{"i": "UniProtKB:G3V5T9", "l": "G3V5T9_HUMAN Cyclin-dependent kinase 2 (trembl)", "d": [], "t": ["NCBITaxon:9606"]}, {"i": "ENSEMBL:ENSP00000452514", "l": "", "d": [], "t": []}, {"i": "ENSEMBL:ENSP00000452514.1", "l": "", "d": [], "t": []}], "preferred_name": "G3V5T9_HUMAN Cyclin-dependent kinase 2 (trembl)", "taxa": ["NCBITaxon:9606"]} +{"type": "biolink:Protein", "ic": 86.30756602822301, "identifiers": [{"i": "UniProtKB:P24941", "l": "CDK2_HUMAN Cyclin-dependent kinase 2 (sprot)", "d": [], "t": ["NCBITaxon:9606"]}, {"i": "PR:P24941", "l": "cyclin-dependent kinase 2 (human)", "d": ["A cyclin-dependent kinase 2 that is encoded in the genome of human."], "t": []}, {"i": "UMLS:C0108855", "l": "CDK2 protein, human", "d": [], "t": []}, {"i": "NCIT:C25842", "l": "Cyclin-Dependent Kinase 2", "d": ["Cyclin-dependent kinase 2 (298 aa, ~34 kDa) is encoded by the human CDK2 gene. This protein is involved in the regulation of both phosphorylation of cyclins and the cell cycle progression from the G2 phase to mitosis."], "t": []}, {"i": "MESH:C495894", "l": "CDK2 protein, human", "d": [], "t": []}], "preferred_name": "CDK2_HUMAN Cyclin-dependent kinase 2 (sprot)", "taxa": ["NCBITaxon:9606"]} +{"type": "biolink:Protein", "ic": null, "identifiers": [{"i": "UniProtKB:E7ESI2", "l": "E7ESI2_HUMAN Cyclin-dependent kinase 2 (trembl)", "d": [], "t": ["NCBITaxon:9606"]}, {"i": "ENSEMBL:ENSP00000393605", "l": "", "d": [], "t": []}, {"i": "ENSEMBL:ENSP00000393605.2", "l": "", "d": [], "t": []}], "preferred_name": "E7ESI2_HUMAN Cyclin-dependent kinase 2 (trembl)", "taxa": ["NCBITaxon:9606"]} diff --git a/tests/test_loader_integration.py b/tests/test_loader_integration.py index 27339325..42136206 100644 --- a/tests/test_loader_integration.py +++ b/tests/test_loader_integration.py @@ -9,6 +9,7 @@ """ import json from pathlib import Path +from types import SimpleNamespace import pytest import redis @@ -16,6 +17,8 @@ import node_normalizer.config as config_mod import node_normalizer.loader.loader as loader_mod +import node_normalizer.normalizer as normalizer_mod +from node_normalizer.redis_adapter import RedisConnectionFactory pytestmark = pytest.mark.integration @@ -68,18 +71,28 @@ def loaded_redis(tmp_path, monkeypatch): conflation_members = ["CHEBI:15377", "DRUGBANK:DB00898"] (tmp_path / "DrugChemical.txt").write_text(json.dumps(conflation_members) + "\n") + # A real gene/protein conflation (CDK2): the gene NCBIGene:1017 conflated with + # four UniProtKB protein cliques, gene-first. load_conflation reads it from the + # conflation_directory, so copy the resource file there. + (tmp_path / "GeneProtein.txt").write_text((RESOURCES / "GeneProtein.txt").read_text()) + config = { "compendium_directory": str(RESOURCES), "conflation_directory": str(tmp_path), "biolink_version": "v4.4.3", "test_mode": 0, - "data_files": ["Cell.txt"], + "data_files": ["Cell.txt", "Disease.txt", "PhenotypicFeature.txt", "Gene.txt", "Protein.txt"], "conflations": [ { "types": ["biolink:ChemicalEntity", "biolink:Drug"], "file": "DrugChemical.txt", "redis_db": "chemical_drug_db", - } + }, + { + "types": ["biolink:Gene", "biolink:Protein"], + "file": "GeneProtein.txt", + "redis_db": "gene_protein_db", + }, ], } config_path = tmp_path / "config.json" @@ -92,7 +105,7 @@ def loaded_redis(tmp_path, monkeypatch): loader_mod.load_all(block_size=100) - yield host, port, conflation_members + yield host, port, conflation_members, redis_config_path loader_mod.redis_connect.cache_clear() @@ -102,11 +115,12 @@ def _client(host, port, db): def test_load_populates_correct_databases(loaded_redis): - host, port, conflation_members = loaded_redis + host, port, conflation_members, _redis_config_path = loaded_redis eq_id_to_id = _client(host, port, DB_INDEX["eq_id_to_id_db"]) id_to_eqids = _client(host, port, DB_INDEX["id_to_eqids_db"]) id_to_type = _client(host, port, DB_INDEX["id_to_type_db"]) + info_content = _client(host, port, DB_INDEX["info_content_db"]) chemical_drug = _client(host, port, DB_INDEX["chemical_drug_db"]) # The compendium populated the id databases. @@ -117,6 +131,33 @@ def test_load_populates_correct_databases(loaded_redis): # eq_id_to_id keys are upper-cased; a known Cell.txt id resolves. assert eq_id_to_id.get("UMLS:C0229659".upper()) is not None + # info_content_db is now a clique-property store keyed by canonical id: its value + # is a JSON dict {"preferred_name", "ic"}, not a bare float. Cell.txt has no + # preferred_name, so it loads as "" alongside the ic. + # + # ic here is the *string* "100", not 100.0: Babel encodes ic as a JSON string in + # Cell.txt but as a JSON number in Disease.txt (below), and the loader stores + # instance["ic"] verbatim without coercing, so db 5 preserves whichever type came + # in. _clique_props() tolerates both. + props = json.loads(info_content.get("UMLS:C0229659")) + assert props == {"preferred_name": "", "ic": "100"} + + # A clique that carries both a preferred_name and an IC (Disease.txt) round-trips + # both through db 5, keyed by its canonical id. + disease_props = json.loads(info_content.get("UMLS:C4288892")) + assert disease_props == {"preferred_name": "Infant Acute Undifferentiated Leukemia", "ic": 100.0} + + # Taxa are stored per-identifier in the "t" field of the id_to_eqids_db blob (not + # in db 5), so they round-trip alongside labels and descriptions. PhenotypicFeature.txt + # has a human-taxon clique; its canonical id's identifiers carry the taxon. + pheno_ids = json.loads(id_to_eqids.get("HP:0009278")) + assert pheno_ids[0] == { + "i": "HP:0009278", + "l": "Ulnar deviation of the 4th finger", + "d": ["Displacement of the 4th finger towards the ulnar side (i.e., towards the 5th finger)."], + "t": ["NCBITaxon:9606"], + } + # The conflation landed in chemical_drug_db (db 6)... for member in conflation_members: assert chemical_drug.get(member) is not None @@ -125,3 +166,80 @@ def test_load_populates_correct_databases(loaded_redis): # before the fix chemical_drug_db was db 0, so these would leak into it. for member in conflation_members: assert eq_id_to_id.get(member) is None + + +# The real CDK2 gene/protein conflation: one gene clique conflated with four protein +# cliques, listed gene-first (from tests/resources/GeneProtein.txt). +GENE_PROTEIN_CLIQUE = [ + "NCBIGene:1017", + "UniProtKB:B4DDL9", + "UniProtKB:E7ESI2", + "UniProtKB:G3V5T9", + "UniProtKB:P24941", +] + + +def test_gene_protein_conflation_loaded(loaded_redis): + """Loading side: every member of the CDK2 clique maps to the whole gene-first + conflation list in gene_protein_db, and the gene's preferred_name lands in db 5.""" + host, port, _members, _redis_config_path = loaded_redis + + gene_protein = _client(host, port, DB_INDEX["gene_protein_db"]) + info_content = _client(host, port, DB_INDEX["info_content_db"]) + + # Each clique member (gene and every protein) resolves to the same gene-first list. + for member in GENE_PROTEIN_CLIQUE: + assert json.loads(gene_protein.get(member)) == GENE_PROTEIN_CLIQUE + + # The gene clique's Babel preferred_name is stored in db 5 under its canonical id. + assert json.loads(info_content.get("NCBIGene:1017"))["preferred_name"] == "CDK2" + + +@pytest.mark.asyncio +async def test_query_gene_protein_conflation_uses_gene_preferred_name(loaded_redis): + """Querying side: under gene/protein conflation, any protein in the CDK2 clique + normalizes to the leading gene identifier (NCBIGene:1017) and the gene's + preferred_name ("CDK2") -- not the queried protein's own name. Drives the real + get_normalized_nodes against the freshly loaded Redis.""" + _host, _port, _members, redis_config_path = loaded_redis + + # The frontend builds its connections through this class-level cache; reset it so we + # attach to this test's container rather than a pool left over from another test. + RedisConnectionFactory.connections = {} + await RedisConnectionFactory.create_connection_pool(str(redis_config_path)) + try: + state = SimpleNamespace( + eq_id_to_id_db=RedisConnectionFactory.get_connection("eq_id_to_id_db"), + id_to_eqids_db=RedisConnectionFactory.get_connection("id_to_eqids_db"), + id_to_type_db=RedisConnectionFactory.get_connection("id_to_type_db"), + info_content_db=RedisConnectionFactory.get_connection("info_content_db"), + gene_protein_db=RedisConnectionFactory.get_connection("gene_protein_db"), + chemical_drug_db=RedisConnectionFactory.get_connection("chemical_drug_db"), + # Pre-seeded so get_ancestors doesn't need a Biolink Toolkit for this test. + ancestor_map={"biolink:Gene": ["biolink:Gene"], "biolink:Protein": ["biolink:Protein"]}, + toolkit=None, + ) + app = SimpleNamespace(state=state) + + # Query the canonical protein (P24941) and a trembl protein (B4DDL9); both must + # collapse onto the gene-led clique. + for queried in ["UniProtKB:P24941", "UniProtKB:B4DDL9"]: + result = await normalizer_mod.get_normalized_nodes( + app, [queried], conflate_gene_protein=True, conflate_chemical_drug=False, + ) + assert result[queried]["id"] == {"identifier": "NCBIGene:1017", "label": "CDK2"} + + # Sanity check the other direction: without conflation the protein keeps its own + # identity and preferred_name. + result = await normalizer_mod.get_normalized_nodes( + app, ["UniProtKB:P24941"], conflate_gene_protein=False, conflate_chemical_drug=False, + ) + assert result["UniProtKB:P24941"]["id"] == { + "identifier": "UniProtKB:P24941", + "label": "CDK2_HUMAN Cyclin-dependent kinase 2 (sprot)", + } + finally: + for conn in RedisConnectionFactory.get_all_connections().values(): + conn.close() + await conn.wait_closed() + RedisConnectionFactory.connections = {} diff --git a/tests/test_normalizer.py b/tests/test_normalizer.py index 0e2beea2..d7a27f56 100644 --- a/tests/test_normalizer.py +++ b/tests/test_normalizer.py @@ -13,13 +13,28 @@ # Need to add to sources root to avoid linter warnings from .helpers.redis_mocks import mock_get_equivalent_curies, mock_get_ic +from types import SimpleNamespace + from node_normalizer.normalizer import ( normalize_kgraph, _hash_attributes, _merge_node_attributes, + _clique_props, + create_node, + get_normalized_nodes, ) +class _MgetRedis: + """Minimal stand-in for an aioredis connection: only mget, keyed dict lookup.""" + + def __init__(self, data): + self.data = data + + async def mget(self, *keys, **kwargs): + return [self.data.get(k) for k in keys] + + def find_diffs(x, y, parent_key=None, exclude_keys=[], epsilon_keys=[]): """ Take the diff of JSON-like dictionaries @@ -215,6 +230,82 @@ def test_hashable_attribute(self): # ) # assert _hash_attributes([hashable_attribute]) is False + def test_clique_props(self): + # New JSON format round-trips. + assert _clique_props('{"preferred_name": "Foo", "ic": 100.0}') == {"preferred_name": "Foo", "ic": 100.0} + # Legacy bare float/int/string all normalize to {"ic": ...}. + assert _clique_props("100.0") == {"ic": 100.0} + assert _clique_props("100") == {"ic": 100} + # Missing/absent value -> empty dict. + assert _clique_props(None) == {} + + @pytest.mark.asyncio + async def test_create_node_uses_stored_preferred_name(self): + # When a Babel-computed preferred_name is present, create_node uses it verbatim + # for the clique label -- even a >demote_labels_longer_than (15) name -- and does + # NOT run the fallback algorithm. app=None proves no Redis is touched on this path. + canonical = "MONDO:0011996" + eids = {canonical: [{"i": canonical, "l": "raw list label"}]} + node = await create_node( + app=None, + canonical_id=canonical, + equivalent_ids=eids, + types={canonical: ["biolink:Disease"]}, + info_contents={canonical: None}, + preferred_names={canonical: "Infant Acute Undifferentiated Leukemia"}, + conflations={"GeneProtein": False, "DrugChemical": False}, + ) + assert node["id"] == {"identifier": canonical, "label": "Infant Acute Undifferentiated Leukemia"} + + @pytest.mark.asyncio + async def test_create_node_empty_preferred_name_falls_back(self): + # An empty stored preferred_name ("" -- Babel emitted none) must fall through to + # the legacy algorithm, which picks the clique's own label. No conflation => no Redis. + canonical = "MONDO:0011996" + eids = {canonical: [{"i": canonical, "l": "Leukemia"}]} + node = await create_node( + app=None, + canonical_id=canonical, + equivalent_ids=eids, + types={canonical: ["biolink:Disease"]}, + info_contents={canonical: None}, + preferred_names={canonical: ""}, + conflations={"GeneProtein": False, "DrugChemical": False}, + ) + assert node["id"] == {"identifier": canonical, "label": "Leukemia"} + + @pytest.mark.asyncio + async def test_conflation_uses_leading_identifier_preferred_name(self): + # Under gene/protein conflation, the clique's label must be the *leading* + # (gene) sub-clique's preferred_name, and the node id its leading identifier + # -- not the queried protein's. gene_protein_db lists the members gene-first, + # so get_normalized_nodes merges them gene-first (dereference_ids[c][0]) and + # keys the db-5 preferred_name lookup on that leading id. + gene, protein = "NCBIGene:1017", "UniProtKB:P24941" + app = SimpleNamespace(state=SimpleNamespace( + eq_id_to_id_db=_MgetRedis({protein.upper(): protein}), + id_to_eqids_db=_MgetRedis({ + protein: json.dumps([{"i": protein, "l": "CDK2 protein raw label"}]), + gene: json.dumps([{"i": gene, "l": "CDK2 gene raw label"}]), + }), + id_to_type_db=_MgetRedis({protein: "biolink:Protein", gene: "biolink:Gene"}), + # db 5, keyed by each clique's canonical id: gene carries "CDK2", protein a + # different name we must NOT surface for the conflated clique. + info_content_db=_MgetRedis({ + gene: json.dumps({"preferred_name": "CDK2", "ic": 100.0}), + protein: json.dumps({"preferred_name": "Cyclin-dependent kinase 2", "ic": 100.0}), + }), + gene_protein_db=_MgetRedis({protein: json.dumps([gene, protein])}), + chemical_drug_db=_MgetRedis({}), + ancestor_map={"biolink:Protein": ["biolink:Protein"], "biolink:Gene": ["biolink:Gene"]}, + )) + + result = await get_normalized_nodes( + app, [protein], conflate_gene_protein=True, conflate_chemical_drug=False, + ) + # Leading (gene) identifier and its preferred_name win. + assert result[protein]["id"] == {"identifier": gene, "label": "CDK2"} + def test_merge_node_attributes(self): node_a = { "id": "primary:id",