[snyaggarwal]

OpenConceptLab/ocl_online#655

What this does

This adds isolated source versions, for publishers like LOINC who release each version as a complete, standalone artifact. Today every new version carries HEAD forward. An isolated version instead makes HEAD exactly what the release contains, and then snapshots that HEAD.

You start one with isolated_version=true on POST /importers/bulk-import/. The import must contain the content, then exactly one Source Version line. It works with the JSON-lines and zip importers; npm imports are refused.

• Checked first: before anything imports, the version line is checked: exactly one, after the content, a valid mode, a version that doesn't exist yet and edit access to the source. A failed check rejects the whole import.
• Order: concepts and mappings import first, and the Source Version line runs last. The version is created only if no content failed. If some content fails, what did import stays on HEAD, but nothing is dropped.
• One commit step for every path. IsolatedVersionCommit does the last step for the inline, parallel and zip/tar runners. It replaces HEAD's hierarchy, drops what the import didn't contain and creates the version in one transaction. If the commit is rejected (a validation error, or the hierarchy root would be dropped), the hierarchy and HEAD membership stay as they were. After the commit, a retried task on the indexing queue (index_dropped_source_children) reindexes the dropped codes and recounts HEAD. Its Task record shows a final failure.
• The mode is fixed before anything imports. isolated_version decides the mode. A Source Version line may repeat it in versioning_mode, but it can't contradict it: isolated without the flag, or carry_forward with it, rejects the whole import up front. The zip importer reads the Source Version lines (only those) while it counts resources.
• Hierarchy: HEAD's parent links are rebuilt from the import's parent_concept_urls and replace the old links, not add to them. A parent change forces a new concept version. ImportHierarchy resolves the links once into ids, then adds the latest-version links and replaces HEAD's links 5,000 children at a time. The parallel runner and the make_hierarchy task use it too.
• Codes the release doesn't contain are dropped from HEAD. They aren't retired: the versioned object goes inactive and leaves HEAD's membership. Older versions keep their pinned rows. A code that comes back later is revived on the same versioned object. The hierarchy root can't be dropped.
• No inheritance: a new concept or mapping version takes extras, external_id, datatype, sort_weight and the retire fields from the import, not from its previous version.
• Metadata: the metadata fields the Source Version line sets overwrite HEAD's. Fields it leaves out keep HEAD's value. The version then copies HEAD as usual. The ticket's spec was updated to match.
• Recording the mode: Source.versioning_mode (carry_forward | isolated, default carry_forward) records how each version was made. It is read-only in the API. Migration 0047 only adds the column.
• Version views: children/ and parents/ show the requested version's own members.

Changelog compare on large diffs

A 2.81 → 2.82 compare ran out of memory, and once it fit in memory, its result (1.4 GB) was over Postgres's 1 GB jsonb limit.

• Changelog enrichment now loads concepts 1,000 at a time and loads only the mapping fields the summary uses.
• Mappings are loaded per batch too. Between batches the changelog keeps only mapping ids. Display names for mapping-only changes come from one query per batch, with names prefetched.
• Above CHANGELOG_ENRICHMENT_MAX_RESOURCES (default 50,000 new, removed or changed resources), a changelog skips names, descriptions and prev_*, and sets meta.enrichment_skipped.

Also fixed

• Zip importer: numbers came through as Decimal (ijson), so any concept with a number in extras failed to save.

Behaviour changes to be aware of

• HEAD is no longer append-only for isolated sources. A code can leave HEAD without being retired, so GET on HEAD returns 404 for it while older versions still have it.
• Other sources' mappings to a dropped concept keep their FK, but the target is inactive.
• Collection references to HEAD or the latest version re-resolve without dropped codes. Pinned references are unaffected.
• HEAD metadata edits made between releases are overwritten by the next isolated version.
• Deleting an isolated version doesn't restore HEAD.

Testing

• New tests in core/importers/tests.py and core/integration_tests/tests_sources.py:
  • isolated import v1 → v2 → v3: drop, revive, no inheritance, metadata overwrite;
  • a rejected version leaving HEAD, the hierarchy and search untouched;
  • an existing version or missing edit access rejected before any content imports;
  • a failed search update after the commit not undoing the version, and the repair being queued as a retried task;
  • the zip look-ahead reading only Source Version lines, of any JSON type;
  • mode conflicts rejected before anything imports;
  • parent replacement;
  • version-scoped children and parents;
  • hierarchy root guard;
  • zip and inline runners;
  • changelog batching and the enrichment skip.
• pylint: pylint -j0 core/ exits 0.
• Focused tests: core.importers core.sources core.integration_tests.tests_sources core.common core.concepts core.mappings ran 1,224 tests. BulkImportLimitsTest.test_grandfathered_npm_url_is_downloaded_and_handed_over_as_upload fails, as it does on master. A few tests that share the local ES fail only in parallel runs and pass on their own.
• Local LOINC import: 2.82 was imported as an isolated version on top of 2.81, and the result was checked row by row against the release files.
  • All 252,212 concepts and 192,208 mappings match.
  • HEAD equals 2.82, its 252,969 parent links match the release, and 2.81 is unchanged.
  • ES is fully reindexed with vectors.
  • Details are on the ticket.

