Skip to content

perf: skip quadratic curies duplicate check for valid contexts - #85

Merged
kevinschaper merged 4 commits into
linkml:mainfrom
ASCS-eV:perf/linear-duplicate-check
Nov 6, 2026
Merged

kevinschaper merged 4 commits into
linkml:mainfrom
ASCS-eV:perf/linear-duplicate-check

Conversation

@jdsika

@jdsika jdsika commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Fixes #75. Also relevant to monarch-initiative/monarch-app#819 (slow CLI startup), since Monarch calls load_converter("merged.monarch").

Problem

Context.as_converter() relied on curies' default strict=True. That check (_get_duplicate_prefixes / _get_duplicate_uri_prefixes) compares every pair of records, so its cost grows quadratically. For each merged context (4,273 records) it takes 15–25 s, so every load_converter("merged") pays that on startup.

Change

  • perf: Add _has_duplicates(), a linear scan for prefixes or URI prefixes (canonical or synonym) shared by two records.
    • When there are none, the converter is built with strict=False.
    • When there are some, strict=True is kept, so curies raises its usual DuplicatePrefixes / DuplicateURIPrefixes with the same message as before. The public API is unchanged.
  • test: Run the two canonical-uniqueness integrity tests on merged, merged.monarch and merged.oak too (they pass). The alias tests still skip the merged contexts (Add tests for integrity of merged contexts #26). This is the follow-up offered in Contexts don't guarantee bi-directional mapping #11.
  • docs: docs/intro.md built converters with Converter.from_extended_prefix_map(context.as_extended_prefix_map()), which is the slow strict path. It now uses ctxt.as_converter(). This also fixes examples that assigned ctxt but read context, and one line repeated three times.

Results

before after
load_converter("merged") ~14–18 s ~0.3 s
converters for all 11 bundled contexts ~85 s ~1 s
full test suite (local, w3id test excluded) ~27 s ~11 s, including the new test that builds all 11 converters

Verification

An independent adversarial review checked equivalence with curies' strict check:

  • Code across versions: compared the relevant code in all 90 curies releases in the supported range (0.5.3–0.15.4). strict only ever controls these two exact-match checks, and from_extended_prefix_map passes strict= through in every version.
  • Fuzzing: 20k random record sets plus hand-built edge cases, run on curies 0.15.4, 0.7.9 and 0.5.3 (Python 3.14 and 3.8). Zero disagreements with curies' check.
  • Real data: for every bundled context, the converter built the new way is identical to the strict one: records, prefix_map, reverse_prefix_map, pattern_map, and 20k expand/compress probes on merged.
  • Mutation testing: the new duplicate tests fail if the fallback, the URI-synonym check or the canonical-prefix check is removed.
  • Lint: flake8 with the tox -e flake8 plugin set passes on Python 3.11.

Notes

  • If a future curies release adds more checks under strict=True, they would be skipped for contexts that pass the linear scan. A linear duplicate check upstream in curies would be the long-term fix, and we're happy to propose one there.
  • Three outputs in docs/intro.md were already stale on main and are left unchanged here:
    • FlyBase:FBgn123 no longer expands in the first example.
    • GEOGEO:1 now expands in the last two examples.
      These are data changes, not caused by this PR, since the converters are identical.

Context.as_converter() let curies run its strict duplicate check, which
compares every pair of records. For each merged context (4,273
records) that takes 15-25 seconds, so load_converter("merged") is slow
for every downstream user (see linkml#75 and monarch-app#819).

Scan the extended prefix map for shared prefixes or URI prefixes in
linear time, and build the converter with strict=False when there are
none. When duplicates do exist, keep strict=True so curies still raises
DuplicatePrefixes or DuplicateURIPrefixes with its usual message.
load_converter("merged") now takes about 0.3 seconds, and converters
for all bundled contexts build in about one second in total.

Add tests that every bundled context builds a converter, and that each
kind of shared prefix or URI prefix (canonical or synonym) is still
rejected.

Fixes linkml#75

Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
The integrity tests skipped merged, merged.monarch and merged.oak
entirely. Their canonical expansions are bijective, so run the two
canonical-uniqueness tests on every bundled context. The alias tests
still skip the merged contexts, because those still contain aliases
without a matching canonical record (linkml#26).

Follow-up to the discussion in linkml#11.

Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
The examples built converters with
Converter.from_extended_prefix_map(context.as_extended_prefix_map()),
which runs the slow strict duplicate check. Use as_converter() instead.
This also fixes examples that assigned ctxt but read context, one line
repeated three times, and the missing load_context import.

Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
@jdsika

jdsika commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

The red check is not caused by this change.

  • The only failure is tests/test_etl/test_w3id.py::TestW3id::test_w3id (KeyError: 'kgcl'). It also fails on current main since perma-id/w3id.org moved its namespaces under ids/ on 2026-09-04, and fix: read w3id namespaces from ids/ after upstream move #84 fixes it.
  • The other 31 tests passed in the 3.12 job.
  • The 3.8 and 3.11 jobs were cancelled by fail-fast. With test_w3id deselected, they pass locally: 3.8 and 3.11 both give 31 passed, 1 skipped, with the locked dependency versions (curies 0.7.9, bioregistry 0.11.10, pydantic 2.7.0).

I'll rebase this branch once #84 is merged so CI goes fully green.

@kevinschaper
kevinschaper merged commit 9c033de into linkml:main Nov 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible unnecessary error checking loading from included contexts

3 participants