Repository navigation
Conversation
/get_semantic_types collects types from a terms aggregation on `type`.
Its bucket keys are the indexed keyword values, which the index mapping
lowercases ("biolink:smallmolecule"); /get_normalized_nodes reads
`_source`, which keeps the case. biothings#58 switched the ancestors to bmt's
class_uri but still added the raw bucket key, so CI returned both
"biolink:smallmolecule" and "biolink:SmallMolecule".
Each key is now looked up with toolkit.get_element(), which matches
case-insensitively, and reported as its class_uri; a key the model
doesn't know is reported unchanged. No reindex is needed.
The test's fake Elasticsearch returned "biolink:Disease" as the bucket
key, which is why it missed this. It now returns the lowercased key
real indices give, and the test also rejects case-insensitive
duplicates.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gaurav
added a commit
to TranslatorSRI/babel-validation
that referenced
this pull request
Nov 7, 2026
bmt's Toolkit wraps get_element() and get_ancestors() in lru_cache, so a cache in the handler would add nothing; say so where someone would think of adding one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gaurav
marked this pull request as ready for review
November 7, 2026 23:45
gaurav
added a commit
to TranslatorSRI/babel-validation
that referenced
this pull request
Nov 9, 2026
The strict xfails pass raises=KnownElasticsearchDiscrepancy so that a new breakage can't read as the old bug, but each check raised that class for more than its bug: - /get_semantic_types raised it for anything that wasn't a Biolink class CURIE, so a non-string or 'FOO:bar' would have xfailed. Only an all-lower-case 'biolink:' class (biothings/NodeNormalizationAPI#59) is the known bug now; anything else fails with a plain assert. - POST /get_setid raised it for any JSON object, including an error body. Now it raises only for an object keyed '0'..'n-1' with a set ID under each key. - /get_allowed_conflations raised it for any 500 on any target. Now it raises only on ES before 1.2.0, only for Tornado's bare error page, and only if get_normalized_nodes, which also needs ES, still works. The bug returns the same page an ES outage would, so the body alone can't tell them apart. The version gate now uses the shared nodenorm_version(), and the OpenAPI document comes from the cached get_openapi(). CLAUDE.md records the rule: match the bug's signature, not just the check it fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gaurav
added a commit
to TranslatorSRI/babel-validation
that referenced
this pull request
Nov 9, 2026
…types fix The module docstring still said #58 fixed most of the lower-casing in NodeNorm ES 1.2.0, but test runs 1.2.0 and still lower-cases every class; #59 is the fix, as the xfail reason already said. Read as written, the docstring would lead someone to add before=(1, 2, 0) to that xfail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gaurav
added a commit
to TranslatorSRI/babel-validation
that referenced
this pull request
Nov 9, 2026
…document (#153) NodeNorm Redis and NodeNorm ES are meant to be interchangeable for the endpoints they share, but three NodeNorm ES endpoints answered differently from Redis and from ES's own OpenAPI document: `GET /get_allowed_conflations` returned HTTP 500, `POST /get_setid` returns an object keyed `"0"`, `"1"`, … instead of a list, and `/get_semantic_types` lower-cases Biolink classes. This PR adds `tests/nodenorm/test_nodenorm_endpoints.py`, which runs against every target and would have caught all three. It also checks that each deployment documents the endpoints both backends share, and that every path its OpenAPI document lists is actually served. biothings/NodeNormalizationAPI#58, released as NodeNorm ES 1.2.0, fixed the conflations 500; `ci` and `test` run 1.2.0, prod still runs 1.0.0. The setid shape and the semantic-types casing are fixed by biothings/NodeNormalizationAPI#48 and #59, both still open. What is still wrong is a strict xfail on the ES targets, so an unexpected pass reports when a marker can come out. ## Calls worth overruling - **Each xfail passes `raises=KnownElasticsearchDiscrepancy`, and that class is raised only on the bug's own signature.** That means only lower-cased `biolink:` classes, only an object keyed `"0"`..`"n-1"` with a set ID under each key, and only Tornado's bare 500 page while `get_normalized_nodes` still works. Anything else the same check catches fails with a plain assert. A bare strict xfail, or a class raised for "not a list" or "any 500", would also absorb an outage or an unrelated regression. [`CLAUDE.md`](https://github.com/TranslatorSRI/babel-validation/blob/46e69ea/CLAUDE.md#testing-patterns) records the pattern, and why an imperative `pytest.xfail()` doesn't work with the dashboard. - **The conflations xfail is keyed on the version `/status` reports**, not just the backend: ES targets older than 1.2.0 expect the 500, later ones must pass. `nodenorm_version()` in `service_checks.py` reads `1.2.0`, `v1.2.1`, `1.3`, `1.3.0-rc1` and PEP 440's `1.3.0.dev0` or `1.2.0.post1`; a suffix is ignored, so a pre-release counts as its release. A target whose `/status` has no release number at all fails rather than guessing. - **The path sweep skips `/get_allowed_conflations` where its 500 is expected**, instead of being a second xfail for the same bug. `test_get_allowed_conflations` tracks it, and the sweep still checks every other path on prod. - **The shared-endpoint list is the five both backends serve**, not everything Redis documents. NodeNorm ES deliberately leaves out `/query`, `/asyncquery` and `/get_curie_prefixes` (biothings/NodeNormalizationAPI#36). - **A path counts as served unless a bare GET returns 404 or 5xx.** A POST-only or parameter-requiring path legitimately answers 405, 400 or 422. A timeout or refused connection is recorded against its path rather than raised, so one hung path doesn't hide the rest. - **The OpenAPI document is treated as untrusted.** Its path keys must be plain `/segment` paths, and a document may list at most 20, before any URL is built from them, so a key such as `//host/x` cannot send a request elsewhere. The sweep requests five paths at a time with a (5s, 10s) connect/read timeout, so even a hung deployment fails inside pytest-timeout's 300s with each path named. Every message goes through `truncated_repr()`. - **The setid test checks only the shape**, with a single POST. `test_setid_basic` already compares the set IDs from GET and POST. It now asserts the POST succeeded and returned a list, showing the status and body, rather than failing with a `TypeError`. It gets no xfail, because it also fails on Redis targets today (water now normalizes to `CHEBI:15377`), and an xfail would hide that. ## Also here - `service_checks.py` gains `get_openapi()`, which caches each target's OpenAPI document per process the way `get_status()` caches `/status`. Both `test_openapi_json` tests and the new tests share it. - The shared fetch checks for a JSON object inside the cache, so a brief 200 with `null` is fetched again by the next test rather than remembered. - Every request in `test_nodenorm_setid.py` now has a timeout. ## Verification On 46e69ea, against `prod`, `test`, `ci`, `ci-nameres-es`, `dev` and `exp`, every test in the new file passes or xfails as expected. The nine xfails are semantic types and setid on the four ES targets, plus conflations on prod. The unit suite passes. Outside the new file, the touched tests fail on exactly the targets they fail on `main`, for reasons in the deployments: missing or wrong `x-translator` blocks and a `maxEdges` schema error in `test_openapi_json`, non-JSON setid bodies on the ES targets, and water now normalizing to `CHEBI:15377` in `test_setid_basic`. Faked responses confirmed that an error object, a non-Biolink type, or a 500 during an outage or on 1.2.0 fails outright. ## After merging Nothing blocks merging. The markers come out as the strict XPASSes report: conflations once prod reaches 1.2.0, setid with biothings/NodeNormalizationAPI#48, and semantic types with biothings/NodeNormalizationAPI#59. Whether lower-cased Biolink classes would also be acceptable is a separate question, #158. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/get_semantic_typesreturns Biolink classes in lowercase (biolink:smallmolecule), while/get_normalized_nodesreturns them with their real case (biolink:SmallMolecule). On CI, which runs currentmain, it returns both forms side by side, so the list has duplicates that differ only in case. This PR makes the endpoint return each class once, as its Biolinkclass_uri.The data in Elasticsearch is fine, so no reindex or loader change is needed. The endpoint builds its list from a
termsaggregation ontype. Aggregation keys are the indexed keyword values, and the index lowercases those./get_normalized_nodesreads_source, which keeps the original case. #58 switched the ancestors to bmt'sclass_uribut kept adding the raw bucket key, which is where the lowercase duplicates come from.The call worth overruling
Each bucket key is looked up with
toolkit.get_element(), which matches case-insensitively, and reported as itsclass_uri. A key the Biolink model doesn't know is reported unchanged rather than dropped, so a type from a newer Babel than the configured model is still visible. The other option was to make the mapping case-preserving, for example with a non-normalizedtype.rawsubfield. That would need a reindex and a change in the loader, for no gain here. A comment in the handler explains why the keys can't be used as they are, and notes that bmt already caches these lookups withlru_cache, so the handler doesn't cache them itself.What's here
src/nodenorm/handlers/semantic_types.py: the bucket keys are mapped back toclass_uri.tests/test_openapi.py: the fake Elasticsearch now returns a lowercase bucket key (biolink:disease), which is what real indices return. Its capitalised key was why the existing regression test missed this. The test also rejects duplicates that differ only in case.Verification
With the fix reverted, the updated test fails with
['biolink:disease', 'biolink:Disease', …], the same mix CI serves. With the fix, it passes. On 5ea349a, the full suite passes apart from 4 tests that need a live Elasticsearch:test_set_identifiers.py::TestSetIdentifierHandlerGet::test_get_endpoint,TestSetIdentifierHandlerPost::test_post_endpoint, andtest_normalized_nodes.py::TestNodeNormalizationHandler::test_node_normalization_get/_post. All four fail the same way onmain.Before merging
/get_semantic_typeshas onlybiolink:[A-Z]…CURIEs and no duplicates that differ only in case.Prod (
nodenorm-es.transltr.io) gets the fix the next timemainis deployed there.🤖 Generated with Claude Code