Not in this PR

• API version create doesn't take versioning_mode; isolated versions come from imports only, as decided on the ticket.
• Imported versions don't take HEAD's match_algorithms.
• New mappings without an id are slow: each runs a MAX(mnemonic) scan, 2–6 s each.
• The import status API returns 500 after a chord member dies.

The frontend part, showing the versioning mode in version details, is in OpenConceptLab/oclweb3 on the branch of the same name.

🤖 Generated with Claude Code

[snyaggarwal]

Findings

• [P1] Failure atomicity: hierarchy and HEAD membership can change before version persistence completes, so a failed version write can leave partially applied data. The parallel import path also reconciles hierarchy after rejecting a version line. Specific locations and remediation are on the linked tracker issue: OpenConceptLab/ocl_online#655.
• [P2] Mode selection: the workflow path and the persisted mode can diverge when the request flag and Source Version line disagree. Details and expected behavior are on OpenConceptLab/ocl_online#655.
• [P3] OOP / duplicated logic: isolated-version orchestration is implemented separately in the inline, parallel JSON-lines, and archive subtask paths. Consolidating validation, hierarchy reconciliation, and sweep coordination into a shared importer service would prevent their current behavior from drifting; see OpenConceptLab/ocl_online#655 for the concrete differences.

I found no additional P4/P5 findings or nits.

[snyaggarwal]

Addressed in 308432d:

• [P1] Failure atomicity. The new IsolatedVersionCommit replaces HEAD's hierarchy, drops what the import didn't contain and creates the version in one transaction, so a rejected version rolls all of it back.
  • It checks content failures, an existing version, edit access and the mode before it opens the transaction.
  • Search and count updates for dropped codes now run on commit.
  • The parallel runner no longer reconciles the hierarchy in isolated mode: the commit owns it, so a rejected version leaves it untouched.
  • Tests cover a failed version save, the hierarchy-root guard, an existing version and missing edit access.
• [P2] Mode selection. isolated_version decides the mode, and Source.resolve_versioning_mode is the one place that checks a line's versioning_mode against it. A conflicting line rejects the whole import before anything runs: up front in the inline and parallel runners, and while counting resources in the zip importer. SourceVersionImporter applies the same rule to any line that reaches it, and the version records the resolved mode.
• [P3] Duplicated orchestration. All three runners now hand their version line, hierarchy map, seen ids and content-failure count to IsolatedVersionCommit.

pylint is clean. The isolated-version tests pass with Redis unreachable, and the focused suite has only the BulkImportLimitsTest failure that is also on master.

[snyaggarwal]

Second review

The prior hierarchy/sweep/version transaction finding and duplicated orchestration finding are addressed. Remaining/new findings:

• [P1] Post-commit ordering can leave a committed version unseeded. The fallible indexing callback is registered before the version-seeding callback; if indexing raises, later callbacks do not run and the import can report failure after the DB commit.
• [P1] A rejected final version still leaves earlier content imports on HEAD. The shared transaction starts after concept/mapping tasks have committed, and the existing-version test only reimports unchanged content.
• [P2] Mode resolution still differs from the linked requirement. The request flag is authoritative instead of the Source Version line/HEAD default, and API version creation still ignores the requested mode.
• [P2] Omitted isolated metadata is inherited. The implementation and test retain fields such as website, while the linked requirement says missing payload metadata is cleared.
• [P2] Archive look-ahead scans every string versioning_mode field and skips non-string values. This can reject a valid archive because of a Source line or defer an invalid Source Version value until after content imports.

Full reproductions and suggested fixes are on OpenConceptLab/ocl_online#655. No additional P3-P5 findings or nits.

[snyaggarwal]

Second review addressed in a5a1fd7. Details are on OpenConceptLab/ocl_online#655.

• [P1] Post-commit ordering: fixed. The search and count update for dropped codes is a robust on_commit callback, so a failure there no longer stops the version's seeding. A regression test covers it.
• [P1] A rejected version left content on HEAD: an isolated import now preflights the version line before any content imports. That checks the id, that the version doesn't exist yet, the mode and edit access. It runs on all three paths. Content that imports before a later content failure or a commit failure still stays on HEAD, and the ticket and the PR text now say so.
• [P2] Mode resolution and [P2] omitted metadata: no code change. Both are human decisions, now written into the ticket's spec.
• [P2] Archive look-ahead: fixed. Only Source Version lines are read, and a non-string versioning_mode is rejected before any content imports.

[snyaggarwal]

[P1] Persist a retry instead of only logging a failed search repair. robust=True fixes the earlier callback-order bug, but it makes a failed reindex/count update best-effort: the database has already committed is_active=False and removed the HEAD membership rows while Elasticsearch and the stored counts can still describe the old HEAD. The later version indexing is partial (membership/latest-release flags) and does not reliably full-index every dropped HEAD document, so a transient ES failure can leave user-visible search/counts stale until a manual rebuild. Register a durable, retryable task after commit carrying the source and dropped IDs (and expose terminal failure), rather than doing the only repair in a log-only callback. The existing ES down test should also assert that the repair is queued/retried, not just that the DB commit survives.

