Repository navigation
OpenConceptLab/ocl_online#655 | Isolated source versions - #985
snyaggarwal wants to merge 11 commits into
Conversation
…eplaces HEAD content, metadata and hierarchy
…ersion tests so they need no Redis
snyaggarwal
left a comment
There was a problem hiding this comment.
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
|
Addressed in 308432d:
pylint is clean. The isolated-version tests pass with Redis unreachable, and the focused suite has only the |
snyaggarwal
left a comment
There was a problem hiding this comment.
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_modefield 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>
|
Second review addressed in a5a1fd7. Details are on OpenConceptLab/ocl_online#655.
|
snyaggarwal
left a comment
There was a problem hiding this comment.
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
|
Third review addressed in da8dc58.
|
…epair task test to the drop so it needs no Redis
snyaggarwal
left a comment
There was a problem hiding this comment.
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:
- 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.
- P2 — dropped-code repair arguments are not persisted. The stranded-task/manual rerun path requeues the repair without its required arguments.
- 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
|
Fourth review addressed in 76f0a23.
|
snyaggarwal
left a comment
There was a problem hiding this comment.
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.
…of versioning_mode; the import switch is seed_from_import
|
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" ( Testing: |
…import task id argument
paynejd
left a comment
There was a problem hiding this comment.
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
- [P1]
core/concepts/views.py:626(and the parents view):is_source_version()readsparent_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 atb14c5f0f:v1tov3each with P, A and B (A and B children of P), thenv4with P and A.children/of P returns A and B on all four versions, thoughv4holds 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. - [P1]
core/concepts/models.py:1419: after an import drops a child, the parent's HEADchildren/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 HEADP/children/returns A and B. Filter HEAD hierarchy lookups to active HEAD members inget_hierarchy_related_queryset. With oclapi2#990 merged,get_viewable_hierarchy_concept_urlswraps 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 atb14c5f0f: the version and HEAD end with 0 mappings. LOINC doesn't nest mappings. Track their ids, or reject nested mappings whenseed_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 atb14c5f0f: 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=trueon the deprecated/importers/bulk-import/upload/and/importers/bulk-import/file-url/is accepted, then dropped.queue_bulk_importpasses the switch only to inline tasks, andbulk_importdoesn'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, butimport_responsevalidates 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 inTEST_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 asto_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 oldmake_hierarchyaccepted. 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, butbulk_import_inlineisn't bound, so its version records{"type": "import", "task": null}. A TODO inqueue_bulk_importsays that task can't be reached, butBulkImportInlineViewreaches it withthreads=None. Bind the task and passself.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_urlrebuilds a version's URL from its label. Use the new version's ownuri, which is the same string today, so the seed and the version's own URL can't drift apart. - [P3] New:
0047addsversioning_modeand0048removes it. Neither has shipped, so folding them into a single migration that addsseeded_fromdrops the round trip, and the reverse-migration gap Codex found in0048with it. Do it unless a shared database (staging?) already ran0047. oclapi2#955 also adds a sources0047; 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 aseeded_fromin 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_fromand noversioning_mode. - A Source or Source Version import line can't set it, since neither importer's
allowed_fieldsincludes 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 withmasterat015eaaa2(#987, #990, #991 and #992 in). CI is green. - Re-ran the first review's probes at
b14c5f0fon a throwaway stack, plus new probes forseeded_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
}
]…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
|
Pass 2 review: changes in 85a6d41. Fixed
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, The local stack was down, so CI is running the tests for this push. Pylint 🤖 Addressed by Claude Code |
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=trueonPOST /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.IsolatedVersionCommitdoes 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 theindexingqueue (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.seed_from_import=truemakes 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.parent_concept_urlsand replace the old links, not add to them. A parent change forces a new concept version.ImportHierarchyresolves 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 themake_hierarchytask use it too.extras,external_id,datatype,sort_weightand the retire fields from the import, not from its previous version.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 otherwisenull. Migration0047added aversioning_modecolumn;0048replaces it withseeded_fromand backfills it: existing versions as seeded from HEAD, or from an import where 0047 recorded one.children/andparents/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
jsonblimit.CHANGELOG_ENRICHMENT_MAX_RESOURCES(default 50,000 new, removed or changed resources), a changelog skips names, descriptions andprev_*, and setsmeta.enrichment_skipped.Also fixed
Decimal(ijson), so any concept with a number inextrasfailed to save.Behaviour changes to be aware of
GETon HEAD returns 404 for it while older versions still have it.Testing
core/importers/tests.pyandcore/integration_tests/tests_sources.py:pylint -j0 core/exits 0.core.importers core.sources core.integration_tests.tests_sources core.common core.concepts core.mappingsran 1,224 tests.BulkImportLimitsTest.test_grandfathered_npm_url_is_downloaded_and_handed_over_as_uploadfails, as it does on master. A few tests that share the local ES fail only in parallel runs and pass on their own.Not in this PR
seeded_fromcan't be set in a request.match_algorithms.idare slow: each runs aMAX(mnemonic)scan, 2–6 s each.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