From caa735bfa85558ba9a927134e3387e7a99c97ce9 Mon Sep 17 00:00:00 2001 From: Gaurav Vaidya Date: Wed, 15 Jul 2026 02:49:28 -0400 Subject: [PATCH 1/5] Speed up the loader and disable Redis snapshots during load The full mode:load takes 0.5-1.5 days, bounded by the two single-threaded, write-heavy Redis instances (eq_id_to_id_db, id_to_eqids_db) that every compendium Job writes to at once. Three low-risk loader changes: - disable_periodic_save(): load_all issues CONFIG SET save "" on each backend at load start. The default `save 300 1000000` forks a multi-GB BGSAVE every few minutes during the write storm, which is wasted work: the backend DBs are write-once and persisted by a manual BGSAVE per the loader SOP. Best-effort (logs and continues if CONFIG SET is refused) and auto-scoped to mode:load, so mode:restore instances keep their periodic-save safety net. - pipeline(transaction=False) everywhere: this is a bulk load, not an atomic update, so the per-block MULTI/EXEC framing is pure overhead. - _accumulate_source_prefixes(): count a line's CURIE prefixes once and fold them into every implied Biolink type, instead of re-splitting each identifier once per ancestor type (was O(ancestors x identifiers) per line). Also ignore the documented nodeNormalization-env venv. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 1 + node_normalizer/loader/__init__.py | 2 + node_normalizer/loader/loader.py | 87 ++++++++++++++++++++++-------- tests/test_loader.py | 30 ++++++++++- 4 files changed, 96 insertions(+), 24 deletions(-) diff --git a/.gitignore b/.gitignore index 0771e22..3a6958f 100644 --- a/.gitignore +++ b/.gitignore @@ -112,6 +112,7 @@ venv/ ENV/ env.bak/ venv.bak/ +nodeNormalization-env/ # Spyder project settings .spyderproject diff --git a/node_normalizer/loader/__init__.py b/node_normalizer/loader/__init__.py index 700a695..283a389 100644 --- a/node_normalizer/loader/__init__.py +++ b/node_normalizer/loader/__init__.py @@ -7,6 +7,7 @@ get_compendia, get_ancestors, redis_connect, + disable_periodic_save, ) __all__ = [ @@ -18,4 +19,5 @@ "get_compendia", "get_ancestors", "redis_connect", + "disable_periodic_save", ] diff --git a/node_normalizer/loader/loader.py b/node_normalizer/loader/loader.py index f561956..16b63f2 100644 --- a/node_normalizer/loader/loader.py +++ b/node_normalizer/loader/loader.py @@ -5,7 +5,7 @@ This is a batch script — read a file, fill a pipeline, execute, repeat — so it is written synchronously against redis-py. It is invoked one file at a time by the loader Helm chart (which generates a single-file config.json per Kubernetes -Job and runs `python load.py`); see documentation/Development.md. +Job and runs `python load.py`); see documentation/Loader.md. """ import json from functools import lru_cache @@ -44,6 +44,23 @@ def get_ancestors(input_type: str) -> list: return ancestors +def _accumulate_source_prefixes(source_prefixes: dict, identifiers: list, semantic_types: list) -> None: + """ + Fold one compendium line's CURIE-prefix counts into every implied semantic + type. The prefixes are counted once for the line and then added to each + ancestor bucket, rather than re-splitting every identifier once per type. + """ + line_prefix_counts: dict = {} + for equivalent_id in identifiers: + source_prefix = equivalent_id["i"].split(":", 1)[0] + line_prefix_counts[source_prefix] = line_prefix_counts.get(source_prefix, 0) + 1 + + for semantic_type in semantic_types: + type_counts = source_prefixes.setdefault(semantic_type, {}) + for source_prefix, count in line_prefix_counts.items(): + type_counts[source_prefix] = type_counts.get(source_prefix, 0) + count + + @lru_cache(maxsize=None) def _load_schema() -> dict: with open(RESOURCES_DIR / "valid_data_format.json") as schema_file: @@ -77,6 +94,34 @@ def redis_connect(db_name: str) -> redis.Redis: ) +def disable_periodic_save() -> None: + """ + Turn off automatic RDB snapshots (`CONFIG SET save ""`) on every backend + Redis for the duration of the load. + + The NodeNorm backend databases are write-once: they are created empty, + populated by a load, then persisted with a single *manual* BGSAVE (per the + loader SOP) and copied elsewhere to seed future `mode: restore` loads. The + default `save 300 1000000` therefore just forks a multi-GB BGSAVE every few + minutes during the write storm for no benefit — on the 150 GB+ databases the + copy-on-write churn is a real drag. A crashed load leaves the database empty + rather than partial, which is fine because loads are re-runnable. + + Best-effort: a server that refuses CONFIG SET (e.g. a locked-down managed + Redis) is logged and skipped rather than failing the load. CONFIG SET save + is server-wide, so this hits each backend instance once. + """ + with open(REDIS_CONFIG_PATH) as config_file: + db_names = list(yaml.load(config_file, yaml.FullLoader).keys()) + + for db_name in db_names: + try: + redis_connect(db_name).config_set("save", "") + logger.info(f"Disabled periodic RDB save on {db_name} for the load.") + except Exception as e: + logger.warning(f"Could not disable periodic save on {db_name}: {e}") + + def validate_compendium(in_file) -> bool: """Validate the first few lines of a compendium against the data schema.""" schema = _load_schema() @@ -118,10 +163,12 @@ def load_compendium(compendium_filename, block_size: int, test_mode: int = 0) -> id2type_redis = redis_connect("id_to_type_db") info_content_redis = redis_connect("info_content_db") - term2id_pipeline = term2id_redis.pipeline() - id2eqids_pipeline = id2eqids_redis.pipeline() - id2type_pipeline = id2type_redis.pipeline() - info_content_pipeline = info_content_redis.pipeline() + # transaction=False: this is a bulk load, not an atomic update, so skip the + # MULTI/EXEC framing around every flushed block. + term2id_pipeline = term2id_redis.pipeline(transaction=False) + id2eqids_pipeline = id2eqids_redis.pipeline(transaction=False) + id2type_pipeline = id2type_redis.pipeline(transaction=False) + info_content_pipeline = info_content_redis.pipeline(transaction=False) line_counter = 0 with open(compendium_filename, "r", encoding="utf-8") as compendium: @@ -139,15 +186,7 @@ def load_compendium(compendium_filename, block_size: int, test_mode: int = 0) -> semantic_types = get_ancestors(instance["type"]) # Accumulate prefix statistics for the leaf type and every ancestor. - for semantic_type in semantic_types: - if source_prefixes.get(semantic_type) is None: - source_prefixes[semantic_type] = {} - for equivalent_id in instance["identifiers"]: - source_prefix = equivalent_id["i"].split(":")[0] - if source_prefixes[semantic_type].get(source_prefix) is None: - source_prefixes[semantic_type][source_prefix] = 1 - else: - source_prefixes[semantic_type][source_prefix] += 1 + _accumulate_source_prefixes(source_prefixes, instance["identifiers"], semantic_types) # The Redis writes are independent of the semantic type, so do them # once per line rather than once per ancestor. @@ -164,10 +203,10 @@ def load_compendium(compendium_filename, block_size: int, test_mode: int = 0) -> id2type_pipeline.execute() info_content_pipeline.execute() - term2id_pipeline = term2id_redis.pipeline() - id2eqids_pipeline = id2eqids_redis.pipeline() - id2type_pipeline = id2type_redis.pipeline() - info_content_pipeline = info_content_redis.pipeline() + term2id_pipeline = term2id_redis.pipeline(transaction=False) + id2eqids_pipeline = id2eqids_redis.pipeline(transaction=False) + id2type_pipeline = id2type_redis.pipeline(transaction=False) + info_content_pipeline = info_content_redis.pipeline(transaction=False) logger.info(f"{line_counter} {compendium_filename} lines processed") @@ -190,7 +229,7 @@ def load_conflation(conflation: dict, conflation_directory: Path, block_size: in """Load a conflation file: each identifier in a clique -> the whole clique line.""" conflation_file = conflation["file"] conflation_redis = redis_connect(conflation["redis_db"]) - conflation_pipeline = conflation_redis.pipeline() + conflation_pipeline = conflation_redis.pipeline(transaction=False) line_counter = 0 with open(f"{conflation_directory}/{conflation_file}", "r", encoding="utf-8") as cfile: @@ -205,7 +244,7 @@ def load_conflation(conflation: dict, conflation_directory: Path, block_size: in if test_mode != 1 and line_counter % block_size == 0: conflation_pipeline.execute() - conflation_pipeline = conflation_redis.pipeline() + conflation_pipeline = conflation_redis.pipeline(transaction=False) logger.info(f"{line_counter} {conflation_file} lines processed") if test_mode != 1: @@ -228,7 +267,7 @@ def merge_semantic_meta_data(test_mode: int = 0) -> None: meta_data_keys = [key for key in types_prefixes_redis.keys("file-*") if key != "semantic_types"] - pipeline = types_prefixes_redis.pipeline() + pipeline = types_prefixes_redis.pipeline(transaction=False) for meta_data_key in meta_data_keys: pipeline.get(meta_data_key) meta_data = pipeline.execute() @@ -245,7 +284,7 @@ def merge_semantic_meta_data(test_mode: int = 0) -> None: for curie_prefix, count in curie_counts.items(): sources_prefix[bl_type][curie_prefix] = sources_prefix[bl_type].get(curie_prefix, 0) + count - pipeline = types_prefixes_redis.pipeline() + pipeline = types_prefixes_redis.pipeline(transaction=False) if sources_prefix: pipeline.lpush("semantic_types", *list(sources_prefix.keys())) for bl_type, counts in sources_prefix.items(): @@ -269,6 +308,8 @@ def load_all(block_size: int = 100_000) -> bool: if test_mode == 1: logger.debug("Test mode enabled. No data will be produced.") + else: + disable_periodic_save() compendia = get_compendia(compendium_directory, data_files) if len(compendia) != len(data_files): @@ -283,7 +324,7 @@ def load_all(block_size: int = 100_000) -> bool: source_prefixes = load_compendium(comp, block_size, test_mode) - pipeline = types_prefixes_redis.pipeline() + pipeline = types_prefixes_redis.pipeline(transaction=False) # @TODO add meta data about files eg. checksum to this object pipeline.set(f"file-{comp}", json.dumps({"source_prefixes": source_prefixes})) if test_mode != 1: diff --git a/tests/test_loader.py b/tests/test_loader.py index 1a79543..83d0a9c 100644 --- a/tests/test_loader.py +++ b/tests/test_loader.py @@ -3,6 +3,7 @@ import node_normalizer.loader.loader as loader_mod from node_normalizer.loader import load_compendium, validate_compendium +from node_normalizer.loader.loader import _accumulate_source_prefixes good_json = Path(__file__).parent / "resources" / "datafile.json" @@ -36,7 +37,7 @@ class _FakeRedis: def __init__(self): self.sets = [] - def pipeline(self): + def pipeline(self, transaction=True): return _FakePipeline(self) @@ -57,3 +58,30 @@ def fake_connect(db_name): num_lines = sum(1 for line in open(good_json) if line.strip()) assert len(fakes["id_to_eqids_db"].sets) == num_lines assert len(fakes["id_to_type_db"].sets) == num_lines + + +def test_accumulate_source_prefixes(): + """ + The per-line prefix counter must fold each identifier's prefix into every + implied semantic type, accumulating across lines. This pins the behaviour + of the optimized (count-once-per-line) accumulation. + """ + source_prefixes = {} + + # Line 1: two NCBIGene + one ENSEMBL, implied by Gene and its ancestors. + _accumulate_source_prefixes( + source_prefixes, + [{"i": "NCBIGene:1"}, {"i": "NCBIGene:2"}, {"i": "ENSEMBL:3"}], + ["biolink:Gene", "biolink:BiologicalEntity"], + ) + # Line 2: one NCBIGene, only Gene (a leaf-only type here). + _accumulate_source_prefixes( + source_prefixes, + [{"i": "NCBIGene:9"}], + ["biolink:Gene"], + ) + + assert source_prefixes == { + "biolink:Gene": {"NCBIGene": 3, "ENSEMBL": 1}, + "biolink:BiologicalEntity": {"NCBIGene": 2, "ENSEMBL": 1}, + } From 4b9a774f7283b1fd67fba8445f24b31d43ada370 Mon Sep 17 00:00:00 2001 From: Gaurav Vaidya Date: Wed, 15 Jul 2026 02:49:52 -0400 Subject: [PATCH 2/5] Split loader/loading docs into Loader.md; reorganize dev docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit documentation/Development.md mixed loader internals, general dev setup, and the frontend. Split it: - Loader.md: everything loader — mode:load/restore, how a load runs, the write-once DB lifecycle, why saves are disabled during load, the perf choices (transaction=False, prefix loop), the integration-test gotchas, the requests/docker landmine, and the v2.6.0 loader-speed backlog (#387-390). Absorbs the load-process content. - CONTRIBUTOR.md: two-tools overview, running locally, tests, a short Frontend section, and the project-wide v2.5.0 backlog (#379-383). - Development.md: removed. Updates the references in CLAUDE.md and the loader module docstring. Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 4 +- documentation/CONTRIBUTOR.md | 88 ++++++++++++++++++++ documentation/Development.md | 145 --------------------------------- documentation/Loader.md | 152 +++++++++++++++++++++++++++++++++++ 4 files changed, 242 insertions(+), 147 deletions(-) create mode 100644 documentation/CONTRIBUTOR.md delete mode 100644 documentation/Development.md create mode 100644 documentation/Loader.md diff --git a/CLAUDE.md b/CLAUDE.md index 8e50556..e213c6a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ NodeNormalization is a FastAPI microservice for the NIH NCATS Translator project Data comes from Babel (identifier equivalence project) and is stored in Redis across 7 separate databases. Standalone Redis only (cluster mode was removed; see `documentation/Redis.md`). -See `documentation/Development.md` for how the frontend and loader are organized and how a data load actually runs. +See `documentation/CONTRIBUTOR.md` for how the frontend and loader are organized and how to run NodeNorm locally, and `documentation/Loader.md` for how a data load actually runs. **Scratch space:** the `data/` directory is git-ignored; use it for scratch/working files in preference to `/tmp`. @@ -73,7 +73,7 @@ Babel Compendia Files → loader.py → Redis (7 DBs) → FastAPI (server.py) - **`node_normalizer/server.py`** — FastAPI app with all REST endpoints. Uses lifespan events for Redis connection setup/teardown. Root path is `/1.3`. - **`node_normalizer/normalizer.py`** — Core logic: `get_normalized_nodes()`, `normalize_message()` (for TRAPI), and equivalent CURIE discovery. Traverses Biolink Model ancestors for semantic type expansion. -- **`node_normalizer/loader/`** — the data loader: synchronous module-level functions (`load_all`, `load_compendium`, `load_conflation`, `merge_semantic_meta_data`) that read flat compendia files and populate Redis. Validates input against `resources/valid_data_format.json`. Batch size: 100,000. Invoked via the root `load.py`. See `documentation/Development.md`. +- **`node_normalizer/loader/`** — the data loader: synchronous module-level functions (`load_all`, `load_compendium`, `load_conflation`, `merge_semantic_meta_data`) that read flat compendia files and populate Redis. Validates input against `resources/valid_data_format.json`. Batch size: 100,000. Invoked via the root `load.py`. `load_all` disables Redis periodic saves (`CONFIG SET save ""`) for the duration of the load — the backend DBs are write-once and persisted by a manual BGSAVE, so snapshotting during the load is wasted work. See `documentation/Loader.md`. - **`node_normalizer/config.py`** — shared repo-relative paths (`config.json`, `redis_config.yaml`, `resources/`) and `get_config()`. Used by both frontend and loader. - **`node_normalizer/redis_adapter.py`** — async `RedisConnection`/`RedisConnectionFactory` over `aioredis`, used by the frontend. Standalone Redis only; cluster mode was removed (see `documentation/Redis.md`). - **`node_normalizer/model/`** — Pydantic request/response models (`input.py`, `response.py`). diff --git a/documentation/CONTRIBUTOR.md b/documentation/CONTRIBUTOR.md new file mode 100644 index 0000000..a5eca73 --- /dev/null +++ b/documentation/CONTRIBUTOR.md @@ -0,0 +1,88 @@ +# Contributing to NodeNorm + +How NodeNorm is put together and how to run it locally. For the two large +subsystems see [Loader.md](Loader.md) (the data loader) and [Redis.md](Redis.md) +(the backend database layout). + +## Two tools in one repo + +NodeNorm is really two programs that share some code: + +- **The frontend** — a FastAPI service (`node_normalizer/server.py`) that answers + normalization queries by reading the backend Redis databases. This is what runs + in production behind `/1.3`. See [Frontend](#frontend) below. +- **The loader** — a batch program (`node_normalizer/loader/`, invoked via the + root `load.py`) that reads Babel compendium/conflation files and populates those + Redis databases. See [Loader.md](Loader.md). + +Shared code lives directly in `node_normalizer/` (`config.py`, `util.py`, +`normalizer.py`, `redis_adapter.py`, `model/`). Loader-only code lives in +`node_normalizer/loader/`. The frontend has **no** dependency on the loader. + +`node_normalizer/config.py` is the single source of truth for repo-relative paths +(`config.json`, `redis_config.yaml`, `resources/`). Prefer it over recomputing +`Path(__file__).parents[N]`. + +## Running locally + +```bash +# Start a local Redis (all 7 logical DBs live in one instance, distinct db index) +docker compose -f docker-compose-redis.yml up -d + +# Point config.json at your compendium/conflation directory, then load (see Loader.md): +python load.py + +# Run the frontend: +uvicorn --host 0.0.0.0 --port 8000 --workers 1 node_normalizer.server:app +``` + +Tests: + +```bash +pip install -r requirements.txt -r requirements-loader.txt -r requirements-test.txt + +pytest -m "not integration" # unit tests, no Docker needed +pytest -m integration # loader against a real Redis (needs Docker) +``` + +Integration tests are marked `integration` and start a real Redis via +`testcontainers`. CI runs both in `.github/workflows/test.yml`. For the loader +test gotchas and the requests/docker/testcontainers landmine, see +[Loader.md](Loader.md#writing-loader-integration-tests). + +## Frontend + +The FastAPI app in `node_normalizer/server.py` serves the normalization API under +root path `/1.3` (`GET/POST /get_normalized_nodes`, `/query`, `/get_setid`, +`/get_semantic_types`, `/get_curie_prefixes`, `/status`). It reads the backend +Redis databases via the async `RedisConnection` in `node_normalizer/redis_adapter.py` +and never touches the loader. Core logic lives in `node_normalizer/normalizer.py`; +request/response models in `node_normalizer/model/`. Run it with the `uvicorn` +command above; interactive docs are at `http://localhost:8000/docs`. + +## Backlog + +Larger cleanups that are out of scope for any single PR, filed as issues on the +**NodeNorm v2.5.0** milestone: + +- **`semantic_types` should be a Redis SET, not a LIST** ([#379](https://github.com/NCATSTranslator/NodeNormalization/issues/379)). + `merge_semantic_meta_data` `LPUSH`es into `semantic_types`, and because it runs + once per loader Job the list ends up with one copy of every type per file. The + frontend papers over this by deduping on read. The proper fix (`SADD`/`SMEMBERS`) + is a stored-format change, and `mode: restore` loads RDB backups built by the + *old* loader — a new server doing `SMEMBERS` on a restored LIST would get + `WRONGTYPE`. Needs a coordinated loader/server/backup rollout. +- **`merge_semantic_meta_data` runs once per Job** ([#380](https://github.com/NCATSTranslator/NodeNormalization/issues/380)). + It should run once, after all compendium Jobs finish, rather than re-reading + every `file-*` key and re-summing on every Job. +- **Migrate the frontend off `aioredis`** ([#381](https://github.com/NCATSTranslator/NodeNormalization/issues/381)), + unmaintained and pinned at 1.3.1. `redis-py` >= 4.2 has asyncio built in, which + would let the frontend and loader share one client library. +- **`uv` / `pyproject.toml` migration** ([#382](https://github.com/NCATSTranslator/NodeNormalization/issues/382)). + Turn this into a proper package with dependency groups, and replace the root + `load.py` shim with a `nodenorm-load` console script (which will require a + matching chart change). +- **Wire the docker-compose-based tests into CI** ([#383](https://github.com/NCATSTranslator/NodeNormalization/issues/383)). + `.github/workflows/test.yml` runs the headless unit + loader-integration tests; + `test_endpoints.py` and `test_callback.py` still need porting off the legacy + docker-compose harness. diff --git a/documentation/Development.md b/documentation/Development.md deleted file mode 100644 index cebf8ba..0000000 --- a/documentation/Development.md +++ /dev/null @@ -1,145 +0,0 @@ -# NodeNorm development notes - -This document records how NodeNorm is built and, in particular, how a data load -actually works — details that are otherwise scattered across this repo and the -[translator-devops](https://github.com/helxplatform/translator-devops) Helm -charts. It also lists larger cleanups that are out of scope for any single PR. - -## Two tools in one repo - -NodeNorm is really two programs that share some code: - -- **The frontend** — a FastAPI service (`node_normalizer/server.py`) that answers - normalization queries by reading the backend Redis databases. This is what runs - in production behind `/1.3`. -- **The loader** — a batch program (`node_normalizer/loader/`, invoked via the - root `load.py`) that reads Babel compendium/conflation files and populates those - Redis databases. - -Shared code lives directly in `node_normalizer/` (`config.py`, `util.py`, -`normalizer.py`, `redis_adapter.py`, `model/`). Loader-only code lives in -`node_normalizer/loader/`. The frontend has **no** dependency on the loader. - -`node_normalizer/config.py` is the single source of truth for repo-relative -paths (`config.json`, `redis_config.yaml`, `resources/`). Prefer it over -recomputing `Path(__file__).parents[N]`. - -## How a data load actually runs - -Loading is driven by the `node-normalization-loader` Helm chart in the -translator-devops repo, which has two modes selected by `values.yaml: mode`. - -### `mode: load` — the Python path - -The chart generates, **per file**, a `config.json` containing a single-element -`data_files` (or a single-element `conflations`) list, mounts it at -`/code/config.json`, and runs a small shell script whose payload is literally -`python load.py`. It creates **one Kubernetes Job per compendium file** (dozens, -in parallel) and one per conflation. - -Consequences worth knowing: - -- The image used is the **main webserver image** (root `Dockerfile`, `WORKDIR - /code`). That is why the webserver image ships the loader and its dependencies - (`requirements.txt` **and** `requirements-loader.txt`). -- `load.py` takes **no arguments**; everything comes from `config.json`. The - loader reads exactly five keys: `compendium_directory`, `conflation_directory`, - `test_mode`, `data_files`, `conflations`. -- Redis connection details come **only** from `redis_config.yaml`. In Kubernetes - the chart overwrites it with a generated ConfigMap, so the copy checked into - this repo governs **local dev and tests only**. -- Each Job runs `merge_semantic_meta_data()` at the end, so that aggregation - currently runs once per file rather than once per load (see backlog below). - -### `mode: restore` — no Python - -Downloads pre-built `.rdb.gz` Redis backups and pipes them straight into Redis -with `redis-cli --pipe`. No Python runs. This is what the `data-loading/` image -is for: it is a `redis:latest` base that compiles `rdb-cli` from -[redis/librdb](https://github.com/redis/librdb) and ships -`data-loading/rdb-to-resp.sh`, which the chart calls as -`bash rdb-to-resp.sh | redis-cli --pipe`. - -(The older chart used `rdb -c protocol` from `redis-rdb-tools`, which does not -work against Redis > 6; that was replaced by `rdb-cli` in PR #349.) - -## Redis databases - -See [Redis.md](Redis.md) for the database layout and what each one stores. - -## Running locally - -```bash -# Start a local Redis (all 7 logical DBs live in one instance, distinct db index) -docker compose -f docker-compose-redis.yml up -d - -# Point config.json at your compendium/conflation directory, then: -python load.py - -# Run the frontend: -uvicorn --host 0.0.0.0 --port 8000 --workers 1 node_normalizer.server:app -``` - -Tests: - -```bash -pip install -r requirements.txt -r requirements-loader.txt -r requirements-test.txt - -pytest -m "not integration" # unit tests, no Docker needed -pytest -m integration # loader against a real Redis (needs Docker) -``` - -Integration tests are marked `integration` and start a real Redis via -`testcontainers` (see `tests/test_loader_integration.py`). CI runs both in -`.github/workflows/test.yml`. - -### Writing loader integration tests - -`tests/test_loader_integration.py` is the pattern to copy. Two non-obvious -things about driving the loader from a test: - -- **`redis_connect` is `@lru_cache`d.** It memoizes one client per db name for - the life of the process, so call `loader.loader.redis_connect.cache_clear()` - before and after a test that points it at a throwaway Redis — otherwise a - later test reuses the previous container's (now-dead) connection. -- **Patch paths on the loader module, not on `config`.** The loader does - `from ..config import get_config, REDIS_CONFIG_PATH`, so it holds its *own* - reference to `REDIS_CONFIG_PATH`. To redirect it, `monkeypatch.setattr` on - `node_normalizer.loader.loader.REDIS_CONFIG_PATH`. `CONFIG_PATH` is different: - `get_config()` reads `config.CONFIG_PATH` at call time, so patch it on - `node_normalizer.config`. - -### Dependency landmine: requests / docker / testcontainers - -`requests >= 2.32` changed its `HTTPAdapter`, which breaks the `docker` SDK -< 7.1 that `testcontainers` pulls in — its unix-socket adapter can no longer -reach the Docker daemon, and `testcontainers` (hence every `integration` test) -fails at container startup. If you bump `requests`, keep `docker>=7.1.0` pinned -in `requirements-test.txt`. This is easy to misdiagnose as a Docker/environment -problem rather than a dependency conflict. - -## Backlog (out of scope for the loader-reorg PR) - -Filed as issues on the **NodeNorm v2.5.0** milestone: - -- **`semantic_types` should be a Redis SET, not a LIST** ([#379](https://github.com/NCATSTranslator/NodeNormalization/issues/379)). - `merge_semantic_meta_data` `LPUSH`es into `semantic_types`, and because it runs - once per loader Job the list ends up with one copy of every type per file. The - frontend papers over this by deduping on read. The proper fix (`SADD`/`SMEMBERS`) - is a stored-format change, and `mode: restore` loads RDB backups built by the - *old* loader — a new server doing `SMEMBERS` on a restored LIST would get - `WRONGTYPE`. Needs a coordinated loader/server/backup rollout. -- **`merge_semantic_meta_data` runs once per Job** ([#380](https://github.com/NCATSTranslator/NodeNormalization/issues/380)). - It should run once, after all compendium Jobs finish, rather than re-reading - every `file-*` key and re-summing on every Job. -- **Migrate the frontend off `aioredis`** ([#381](https://github.com/NCATSTranslator/NodeNormalization/issues/381)), - unmaintained and pinned at 1.3.1. `redis-py` >= 4.2 has asyncio built in, which - would let the frontend and loader share one client library. -- **`uv` / `pyproject.toml` migration** ([#382](https://github.com/NCATSTranslator/NodeNormalization/issues/382)). - Turn this into a proper package with dependency groups, and replace the root - `load.py` shim with a `nodenorm-load` console script (which will require a - matching chart change). -- **Wire the docker-compose-based tests into CI** ([#383](https://github.com/NCATSTranslator/NodeNormalization/issues/383)). - `.github/workflows/test.yml` runs the headless unit + loader-integration tests; - `test_endpoints.py` and `test_callback.py` still need porting off the legacy - docker-compose harness. diff --git a/documentation/Loader.md b/documentation/Loader.md new file mode 100644 index 0000000..d9e0582 --- /dev/null +++ b/documentation/Loader.md @@ -0,0 +1,152 @@ +# The loader + +Everything about the NodeNorm data loader in one place: what it does, how a load +runs in production, how to develop and test it, and why it's tuned the way it is. + +The loader is a batch program — `node_normalizer/loader/`, invoked via the root +`load.py` — that reads Babel compendium/conflation JSONL files and populates the +backend Redis databases the frontend queries. It is synchronous `redis-py` code +(read a file, fill a pipeline, execute, repeat). For the database layout and what +each database stores, see [Redis.md](Redis.md); for the project as a whole, see +[CONTRIBUTOR.md](CONTRIBUTOR.md). + +## The two ways to populate the backend + +The `node-normalization-loader` Helm chart (in the `translator-devops` repo) has +two modes, selected by `values.yaml: mode`: + +- **`mode: load`** — the Python path. Runs `python load.py` (this loader). One + Kubernetes Job per file. This is the slow, once-per-Babel-release step described + below. +- **`mode: restore`** — no Python. Downloads pre-built `.rdb.gz` backups and pipes + them straight into Redis with `redis-cli --pipe`. This is how a database is + normally stood up; it only exists because someone ran `mode: load` once and + saved the result. It uses the `data-loading/` image: a `redis:latest` base that + compiles `rdb-cli` from [redis/librdb](https://github.com/redis/librdb) and + ships `data-loading/rdb-to-resp.sh`, which the chart calls as + `bash rdb-to-resp.sh | redis-cli --pipe`. (The older + chart used `rdb -c protocol` from `redis-rdb-tools`, which does not work against + Redis > 6; replaced by `rdb-cli` in PR #349.) + +## How `mode: load` runs + +The chart generates, **per file**, a `config.json` containing a single-element +`data_files` (or a single-element `conflations`) list, mounts it at +`/code/config.json`, and runs a small shell script whose payload is literally +`python load.py`. It creates **one Kubernetes Job per compendium file** and one +per conflation. Large compendia are pre-split into 10M-line chunks +(`split -d -l 10000000 …`), so `SmallMolecule.txt` / `Protein.txt` become dozens +of Jobs. Cluster capacity means ~5–6 Jobs run at once; a full load takes roughly +0.5–1.5 days. + +Consequences worth knowing: + +- The image used is the **main webserver image** (root `Dockerfile`, `WORKDIR + /code`). That is why the webserver image ships the loader and its dependencies + (`requirements.txt` **and** `requirements-loader.txt`). +- `load.py` takes **no arguments**; everything comes from `config.json`. The + loader reads exactly five keys: `compendium_directory`, `conflation_directory`, + `test_mode`, `data_files`, `conflations`. +- Redis connection details come **only** from `redis_config.yaml`. In Kubernetes + the chart overwrites it with a generated ConfigMap, so the copy checked into + this repo governs **local dev and tests only**. +- Each Job runs `merge_semantic_meta_data()` at the end, so that aggregation + currently runs once per file rather than once per load (see #380). + +### Where the time goes + +The seven backend databases are **separate single-threaded Redis instances**. The +two write-heavy ones are the ceiling, because **every** compendium Job writes to +them at the same time: + +- `eq_id_to_id_db` — one `SET` per *equivalent identifier* (N per input line). +- `id_to_eqids_db` — one `SET` per line, but the values are large JSON blobs; this + is the biggest database (150–220 GB). + +So load throughput is bounded by how fast those two single-threaded servers can +absorb writes, not by the number of Jobs. + +## The database lifecycle: write-once + +A backend database is **created empty, loaded once, then frozen**: + +1. Stand up empty Redis instances (`redis-r3-external` Helm chart). +2. Populate them with `mode: load`. +3. **Manually** `BGSAVE` each instance to flush it to disk (per the loader + [README](https://github.com/helxplatform/translator-devops/blob/develop/helm/node-normalization-loader/README.md)), + then copy the resulting `dump.rdb` files elsewhere. +4. Those RDB files seed all future instances via `mode: restore`. + +Nothing writes to a database after its initial load. This is why the loader can +safely disable snapshotting during the load (next section). + +## Why the loader disables periodic saves + +The `redis-r3-external` instances default to `--save 300 1000000` (snapshot every +5 min if ≥1M keys changed). During a load every hot database easily clears that +threshold, so Redis forks a **multi-GB BGSAVE every few minutes** throughout the +write storm. On the 150 GB+ databases the copy-on-write churn from forking under +continuous writes is a real, pure-waste drag — the periodic snapshot is thrown +away anyway, because persistence is the deliberate manual `BGSAVE` in step 3. + +So `load_all()` calls `disable_periodic_save()`, which issues `CONFIG SET save ""` +on each backend at the start of the load. Notes: + +- **Scoped to `mode: load` automatically.** `mode: restore` runs no Python, so its + instances keep their periodic-save safety net untouched. (That matters: the + restore Job's own persistence step is less robust — see #390.) +- **Best-effort.** A server that refuses `CONFIG SET` is logged and skipped, not + fatal. +- **Accepted trade-off:** a crashed load leaves the database *empty* rather than + partially populated. That's fine — loads are re-runnable, and re-running + overwrites cleanly. + +## Other loader performance choices + +- **`pipeline(transaction=False)`** everywhere — this is a bulk load, not an atomic + update, so the per-block `MULTI`/`EXEC` framing is pure overhead. +- **Prefix stats counted once per line.** `_accumulate_source_prefixes` computes a + line's CURIE-prefix counts once and folds them into every implied Biolink type, + instead of re-splitting each identifier once per ancestor type. + +## Writing loader integration tests + +`tests/test_loader_integration.py` is the pattern to copy. Two non-obvious things +about driving the loader from a test: + +- **`redis_connect` is `@lru_cache`d.** It memoizes one client per db name for the + life of the process, so call `loader.loader.redis_connect.cache_clear()` before + and after a test that points it at a throwaway Redis — otherwise a later test + reuses the previous container's (now-dead) connection. +- **Patch paths on the loader module, not on `config`.** The loader does + `from ..config import get_config, REDIS_CONFIG_PATH`, so it holds its *own* + reference to `REDIS_CONFIG_PATH`. To redirect it, `monkeypatch.setattr` on + `node_normalizer.loader.loader.REDIS_CONFIG_PATH`. `CONFIG_PATH` is different: + `get_config()` reads `config.CONFIG_PATH` at call time, so patch it on + `node_normalizer.config`. + +### Dependency landmine: requests / docker / testcontainers + +`requests >= 2.32` changed its `HTTPAdapter`, which breaks the `docker` SDK < 7.1 +that `testcontainers` pulls in — its unix-socket adapter can no longer reach the +Docker daemon, and `testcontainers` (hence every `integration` test) fails at +container startup. If you bump `requests`, keep `docker>=7.1.0` pinned in +`requirements-test.txt`. This is easy to misdiagnose as a Docker/environment +problem rather than a dependency conflict. + +## Backlog + +Bigger loader-speed ideas that need more than a contained change are filed on the +**NodeNorm v2.6.0** milestone: + +- **`MSET`-batch the high-cardinality writes** ([#387](https://github.com/NCATSTranslator/NodeNormalization/issues/387)) + — cut command count on the shared single-threaded servers. Held out of the 2.5 + work because it's the one change that could *silently* drop data if the flush + logic is wrong; needs a correctness test first. +- **Upgrade `redis-py` 3.5.3 → 4/5 (+ hiredis)** ([#388](https://github.com/NCATSTranslator/NodeNormalization/issues/388)) + — faster client, and ties into migrating the frontend off `aioredis` (#381). +- **Overlap the four per-block pipeline flushes** ([#389](https://github.com/NCATSTranslator/NodeNormalization/issues/389)) + — they target independent databases but currently flush serially. +- **Harden the `mode: restore` BGSAVE** ([#390](https://github.com/NCATSTranslator/NodeNormalization/issues/390)) + — it's fire-and-forget, so a restored database may not be persisted before the + Job exits. (A `translator-devops` chart fix.) From 66933991cc7656280140eafe8a5d032871c65181 Mon Sep 17 00:00:00 2001 From: Gaurav Vaidya Date: Wed, 15 Jul 2026 05:02:16 -0400 Subject: [PATCH 3/5] Document how to benchmark a loader change locally Captures the local benchmark recipe (subset via head, redis:6.2-alpine, drive load_all with a temp config.json) and the three non-obvious gotchas: warm the bmt toolkit before timing (it downloads the pinned biolink model), CONFIG SET save is server-wide and sticky across runs, and identical per-db DBSIZE across runs is a free correctness check. Co-Authored-By: Claude Opus 4.8 --- documentation/Loader.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/documentation/Loader.md b/documentation/Loader.md index 6784b5b..3e5bbe8 100644 --- a/documentation/Loader.md +++ b/documentation/Loader.md @@ -112,6 +112,42 @@ on each backend at the start of the load. Notes: line's CURIE-prefix counts once and folds them into every implied Biolink type, instead of re-splitting each identifier once per ancestor type. +## Benchmarking a loader change locally + +To check whether a loader change actually helps (and, just as important, doesn't +change what gets written), time a real load against a local Redis: + +- **Use a subset of a real split.** `head -n 2000000 SmallMolecule.txt.NN > subset.txt` + keeps memory/time sane on a laptop while still crossing Redis's save thresholds. +- **Drive `load_all` with a throwaway config.** Point it at the subset by + monkeypatching `node_normalizer.config.CONFIG_PATH` to a temp `config.json` + (remember `biolink_version` is now required); the repo `redis_config.yaml` + already targets `127.0.0.1:6379`. Run Redis with `docker run -p 6379:6379 + redis:6.2-alpine` (6.2 matches production). +- **Compare against the baseline** by checking out the old loader into the working + tree between runs: `git checkout -- node_normalizer/loader`, run, then + `git checkout -- node_normalizer/loader` and run again. + +Three gotchas that will otherwise skew the numbers: + +- **Warm the Biolink toolkit before timing.** The first `get_ancestors` call builds + `bmt.Toolkit`, which *downloads* the pinned biolink-model YAML over the network — + a large fixed cost that has nothing to do with load speed. Call + `loader.get_ancestors("biolink:")` once before the timed section. +- **`CONFIG SET save` is server-wide and sticky.** `disable_periodic_save` (or a + manual `CONFIG SET save ""`) persists on the instance across loads, so a later + run silently inherits it. Reset `save` to a known value before each run when + measuring the snapshot effect. +- **Correctness check is free:** identical `DBSIZE` on every database across runs + means the change preserved output. (`eq_id_to_id_db` exceeds the line count — + one key per equivalent identifier, not per line.) + +Caveat: a local single-instance Redis with a small subset *understates* the +snapshot-disable win — on a 2M-key DB a BGSAVE fork is cheap, whereas the whole +point is the 150 GB+ production instances. It cleanly measures the client-side +wins (`transaction=False`, prefix-once), which land at ~11% on Protein and more on +files with deeper type hierarchies / larger cliques. + ## Writing loader integration tests `tests/test_loader_integration.py` is the pattern to copy. Two non-obvious things From 3c257b92bf6b3c05d66610c70cfaace2ea76b0e3 Mon Sep 17 00:00:00 2001 From: Gaurav Vaidya Date: Wed, 15 Jul 2026 05:08:42 -0400 Subject: [PATCH 4/5] Apply suggestion from @gaurav --- documentation/Loader.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/Loader.md b/documentation/Loader.md index 3e5bbe8..7952f33 100644 --- a/documentation/Loader.md +++ b/documentation/Loader.md @@ -123,7 +123,7 @@ change what gets written), time a real load against a local Redis: monkeypatching `node_normalizer.config.CONFIG_PATH` to a temp `config.json` (remember `biolink_version` is now required); the repo `redis_config.yaml` already targets `127.0.0.1:6379`. Run Redis with `docker run -p 6379:6379 - redis:6.2-alpine` (6.2 matches production). + redis:8.2.1-bookworm` (8.2 matches production). - **Compare against the baseline** by checking out the old loader into the working tree between runs: `git checkout -- node_normalizer/loader`, run, then `git checkout -- node_normalizer/loader` and run again. From e0ba93ee29e72fa7d07203653317833332504b50 Mon Sep 17 00:00:00 2001 From: Gaurav Vaidya Date: Wed, 15 Jul 2026 05:27:50 -0400 Subject: [PATCH 5/5] Document explicit-persistence Redis policy for the loader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework "Why the loader disables periodic saves" into "Why persistence is explicit, not periodic": the redis-r3-external instances are now configured with `save ""` (no automatic snapshots), and persistence is driven explicitly per mode — mode:load disables saves and BGSAVEs manually; mode:restore's run_pipe.sh now BGSAVEs and waits. Mark #390 as addressed in the chart. The chart side lives in translator-devops PR #1044. Co-Authored-By: Claude Opus 4.8 --- documentation/Loader.md | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/documentation/Loader.md b/documentation/Loader.md index 7952f33..c3fc5ef 100644 --- a/documentation/Loader.md +++ b/documentation/Loader.md @@ -83,21 +83,27 @@ A backend database is **created empty, loaded once, then frozen**: Nothing writes to a database after its initial load. This is why the loader can safely disable snapshotting during the load (next section). -## Why the loader disables periodic saves +## Why persistence is explicit, not periodic + +The `redis-r3-external` instances are configured with **`save ""`** (no automatic +RDB snapshots) and `appendonly no`. That is deliberate: no `save ` +policy can do what a populate needs. The dirty-key counter blows past any +threshold within seconds of a load starting, so any policy that eventually +snapshots *after* a load also forks a **multi-GB BGSAVE repeatedly during** it — +pure copy-on-write waste on the 150 GB+ databases, and the periodic snapshots are +thrown away anyway. So persistence is driven explicitly, exactly once, per mode: + +- **`mode: load`** — `load_all()` calls `disable_periodic_save()` (`CONFIG SET + save ""`) at the start as belt-and-suspenders, then you BGSAVE all seven + instances manually at the end (step 3 above) and copy the RDBs off. +- **`mode: restore`** — `run_pipe.sh` streams the RDB in, then issues a `BGSAVE` + **and waits for it to finish and verify** (`LASTSAVE` advanced, + `rdb_last_bgsave_status:ok`) before the Job exits, so a pod restart reloads a + complete `dump.rdb`. This wait is the fix for the old fire-and-forget BGSAVE + (#390). + +`disable_periodic_save()` notes: -The `redis-r3-external` instances default to `--save 300 1000000` (snapshot every -5 min if ≥1M keys changed). During a load every hot database easily clears that -threshold, so Redis forks a **multi-GB BGSAVE every few minutes** throughout the -write storm. On the 150 GB+ databases the copy-on-write churn from forking under -continuous writes is a real, pure-waste drag — the periodic snapshot is thrown -away anyway, because persistence is the deliberate manual `BGSAVE` in step 3. - -So `load_all()` calls `disable_periodic_save()`, which issues `CONFIG SET save ""` -on each backend at the start of the load. Notes: - -- **Scoped to `mode: load` automatically.** `mode: restore` runs no Python, so its - instances keep their periodic-save safety net untouched. (That matters: the - restore Job's own persistence step is less robust — see #390.) - **Best-effort.** A server that refuses `CONFIG SET` is logged and skipped, not fatal. - **Accepted trade-off:** a crashed load leaves the database *empty* rather than @@ -187,5 +193,6 @@ Bigger loader-speed ideas that need more than a contained change are filed on th - **Overlap the four per-block pipeline flushes** ([#389](https://github.com/NCATSTranslator/NodeNormalization/issues/389)) — they target independent databases but currently flush serially. - **Harden the `mode: restore` BGSAVE** ([#390](https://github.com/NCATSTranslator/NodeNormalization/issues/390)) - — it's fire-and-forget, so a restored database may not be persisted before the - Job exits. (A `translator-devops` chart fix.) + — *addressed in the `translator-devops` chart*: `run_pipe.sh` now BGSAVEs and + waits for completion (see "Why persistence is explicit, not periodic" above). + Close #390 once that chart change ships.