[snyaggarwal]

[P2] Avoid resolving and materializing the full hierarchy three times. This method first resolves every child/parent URI and builds accessible_map plus the inverted copy. make_hierarchy() then resolves the same URI graph again and materializes pairs/existing; replace_versioned_object_hierarchy() resolves it a third time and builds desired/existing. On a release-sized hierarchy this creates several complete graph copies and repeats the DB lookups inside the final commit transaction. It also splits the same hierarchy domain operation between a module-level task and model methods, which is the duplication/drift risk called out in the review focus. Resolve/access-check once into IDs and let one class/service operation apply both latest-version links and HEAD replacement in chunks. A release-scale query/memory regression test would protect this path.

[snyaggarwal]

[P2] The mapping side of the changelog is still unbounded and can reintroduce the OOM/N+1 behavior. _build_mappings_cache() materializes every changed mapping plus its related concepts before the concept batching starts, and mapping_diff_index retains references to all of those model objects in per-concept lists. In the changed_mappings_only branch, mapping.from_concept.display_name also runs without prefetched names or parent data, so it can issue several queries for each unchanged from-concept. The concept cache is bounded, but the mapping object graph and query count remain proportional to the entire diff. Batch mapping IDs too, prefetch the parent/names needed for display names per batch, and emit summaries/index entries incrementally instead of retaining all mapping models.

[snyaggarwal]

[P2] Do not aggregate every per-resource Celery result in the version worker. get_previous_results() concatenates every content task's full result list, and this code keeps that list alive while also building the concept/mapping ID sets. For a 445k-row import this deserializes and retains hundreds of thousands of scalar statuses plus all seen arrays in one worker, then duplicates the identifiers into sets precisely at the most memory-intensive commit step. Only a failure count and the two seen sets are needed: reduce one task result at a time and discard it, or preferably make each content subtask return an aggregate {failure_count, seen} rather than one status per resource.

[snyaggarwal]

[P3] Reduce child-task results once, not three times. get_seen_ids('concepts'), get_seen_ids('mappings'), and get_content_failures_count() each call get_task_results(), so a large parallel import walks every task result three times and creates three temporary result lists (and may repeat result-backend access). Capture the task results once and derive both ID sets and the failure count in a single pass. A shared result-aggregation object/helper could serve this path and the archive path above, removing another duplicated piece of isolated-version orchestration.

[snyaggarwal]

Third-round review

The earlier rejection/transaction and callback-order findings are addressed in the current head. This pass found one remaining reliability blocker plus four performance/architecture issues in the new large-import paths. Each finding is posted inline with reproduction impact and a concrete remediation.

Summary: 1× P1, 3× P2, 1× P3. No additional P4/P5 findings or nits.

[snyaggarwal]

Third review addressed in da8dc58.

• [P1] Search repair was log-only: fixed. After the commit, drop_unseen_children queues index_dropped_source_children on the indexing queue with the source and the dropped ids. A Task record tracks it, so a final failure shows there. It retries with backoff (5 retries, up to 10 minutes apart), reindexes the dropped concepts and mappings, and recounts HEAD. Queuing is still a robust on_commit callback, so it can't stop the version's seeding. New tests check that the task is queued with the right ids and Task, and test its retry settings and what it runs.
• [P2] Hierarchy resolved three times: fixed. The new ImportHierarchy resolves every child and parent URI once and checks access once. It keeps only versioned-object ids, then works 5,000 children at a time. In each chunk it adds the latest-version links and either adds or replaces HEAD's links. reconcile_import_hierarchy, the parallel runner and the make_hierarchy task all use it, so the separate graph-building in make_hierarchy and replace_versioned_object_hierarchy is gone. A test checks that the URIs are resolved only once. The tests that used to mock make_hierarchy now check the stored links.
• [P2] Changelog mappings unbounded: fixed. The mapping index holds (mnemonic, id) pairs only. Mappings, and their v1 rows when enriching, are fetched per concept batch and per mapping batch, then dropped. Display names for changed_mappings_only come from one query per batch, with names prefetched and each source loaded once. The existing batch-of-one changelog test now runs the mapping side in batches of one too.
• [P2] Version worker held every result: fixed. The archive path reads earlier subtask results one task at a time (iter_previous_results). Each is folded into the seen ids and a failure count, then dropped. I kept one status per resource in the subtask results, because the import summary counts them.
• [P3] Results read three times: fixed. IsolatedVersionCommit.add_runner_result and add_subtask_results fold each result in a single pass. The parallel runner and the archive path both use them, in place of the three separate walks.

pylint -j0 core/ exits 0. The focused suite ran 1,224 tests. The only failure is the BulkImportLimitsTest npm test, which also fails on master. A few tests that use the local ES fail in parallel runs and pass on their own. ConceptPropertyFilterViewTest fails locally both with and without these changes.