Skip to content

OpenConceptLab/ocl_online#655 | Isolated source versions - #985

Open
snyaggarwal wants to merge 11 commits into
masterfrom
ocl_online#655
Open

snyaggarwal wants to merge 11 commits into
masterfrom
ocl_online#655

Conversation

@snyaggarwal

@snyaggarwal snyaggarwal commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

OpenConceptLab/ocl_online#655, OpenConceptLab/ocl_online#672

What this does

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

You start one with seed_from_import=true on POST /importers/bulk-import/ or /importers/bulk-import-parallel-inline/. 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, retried tasks on the indexing queue (index_dropped_source_children, one per 5,000 ids) reindex the dropped codes and recount HEAD. Each Task saves its arguments, so it can be rerun, and shows a final failure. A parallel part that failed or has no result blocks the version.
  • The switch alone decides. seed_from_import=true makes the import seed the version; the Source Version line has no mode key. The zip importer reads the Source Version lines (only those) while it counts resources, so the line is checked before anything imports.
  • 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 seed: each version has a read-only seeded_from. A version made from HEAD records {"type": "repo_version", "url": ".../HEAD/"}, and one seeded from an import records {"type": "import", "task": "<import task id>"}. HEAD records the version an import last re-seeded it from, and is otherwise null. Migration 0047 added a versioning_mode column; 0048 replaces it with seeded_from and backfills it: existing versions as seeded from HEAD, or from an import where 0047 recorded one.
  • 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 sources that use import-seeded versions. 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 import-seeded version.
  • Deleting an import-seeded version doesn't restore HEAD.

Testing

  • New tests in core/importers/tests.py and core/integration_tests/tests_sources.py:
    • import-seeded 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 in chunks of retried, rerunnable tasks;
    • a failed or missing parallel part blocking the version;
    • 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 import-seeded 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 always seeds from HEAD; versions seeded from an import come from imports only, as decided on the ticket. seeded_from can't be set in a request.
  • Counts of what a version adds or no longer includes (OpenConceptLab/ocl_online#674, Bump boto3 from 1.24.28 to 1.34.94 #675).
  • 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 snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…ommit for every import path; versioning mode resolved before import
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

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 snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

… content imports; robust post-commit indexing; archive look-ahead reads only Source Version lines

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

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.

Comment thread core/sources/models.py Resolved
Comment thread core/concepts/models.py Resolved
Comment thread core/common/checksums.py Resolved
Comment thread core/importers/importer.py Resolved
Comment thread core/importers/models.py Resolved

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…s for dropped codes; hierarchy resolved once; import results and changelog mappings handled in batches
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

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.

…epair task test to the drop so it needs no Redis
Comment thread core/importers/models.py Resolved
Comment thread core/sources/models.py Resolved
Comment thread core/sources/models.py Resolved

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Request changes

Reviewed current head d9d8091e. The five findings from the prior round are materially addressed, including the hierarchy consolidation, batched changelog/import aggregation, and retryable dropped-code indexing.

Remaining/new findings:

  1. P1 — failed or missing parallel child results are ignored. The version can still be committed and unseen HEAD content swept after a child task failure.
  2. P2 — dropped-code repair arguments are not persisted. The stranded-task/manual rerun path requeues the repair without its required arguments.
  3. P3 — the repair uses one unbounded Celery payload. A large replacement can send hundreds of thousands of IDs in one broker message.

I left detailed inline comments with concrete remediations and regression cases. No additional P4/P5 findings or nits.

Verification: 55 focused tests passed (IsolatedVersionImportTest, IsolatedVersionZipImportTest, BulkImportParallelRunnerTest, and SourceVersionsChangelogOutputViewTest). The P1 failed-child scenario and replayability of the repair task are not covered by those tests.

…cks the isolated version; dropped-code repair queued in chunks with saved arguments
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

Fourth review addressed in 76f0a23.

  • [P1] Failed or missing parallel parts were ignored: fixed. Before committing, the parallel runner checks every part. A part that didn't succeed, or whose result is missing, expired or not a result dict, counts as a content failure. The version is then refused, and nothing is dropped or swept. The archive path already counted these. A new test covers a part that raised and a part whose result is missing: no version is created and HEAD keeps its codes.
  • [P2] Repair arguments weren't saved: fixed. Each repair Task now stores its arguments and the registered task name when it's created, and is queued with persist_args=True. A rerun from the stranded-indexing sweep, or by hand, gets the original arguments. If queuing fails, the Task is marked failed with the error, so it can still be rerun. New tests check the saved arguments, and that a Task that couldn't be queued reruns with them.
  • [P3] One unbounded payload: fixed. The repair is queued as one Task per 5,000 concept ids and 5,000 mapping ids. The first chunk also recounts HEAD; counts come from the database, so the order doesn't matter. A test checks the chunking.

pylint -j0 core/ exits 0. The isolated-version, zip and parallel runner tests pass with Redis unreachable. In the focused suite, the only failures are the BulkImportLimitsTest npm test, which also fails on master, and LoadFixturesCommandTest, which fails only in parallel runs.

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve

Reviewed current head 76f0a239. The three findings from the previous round are addressed:

  • failed, lost, or missing parallel parts now block the isolated-version commit and preserve HEAD;
  • dropped-code repair tasks persist their arguments and leave a replayable failed task when enqueueing fails;
  • repair work is split into bounded 5,000-ID chunks, with HEAD recounted once.

I found no new P1-P5 findings or nits in this round.

Verification:

  • 58 focused isolated-import, archive, parallel-runner, hierarchy, and changelog tests passed;
  • targeted pylint passed at 10.00/10;
  • GitHub Tests, Pylint, and Snyk checks are green.

@snyaggarwal

Copy link
Copy Markdown
Contributor Author

ab81129f renames this PR's new field and import switch before they become public API (OpenConceptLab/ocl_online#672). What the import does hasn't changed.

  • seeded_from replaces versioning_mode. Each version records what it was seeded from, once, when it's created:

    • a version made from HEAD: {"type": "repo_version", "url": "/orgs/<org>/sources/<source>/HEAD/"};
    • a version seeded from an import: {"type": "import", "task": "<import task id>"}. The zip/tar path now passes the import's task id to its version subtask;
    • HEAD: the version an import last re-seeded it from, otherwise null.

    It's read-only: a seeded_from sent to version create is ignored.

  • seed_from_import=true replaces isolated_version=true on both import endpoints. The Source Version line no longer takes versioning_mode, and an old isolated_version=true is ignored.

  • Migration 0048_source_seeded_from adds seeded_from, backfills it, then removes versioning_mode. Existing versions are recorded as seeded from HEAD, or from an import where 0047 recorded one. A dry run on a local copy of LOINC, rolled back afterwards, gave 2.81 seeded from HEAD, 2.82 seeded from an import, and HEAD re-seeded from 2.82.

The names are the proposed ones and may still change after the decisions on OpenConceptLab/ocl_online#671. oclweb3#97 shows the seed as "Seeded From" (e144c20, with a lint fix in 01a2fc4).

Testing: pylint -j0 core/ exits 0 and makemigrations --check finds no missing migrations. The tests haven't run locally because the local search service is down, so this CI run is their first on this change. In oclweb3, npm test passes 125 tests and eslint src is clean.

@paynejd paynejd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review, pass 2, with a Codex check (commit b14c5f0)

Verdict: fix-first. The rename in ab81129f and b14c5f0f does what D1 to D4 recommend, and I found nothing wrong with it on the main paths. The two P1s below come from the first review, at 76f0a239, which we held back until the version model was settled, so this is the first time they're posted. Your design page lists "Review P1s on #985" as approved; these two are still open. Neither commit touches the code they point at, and both still reproduce at b14c5f0f. That page and this PR's description both say version-scoped children/ and parents/ work, but through the API they don't (item 1).

Fix before merge

  1. [P1] core/concepts/views.py:626 (and the parents view): is_source_version() reads parent_resource, which these views never set, so it's always False. /sources/S/<version>/concepts/X/children/ is answered from HEAD's links, and H4 isn't delivered. Probe at b14c5f0f: v1 to v3 each with P, A and B (A and B children of P), then v4 with P and A. children/ of P returns A and B on all four versions, though v4 holds only A. The model method alone returns only A. The PR's API test passes because HEAD gives the same answer in its case. @snyaggarwal: resolve the repo version from the URL before branching, and add a test where a version differs from HEAD. The HEAD fallback was already there before this PR.
  2. [P1] core/concepts/models.py:1419: after an import drops a child, the parent's HEAD children/ still lists it, and opening it gives a 404. The sweep removes the dropped concept's own links but keeps the links its version rows hold (H3), and the HEAD lookup returns them without an active or membership filter. Same probe: B is 404 on HEAD, yet HEAD P/children/ returns A and B. Filter HEAD hierarchy lookups to active HEAD members in get_hierarchy_related_queryset. With oclapi2#990 merged, get_viewable_hierarchy_concept_urls wraps that lookup, so it gets the fix too.

Not blocking

  • [P2] core/importers/models.py:1160: mappings nested in a Concept line are created but not recorded as seen, so the sweep drops them while the import reports success. Probe at b14c5f0f: the version and HEAD end with 0 mappings. LOINC doesn't nest mappings. Track their ids, or reject nested mappings when seed_from_import=true. (Codex rated this P1 in pass 1.)
  • [P2] core/importers/models.py:458: the dropped counts are returned and thrown away, so the import result never says how many concepts and mappings left HEAD. Probe at b14c5f0f: a file with concepts but no mapping lines made a version with 0 mappings and no error. This is behaviour 6 on the Decisions tab.
  • [P2] New in this pass: seed_from_import=true on the deprecated /importers/bulk-import/upload/ and /importers/bulk-import/file-url/ is accepted, then dropped. queue_bulk_import passes the switch only to inline tasks, and bulk_import doesn't take it. The file runs as a plain import and returns 202: its Source Version line makes a version from HEAD, and nothing is dropped. Neither endpoint lists the switch in Swagger, but import_response validates it there. Reject it with a 400 on the non-inline path, as npm imports are; there are three one-click suggestions for this inline, plus a test that expects the 400. This was already the case before the rename.
  • [P2] core/importers/models.py:369: outside tests, a queued task copies the version's membership from HEAD, so a change to HEAD before it runs changes the release. Tests seed at once in TEST_MODE. The pattern predates this PR, but the sweep makes it worse. Snapshot membership in the commit transaction, or refuse a new seeding import while the previous version is still processing.
  • [P2] core/importers/models.py:456: two seeding imports into one source can run at once, and the first commit's sweep drops what only the second sent. A per-source reservation from preflight to commit would stop it.
  • [P2] core/mappings/models.py:425: a seeding import's mapping update keeps relation-derived text, such as to_concept_name, when the release line leaves it out. LOINC lines carry no names, so LOINC isn't affected.
  • [P3] core/concepts/models.py:1272: seeding imports still queue the per-concept hierarchy task, and a late one could write old parents over a newer commit. You noted on the ticket that you'd skip them.
  • [P3] core/concepts/models.py:1461: the new hierarchy resolver skips a parent URL that names a concept version, which the old make_hierarchy accepted. Not reproduced on the inline runner, and the parallel runner isn't covered by tests. Codex now rates it P2.
  • [P3] New: core/common/tasks.py:285: the deprecated /importers/bulk-import-inline/ honours the switch, but bulk_import_inline isn't bound, so its version records {"type": "import", "task": null}. A TODO in queue_bulk_import says that task can't be reached, but BulkImportInlineView reaches it with threads=None. Bind the task and pass self.request.id (one-click suggestion inline). Codex rates this P1; I don't, because only the audit id is missing.
  • [P3] New: core/sources/models.py:243: get_version_url rebuilds a version's URL from its label. Use the new version's own uri, which is the same string today, so the seed and the version's own URL can't drift apart.
  • [P3] New: 0047 adds versioning_mode and 0048 removes it. Neither has shipped, so folding them into a single migration that adds seeded_from drops the round trip, and the reverse-migration gap Codex found in 0048 with it. Do it unless a shared database (staging?) already ran 0047. oclapi2#955 also adds a sources 0047; whichever merges second renumbers.

The rename (ocl_online#672). Each case below was probed at b14c5f0f:

  • An import-seeded version records {"type": "import", …}, and HEAD records that version, as D4 recommends.
  • POST …/versions/ records HEAD and ignores a seeded_from in the payload.
  • PUT on a version can't change it.
  • HEAD keeps its seed through later versions made from HEAD.
  • The HEAD and version details return seeded_from and no versioning_mode.
  • A Source or Source Version import line can't set it, since neither importer's allowed_fields includes it.
  • The inline, parallel and zip/tar runners pass the import task id. The deprecated inline endpoint doesn't (above).
  • On production, every source version has a HEAD, so the backfill never writes a seed with no URL.

An old isolated_version=true is now ignored. That's fine, since the name never shipped, but any runbook for the LOINC 2.82 staging run needs the new name.

Decisions

The six behaviour questions are D8 on the Decisions tab of the Repo Versions Model doc, for today's meeting. Please note the outcomes here once they're settled.

How it was checked

  • Read your design page, the two new commits and the earlier findings against b14c5f0f. The branch merges cleanly with master at 015eaaa2 (#987, #990, #991 and #992 in). CI is green.
  • Re-ran the first review's probes at b14c5f0f on a throwaway stack, plus new probes for seeded_from: HEAD, import-seeded versions, POST with a spoofed seed, PUT, and the HEAD detail.
  • Codex (codex-cli 0.169.0, xhigh), pass 2: all nine earlier findings unresolved, plus three new ones (P1, P2, P2 by its rating; P3 each here, explained above). I found the deprecated-endpoint switch on my own.
findings.json
[
 {
  "id": "C1",
  "severity": "P1",
  "file": "core/concepts/views.py",
  "line": 626,
  "claim": "Version-scoped children/ and parents/ branches never run: is_source_version() reads parent_resource, which is not the source version in get_queryset, so it is always False.",
  "scenario": "Version URL children/ is answered from HEAD's links; in a 4-import run /v4/concepts/P/children/ returned A and B although B is not in v4.",
  "action": "Resolve the repo version from the URL kwargs before branching and add a test where a version differs from HEAD.",
  "confidence": "high",
  "source": "both",
  "codex_check": "partly",
  "introduced_by_pr": false,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved",
  "probe_b14c5f0f": "reproduced"
 },
 {
  "id": "C2",
  "severity": "P1",
  "file": "core/concepts/models.py",
  "line": 1419,
  "claim": "HEAD children/ of a parent lists a dropped concept: version-row links are kept and HEAD lookup has no active/membership filter.",
  "scenario": "v1-v3 with P, A, B; v4 with P, A: HEAD P/children/ returns A and B while B is 404 on HEAD.",
  "action": "Return only active HEAD members from HEAD hierarchy lookups.",
  "confidence": "high",
  "source": "both",
  "codex_check": "confirmed",
  "introduced_by_pr": true,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved",
  "probe_b14c5f0f": "reproduced"
 },
 {
  "id": "C3",
  "severity": "P2",
  "file": "core/importers/models.py",
  "line": 1160,
  "claim": "Mappings nested in a Concept line are not recorded as seen, so the sweep drops them.",
  "scenario": "Concept with a nested mapping plus a version line: version and HEAD end with 0 mappings.",
  "action": "Track nested mapping ids or reject nested mappings in isolated imports.",
  "confidence": "high",
  "source": "both",
  "codex_check": "partly",
  "introduced_by_pr": true,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved",
  "probe_b14c5f0f": "reproduced"
 },
 {
  "id": "C4",
  "severity": "P2",
  "file": "core/importers/models.py",
  "line": 458,
  "claim": "Dropped counts are discarded; the import result never says how much left HEAD.",
  "scenario": "Concepts-only isolated import leaves 0 mappings in the version and HEAD with no error.",
  "action": "Return the counts; optionally a dry run or a drop threshold.",
  "confidence": "high",
  "source": "claude",
  "codex_check": "partly",
  "introduced_by_pr": true,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved",
  "probe_b14c5f0f": "reproduced"
 },
 {
  "id": "C5",
  "severity": "P2",
  "file": "core/importers/models.py",
  "line": 369,
  "claim": "The version's membership is seeded from HEAD by a queued task, so later HEAD changes alter the release.",
  "scenario": "A second import or edit changes HEAD before the seed task runs.",
  "action": "Snapshot membership in the commit transaction or block a new isolated import while processing.",
  "confidence": "medium",
  "source": "codex",
  "codex_check": "partly",
  "introduced_by_pr": false,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved"
 },
 {
  "id": "C6",
  "severity": "P3",
  "file": "core/concepts/models.py",
  "line": 1272,
  "claim": "Isolated imports still queue per-concept hierarchy tasks that can run late.",
  "scenario": "A late task overwrites HEAD parents after a newer commit.",
  "action": "Skip those tasks in isolated imports.",
  "confidence": "medium",
  "source": "both",
  "codex_check": "partly",
  "introduced_by_pr": false,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved"
 },
 {
  "id": "C7",
  "severity": "P2",
  "file": "core/mappings/models.py",
  "line": 425,
  "claim": "Isolated mapping updates keep relation-derived text such as to_concept_name.",
  "scenario": "Release 2 omits to_concept_name; the new version keeps the old label.",
  "action": "Reset omitted optional fields before resolving relations.",
  "confidence": "medium",
  "source": "both",
  "codex_check": "partly",
  "introduced_by_pr": true,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved"
 },
 {
  "id": "C8",
  "severity": "P2",
  "file": "core/importers/models.py",
  "line": 456,
  "claim": "Two isolated imports into one source can run at once.",
  "scenario": "The first commit's sweep drops what only the second import sent.",
  "action": "Per-source reservation from preflight to commit.",
  "confidence": "medium",
  "source": "both",
  "codex_check": "partly",
  "introduced_by_pr": true,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved"
 },
 {
  "id": "C9",
  "severity": "P3",
  "file": "core/concepts/models.py",
  "line": 1461,
  "claim": "The hierarchy resolver skips parent URLs that name a concept version.",
  "scenario": "Parallel import with a version URL parent: the link is never made. Not reproduced.",
  "action": "Resolve versioned and versionless URLs to the versioned object.",
  "confidence": "low",
  "source": "codex",
  "codex_check": "not-run",
  "introduced_by_pr": true,
  "status_b14c5f0f": "unresolved",
  "codex_pass2": "unresolved",
  "codex_pass2_severity": "P2"
 },
 {
  "id": "C10",
  "severity": "P2",
  "file": "core/common/utils.py",
  "line": 466,
  "claim": "seed_from_import=true on the deprecated upload/ and file-url/ endpoints is validated, then never reaches the runner.",
  "scenario": "POST /importers/bulk-import/upload/?seed_from_import=true with a release file: 202, plain import, a version made from HEAD, nothing dropped.",
  "action": "Reject the switch with a 400 on endpoints that can't seed.",
  "confidence": "high",
  "source": "claude",
  "introduced_by_pr": true
 },
 {
  "id": "C11",
  "severity": "P3",
  "file": "core/common/tasks.py",
  "line": 285,
  "claim": "bulk_import_inline isn't bound, so a seeding import through the deprecated bulk-import-inline endpoint records task null.",
  "scenario": "POST /importers/bulk-import-inline/?seed_from_import=true: seeded_from is {type: import, task: null}.",
  "action": "Bind the task and pass self.request.id as self_task_id.",
  "confidence": "high",
  "source": "codex (F2.3, rated P1)",
  "introduced_by_pr": true
 },
 {
  "id": "C12",
  "severity": "P3",
  "file": "core/sources/models.py",
  "line": 243,
  "claim": "HEAD's seeded_from URL is rebuilt from the version label rather than taken from the version's uri.",
  "scenario": "A label needing URL encoding gives a seed URL that doesn't resolve (as the version's own URI already would).",
  "action": "Use the new version's uri.",
  "confidence": "medium",
  "source": "codex (F2.9, rated P2)",
  "introduced_by_pr": true
 },
 {
  "id": "C13",
  "severity": "P3",
  "file": "core/sources/migrations/0048_source_seeded_from.py",
  "line": 49,
  "claim": "0047 adds versioning_mode and 0048 removes it; reversing 0048 loses which versions were import-seeded.",
  "scenario": "Migrate a database with import-seeded versions back to 0047 and forward again: they become HEAD-seeded.",
  "action": "Fold 0047 and 0048 into one migration while neither has shipped.",
  "confidence": "high",
  "source": "codex (F2.10, rated P2) + claude",
  "introduced_by_pr": true
 }
]

Comment thread core/importers/constants.py Resolved
Comment thread core/importers/views.py Resolved
Comment thread core/importers/views.py Resolved
Comment thread core/importers/views.py Resolved
Comment thread core/common/tasks.py Resolved
…the URL's source version, HEAD hierarchy hides dropped concepts, seed_from_import is refused where it can't seed, and the deprecated inline import records its task
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

Pass 2 review: changes in 85a6d41.

Fixed

  • P1, version-scoped children/ and parents/. The views now take the source version from the URL (source and version kwargs, latest included) instead of parent_resource. HEAD and collection URLs keep the HEAD lookup.
  • P1, HEAD lists dropped children. get_hierarchy_related_queryset returns only active HEAD concepts, so get_viewable_hierarchy_concept_urls is fixed too.
    • New test test_hierarchy_endpoints_follow_each_version runs your probe: v1 {P, A, B} and v2 {P, A}. P/children/ returns A and B on v1, and only A on v2 and on HEAD. B returns 404 on HEAD, and v1/.../B/parents/ returns P.
  • P2, deprecated upload/ and file-url/. These now reject seed_from_import=true with a 400 (your suggestions, plus a test).
  • P3, bulk_import_inline. The task is bound and records its task id. The stale TODO in queue_bulk_import is gone.

Still open, not in this push: the nested-mapping sweep, the dropped counts (D8.6 / ocl_online#674, which is on today's meeting agenda), queued membership seeding, concurrent seeding imports, mapping relation text, late hierarchy tasks, the versioned parent URL resolver, get_version_url using uri, and folding 0047 into 0048. Before folding the migrations I need to know whether a shared database (staging) already ran 0047.

The local stack was down, so CI is running the tests for this push. Pylint -j0 core/ passes.

🤖 Addressed by Claude Code

This branch has not been deployed

No deployments
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.

2 participants