Skip to content

Let identifiers apply identifications to many occurrences in one request#1371

Merged
mihow merged 12 commits into
mainfrom
feat/bulk-identifications
Jul 23, 2026
Merged

Let identifiers apply identifications to many occurrences in one request#1371
mihow merged 12 commits into
mainfrom
feat/bulk-identifications

Conversation

@mihow

@mihow mihow commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

When an identifier confirms or re-identifies a page of occurrences, the site currently sends one request per occurrence. Selecting fifty occurrences and pressing Confirm fires fifty separate POSTs, and if some of them fail the interface asks the person to press the button again to retry the ones that did not land. This is slow on a large page, and it gets slower the more occurrences someone selects — which is exactly when they are working fastest.

This adds a single endpoint that accepts a whole batch of identifications in one request, and points the existing mass-identification UI at it. The visible effect for an identifier is that confirming or re-identifying a selected page is one round trip instead of one per occurrence, and the result comes back per occurrence: if one occurrence in the batch cannot be identified — because someone else deleted it since the page was loaded, say — that one is reported as an error and the rest are still saved. Nothing about how an identification behaves changes; this is a faster way to do what the site already does, deliberately not a different thing.

The motivation is load. During a mass-identification session, the old one-request-per-occurrence fan-out made the site pay the per-request overhead — a round trip, a request transaction, and an authorization — once for every occurrence, exactly when an identifier was working fastest. Collapsing a selection into one request pays that overhead once per batch instead. To be clear about what this does not do: the per-occurrence write work itself (withdrawing the previous identification and recomputing the occurrence's determination) is unchanged — it still runs once per occurrence, inside the batch just as it did in the fan-out. So this collapses N requests into one; the per-row database cost is the same. Reducing that per-row cost is a separate, larger change (a batched write over bulk_create), noted as a follow-up below.

List of Changes

# Change (what it does) How
1 An identifier can submit many identifications in one request instead of one request per occurrence. New POST /api/v2/identifications/bulk/ action on IdentificationViewSet. The batch resolve/validate/save logic lives in ami/main/models_future/identifications.py; the viewset action stays thin.
2 One bad occurrence in a batch no longer discards the good ones — each is reported on its own. Each item is saved in its own savepoint; the response lists an outcome per item, in request order, with created_count / error_count.
3 Identifying in bulk behaves exactly like identifying one at a time. Each item goes through the existing Identification.save(), so withdrawing the user's previous identification and recomputing the occurrence's determination are unchanged rather than reimplemented.
4 A person can only bulk-identify in a project they have permission to identify in. The batch is authorized against the single project it belongs to, via the same Identification.check_permission(user, "create") call the single-item endpoint makes.
5 Sending a bad batch gets a clear rejection instead of a server error. Batch-level problems (empty, over the cap, repeated occurrence, occurrences from two projects) return 400; everything else, including occurrences that no longer exist, is reported per item.
6 The endpoint does not get slower per occurrence as batches grow. Occurrences and taxa for the whole batch are each resolved in one query, rather than one lookup per item.
7 The mass-identification UI now sends one request instead of one per occurrence. The useCreateIdentifications hook posts the whole selection to the new endpoint. Its interface is unchanged, so Agree, Suggest ID, and the quick ID buttons keep working, including reporting how many items were rejected and resending only those on a retry.

Detailed Description

Why a list of items, and not one taxon plus a list of occurrence IDs

The shape was decided by what the interface actually sends. There are three bulk callers today, all of which already build a list of per-item values (ui/src/data-services/hooks/identifications/useCreateIdentifications.ts):

  • Agree / Confirm (ui/src/pages/occurrences/occurrences-actions.tsx) sends, for each occurrence, that occurrence's own current determination as the taxon and that occurrence's own identification or prediction as the agreement target.
  • Suggest ID and the quick ID buttons send one taxon applied to many occurrences.

The Agree flow is the common mass action and it is irreducibly heterogeneous, so a {taxon_id, occurrence_ids: []} request could not express it. A list of per-item objects covers all three and maps onto the payload the frontend already constructs, which keeps the eventual frontend change small.

Request and response

POST /api/v2/identifications/bulk/
{
  "identifications": [
    {"occurrence_id": 1, "taxon_id": 5, "comment": "optional",
     "agreed_with_identification_id": null, "agreed_with_prediction_id": null}
  ]
}
200 OK
{
  "created_count": 49,
  "error_count": 1,
  "results": [
    {"index": 0, "occurrence_id": 1, "status": "created", "id": 991},
    {"index": 1, "occurrence_id": 7, "status": "error",
     "errors": {"occurrence_id": ["Occurrence 7 does not exist."]}}
  ]
}

Results carry the submitted index, so a client matches them back to its request without relying on ordering alone.

All-or-nothing was considered and rejected. There is no invariant that spans the items — each identification is independent — so a transaction around the whole batch would buy no consistency, only a simpler error contract. It would also mean a batch containing one occurrence deleted mid-session retries into the identical failure forever unless the client prunes the offending index first. Saving the valid rows matches what someone doing a mass identification actually wants.

How partial success is actually achieved is worth a look during review. ATOMIC_REQUESTS is on (config/settings/base.py), so the whole request already runs in one transaction and a per-item transaction.atomic() is a savepoint rather than an independent commit. That means partial success does not come for free from wrapping each item: an exception escaping save() would abort the request's transaction and discard every identification already made in it. Each item is therefore saved inside a savepoint and its failure is caught, so rolling back to the savepoint leaves the connection usable and the loop continues. This is what makes the promise in the summary true rather than aspirational, and it is pinned by a test that forces a write to fail part-way through a batch and asserts the surviving items are really committed.

A batch may not span projects (400). All three call sites work within one project page, and restricting it keeps authorization unambiguous. This can be relaxed later without breaking clients.

Repeated occurrence IDs in one batch are rejected (400). Two identifications for one occurrence in a single request have no defined winner — the outcome would depend on insert-order tiebreaks rather than on anything the client asked for.

Two places where this is deliberately stricter than the single-item endpoint

Both are intentional, and both are asymmetries a reviewer should agree with rather than discover:

  1. withdrawn is not accepted from clients. The single-item serializer exposes it as writable, so a client can currently create an already-withdrawn identification. The model maintains this field, and letting a client set it breaks the "one active identification per user per occurrence" invariant. Probably worth a separate issue against the single-item endpoint.
  2. agreed_with_identification_id / agreed_with_prediction_id must belong to the occurrence being identified. The single-item endpoint accepts any identification or classification in the system as an agreement target, with no such check. The bulk endpoint validates it per item. The single-item path is left alone to keep this PR to one change; moving the rule to the model so both paths share it is tracked in Validate agreement-target ownership on every identification write path #1378.

Permissions

A detail=False action is never routed through has_object_permission, so the endpoint performs the check itself, following the transient-instance pattern already used by ProjectPipelineConfigPermission and UserMembershipPermission in ami/base/permissions.py.

Two details are worth flagging for review, because both are the kind of thing that is invisible in a diff:

  1. The action maps to "create", not to its own name. BaseModel.check_permission looks the action up in a CRUD map; an action name of "bulk" would miss that map and fall through to check_custom_permission, asking for a bulk_identification permission that no role grants. That fails closed, but only for non-superusers — a test written as a superuser would pass while every identifier got a 403.
  2. One check per batch rather than one per occurrence. This is equivalent because the "create" branch of Identification.check_permission depends only on the occurrence's project. Since that equivalence is an invariant a future edit could quietly break, this PR adds a note at Identification.check_permission itself — the line someone would actually be editing — rather than only at the endpoint.

Performance

The win here is round trips, not queries: N HTTP requests become one, with one authorization instead of N. Resolving occurrences and taxa is batched (one query each for the whole request), so validation cost does not grow per item.

The write path deliberately remains a loop over the existing Identification.save(). Measured at roughly 8.6 queries per item, all of them inside save() and update_occurrence_determination. At the MAX_BULK_IDENTIFICATIONS = 200 cap that is about 1,700 queries in a single request — on the order of 11 seconds wall-clock, measured in-process on a shared development box, so treat it as a loose upper bound rather than a clean benchmark. Because the request is synchronous, a full-cap batch holds a web worker for that whole time. In practice the UI bounds a selection to the occurrences on the page currently displayed (ui/src/pages/occurrences/occurrences.tsx), which is 25–100 rows, so a real request is a few seconds at most; the 200-item figure is only reachable by calling the API directly. Replacing the loop with bulk_create would mean reimplementing withdraw-previous and the determination recompute, neither of which has test coverage today to compare against — a large correctness risk for a saving that does not yet matter at the page-bounded sizes the UI produces. A batched write (or a lower cap, or making a large batch async) is a sensible follow-up once the determination logic has tests, and the request contract would not change.

Frontend

The useCreateIdentifications hook now makes one POST to the bulk endpoint instead of looping Promise.allSettled over per-occurrence POSTs. It keeps the same return shape (isLoading, isSuccess, error, createIdentifications), so the three callers — Agree (occurrences-actions.tsx), Suggest ID (suggest-id.tsx), and the quick ID buttons (id-button.tsx) — are untouched. Two behaviours are preserved deliberately:

  • Retry resends only the rejected items. The hook remembers which items failed and, on the next submit while an error is showing, sends only those. This matches the old retry-only-rejected behaviour, now against the endpoint's per-item results instead of settled promises.
  • A partial failure is not "success". The request succeeds at the HTTP level even when some items are rejected, so the hook reports success only once every item landed; that is what drives the "Confirmed" state on the Agree button.

The single-occurrence hook (useCreateIdentification, still used by the per-occurrence Agree card) is left as-is.

Testing

35 tests (the BulkIdentification* classes in ami/main/tests.py), written before the implementation:

  • Permission matrix — identifier, project manager, superuser, basic member (403), non-member (403), anonymous. Deliberately not written as a superuser, since that is what would hide the action-name problem described above.
  • Equivalence with the single-item endpoint — the same identification made via POST /identifications/ and via the bulk endpoint must leave the occurrence in an identical state (determination, score, and the set of identifications with their withdrawn flags), starting from an occurrence that already has an earlier identification so that withdraw-previous is part of what the two paths have to agree on. This is the test that catches anything the bulk path skips or reimplements, without having to enumerate each rule.
  • Side effects of save() — the user's previous identification is withdrawn; another user's identification is not.
  • Partial failure — a rejected item does not undo earlier successes, and a write that fails inside save() is reported against its own index while the other items stay committed. The latter fails without the savepoint handling described above, so it pins the behaviour rather than assuming it.
  • Validation — empty batch, missing key, non-integer ID (400 rather than 500), over the cap and exactly at the cap, repeated occurrence, cross-project batch, withdrawn ignored, unknown and cross-occurrence agreement targets, an all-missing batch.
  • Query cost — measured at two batch sizes and compared, since a single exact count cannot separate fixed cost from per-item cost and so cannot catch an N+1 in validation or in the response. The budget is the measured per-item cost, so any new per-item query fails it.
  • Frontend contract — a test sends occurrence and taxon IDs as strings, which is what the form actually sends, so a stricter integer-only field can't silently 400 every real bulk identification.

Beyond the automated tests, the endpoint was exercised over HTTP against a running server on a development deployment with production-copy data, so the real ATOMIC_REQUESTS commit path was covered (the test suite runs each test inside a transaction, where ATOMIC_REQUESTS degrades to a savepoint and the commit behaviour is never really tested). Each payload shape the UI produces was run against a real project and verified in the database:

  • Confirm / agree with an occurrence's own determination, with the machine prediction or an existing identification recorded as the agreement target — the new identification's agreed_with_prediction / agreed_with_identification is populated correctly.
  • Coarsen a selection to a family taxon, and to a computed common ancestor of the selection — the determination flips to the coarser taxon and the optional comment is stored.
  • Apply a reject / exclude taxon to a selection — the determination flips as expected.
  • Per-item guard: an agreement target belonging to a different occurrence is reported as that item's error while the rest of the batch proceeds.
  • Batch-level guards: cross-project and duplicate-occurrence batches, and anonymous requests, are rejected whole with 400 / 403.

Everything created during this manual pass was rolled back afterward. The frontend build, type-check, and lint all pass.

Notes for reviewers

Two pre-existing issues turned up while reading the surrounding code. Neither is touched here, and both are stated as observations rather than as things this PR fixes:

  • update_occurrence_determination compares an ID to a Taxon object. current_determination is read via .values("determination"), which yields the raw ID, and is then compared against top_identification.taxon, a model instance (ami/main/models.py). The comparison is therefore always unequal, so the "no update needed" branch is effectively unreachable and every identification rewrites the determination and score. This is verified, not inferred: a single POST agreeing with an existing determination moves determination_score from the machine score to 1.0. That may well be the desired behaviour — a human-confirmed occurrence arguably should score 1.0 — but it is currently reached by accident rather than by intent, and the surrounding code reads as though it were meant to no-op. Worth a ticket to decide which behaviour is wanted.
  • user_agrees_with_identification (ami/main/models.py) has no callers. It is also decorated with an unbounded functools.cache keyed on model instances, which would return stale results and grow without limit per worker if it were ever wired up. Worth deleting or fixing before anyone reaches for it.

A third, smaller one: Classification.detection is nullable, so a classification can outlive the detection that linked it to an occurrence. Agreeing with such a prediction is reported as an item error rather than crashing.

Summary by CodeRabbit

  • New Features

    • Added bulk identification submission through a single request, supporting up to 200 items.
    • Reports successful and failed items individually while preserving submission order.
    • Automatically retries only failed identifications.
    • Added validation for duplicate, invalid, missing, or cross-project items.
  • Bug Fixes

    • Prevents one failed identification from discarding successful submissions.
    • Improved handling and messaging for partial batch failures.
  • Documentation

    • Added documentation covering bulk submission behavior, design decisions, and future improvements.

Applying determinations to a page of occurrences currently costs one HTTP
request per occurrence: the identification UI fans out N POSTs with
Promise.allSettled and retries whichever ones were rejected. Add
POST /api/v2/identifications/bulk/ so the same work is one request.

The request is a list of per-item objects rather than one taxon applied to a
list of occurrence IDs, because the "Agree" action sends a different taxon and
a different agreement target for every occurrence. This matches the payload
the frontend already builds.

Each item is saved through the existing Identification.save(), so withdrawing
the user's previous identification and recomputing the occurrence
determination keep working exactly as they do for a single POST. Items are
saved in their own savepoints and reported individually, so one stale
occurrence does not discard the rest of the batch.

Occurrences and taxa are resolved in one query each, so validation does not
scale with the size of the batch. A batch is authorized once against the
single project it belongs to; permission to create an identification depends
only on the project, and the check is routed through the same
Identification.check_permission call the single-item endpoint makes.

Co-Authored-By: Claude <noreply@anthropic.com>
@netlify

netlify Bot commented Jul 17, 2026

Copy link
Copy Markdown

Deploy Preview for antenna-ssec ready!

Name Link
🔨 Latest commit 27bea92
🔍 Latest deploy log https://app.netlify.com/projects/antenna-ssec/deploys/6a61a24661d7dd0008f2f624
😎 Deploy Preview https://deploy-preview-1371--antenna-ssec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a bulk identification API with request limits, validation, project authorization, per-item persistence, and ordered results. The frontend submits batches through one request, tracks partial failures, retries failed items, and adds corresponding response types and messages.

Changes

Bulk identification workflow

Layer / File(s) Summary
Bulk API contract and endpoint
ami/main/api/serializers.py, ami/main/api/views.py, ami/main/models.py
Defines bulk request and response schemas, enforces batch constraints, exposes the bulk endpoint, and authorizes single-project batches.
Batch resolution and isolated persistence
ami/main/models_future/identifications.py
Bulk-resolves referenced records, validates occurrence relationships, and saves valid items independently while returning per-item errors.
Endpoint behavior and validation coverage
ami/main/tests.py
Covers successful creation, partial failures, validation, permissions, ordering, side effects, parity, and query-count behavior.
Frontend bulk submission and retry
ui/src/data-services/hooks/identifications/*, ui/src/data-services/models/identification.ts, ui/src/utils/language.ts
Submits one bulk request, tracks indexed failures, retries failed items, and adds response types and rejection messages.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

Possibly related PRs

Suggested labels: backend

Suggested reviewers: annavik

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.26% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: bulk identifications for many occurrences in one request.
Description check ✅ Passed The description is detailed and covers summary, changes, implementation details, testing, and risks; only optional template sections are missing.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/bulk-identifications

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@netlify

netlify Bot commented Jul 17, 2026

Copy link
Copy Markdown

Deploy Preview for antenna-preview ready!

Name Link
🔨 Latest commit 27bea92
🔍 Latest deploy log https://app.netlify.com/projects/antenna-preview/deploys/6a61a2468994c80007e8de77
😎 Deploy Preview https://deploy-preview-1371--antenna-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 55 (🔴 down 10 from production)
Accessibility: 81 (🔴 down 8 from production)
Best Practices: 92 (🔴 down 8 from production)
SEO: 92 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

mihow and others added 4 commits July 17, 2026 04:49
The endpoint promised that one bad item would not discard the rest of the
batch, but ATOMIC_REQUESTS wraps the whole request in a transaction, so the
per-item atomic() block was a savepoint rather than a separate transaction and
nothing committed on its own. An error raised inside save() propagated, aborted
the request transaction, and rolled back every identification already made in
it, returning a 500 instead of a per-item error. Catch the failure so the
savepoint rolls back that item alone and the loop continues, and correct the
comment that claimed otherwise.

Also fixed while reviewing:

- Agreeing with a prediction whose detection had been deleted raised an
  AttributeError and returned a 500, because Classification.detection is
  nullable. It is now reported as an item error.
- A batch in which no occurrence existed returned a batch-level 400 while a
  batch in which only some were missing reported per-item errors. The same
  failure now answers the same way in both cases.
- MAX_BULK_IDENTIFICATIONS moved to serializers, where it is enforced, which
  removes an import of views from inside a serializer method.

Tests: the partial-success guarantee is now pinned by forcing a write to fail
mid-batch, which fails without the fix above. The cap test used repeated IDs, so
the duplicate check rejected the batch and the test passed with the cap removed;
it now uses distinct IDs and asserts the message. The equivalence test seeded no
prior identification, so it never exercised withdraw-previous. Added coverage
for unknown and cross-occurrence agreement targets and for an all-missing batch,
and tightened the per-item query budget to the measured cost.

Co-Authored-By: Claude <noreply@anthropic.com>
…plan

[skip ci]

Co-Authored-By: Claude <noreply@anthropic.com>
The occurrences list already lets an identifier select a page of occurrences
and apply a determination to all of them at once ("select all", then Agree or
Suggest ID). Until now that fanned out one HTTP request per occurrence, so
confirming a page of twenty sent twenty requests, each doing its own
authorization and its own determination recompute. During a mass-identification
session that multiplies the load on the site.

This points the bulk identification hook at the new
POST /api/v2/identifications/bulk/ endpoint, so the whole selection goes in a
single request. The three callers (Agree, Suggest ID, and the quick ID buttons)
are unchanged: the hook keeps the same interface, including reporting how many
items were rejected and resending only those on a retry. A partial failure is
still a successful request, so the "Confirmed" state only shows once every item
landed.

The backend accepts occurrence and taxon IDs as strings, which is what the form
sends; a test pins that contract.

Co-Authored-By: Claude <noreply@anthropic.com>
[skip ci]

Co-Authored-By: Claude <noreply@anthropic.com>
@mihow
mihow marked this pull request as ready for review July 22, 2026 21:29
Copilot AI review requested due to automatic review settings July 22, 2026 21:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a bulk-identifications API endpoint so the mass-identification UI can submit many identifications in a single request (with per-item outcomes), reducing request fan-out and improving performance during identifier workflows.

Changes:

  • Add POST /api/v2/identifications/bulk/ on IdentificationViewSet that processes a list of identifications and returns per-item results.
  • Update the frontend mass-identification hook to send one bulk request and preserve “retry only failed items” behavior.
  • Add comprehensive backend test coverage for permissions, partial failure, validation, and query-slope constraints; include supporting session/planning docs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ui/src/data-services/hooks/identifications/useCreateIdentifications.ts Switches mass-identification to a single bulk POST and tracks per-item failures for retry.
ui/src/data-services/hooks/identifications/useCreateIdentification.ts Exports the field-value converter for reuse by the bulk hook.
ami/main/api/views.py Adds the bulk action and supporting batch resolve/validate/authorize helpers with savepoint-per-item behavior.
ami/main/api/serializers.py Adds request/response serializers and validation rules (cap + duplicate occurrence IDs).
ami/main/models.py Documents a key permission invariant relied upon by the bulk endpoint’s batch-level authorization.
ami/main/test_bulk_identifications.py Adds a full test suite to pin behavior, permissions, partial success semantics, and query scaling.
docs/claude/sessions/2026-07-17-bulk-identifications.md Session notes capturing design decisions and gotchas for future maintainers.
docs/claude/planning/bulk-identifications-batched-write-followup.md Follow-up design doc for a potential future batched-write optimization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ui/src/data-services/hooks/identifications/useCreateIdentifications.ts Outdated
Comment thread ami/main/api/views.py Outdated
mihow and others added 2 commits July 22, 2026 14:53
The retry-by-index path matched failed items by position in the results
array, which only worked because the backend appends results in request
order. Key the failed set off the index the backend reports against each
result so a reordered or sparse response still maps each error back to the
right submitted item.

Co-Authored-By: Claude <noreply@anthropic.com>
…ariant

Name the two decisions a takeaway review flagged as unstated: per-item
problems return 200 with per-item errors while only batch-level problems
return a 400 (a different error shape), and created_count reflects only
savepoint-committed writes because ATOMIC_REQUESTS commits the outer
transaction before the response is sent.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (1)
ami/main/api/views.py (1)

2296-2325: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Narrow the bulk savepoint error handling to the documented failure modes.

DatabaseError broadens the handled cases enough to include OperationalError conditions like connection loss or statement failures, which can be returned as routine "occurrence could not be identified" errors per batch. Catch IntegrityError for integrity/FK failures and exceptions.ObjectDoesNotExist for missing rows instead.

♻️ Proposed narrowing
-            except (DatabaseError, exceptions.ObjectDoesNotExist) as error:
+            except (IntegrityError, exceptions.ObjectDoesNotExist) as error:

Add the import (from django.db import IntegrityError) alongside the existing django.db imports.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ami/main/api/views.py` around lines 2296 - 2325, In the bulk savepoint
handler around identification.save(), replace the broad DatabaseError catch with
IntegrityError and exceptions.ObjectDoesNotExist, and add the IntegrityError
import alongside the existing django.db imports. Preserve the current warning,
error result, and batch-continuation behavior for those documented failure
modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/claude/planning/bulk-identifications-batched-write-followup.md`:
- Around line 50-62: Update the bulk identification transaction around
Identification.objects operations to first lock all affected Occurrence rows
with select_for_update, using a stable ordering before any withdrawal or
insertion. Preserve the current endpoint’s row-locking behavior and ensure the
locked occurrence set is used for the subsequent determination recomputation and
bulk updates.

In `@ui/src/data-services/hooks/identifications/useCreateIdentifications.ts`:
- Around line 76-79: Update the retry-state reset effect in
useCreateIdentifications so it depends on a stable signature derived from the
selected occurrence IDs rather than occurrenceIds.length. Ensure replacing the
selection with a same-sized but different set clears lastAttempt before any
retry can reuse stale selection state.
- Around line 10-22: Move the bulk endpoint response interfaces out of the hook
and define shared ServerBulkIdentificationResult and
ServerBulkIdentificationResponse interfaces under src/data-services/models/.
Update useCreateIdentifications to import and use those server model types,
preserving the existing fields and status union.
- Around line 82-90: Update the partialError and requestError messages in
useCreateIdentifications to use translation-layer keys rather than hardcoded
strings. Add sentence-case entries to STRING and ENGLISH_STRINGS, including the
rejected and total count placeholders, then call translate(...) with the
appropriate keys and values while preserving the existing singular, partial, and
whole-request rejection behavior.
- Around line 70-72: Update the mutation flow in useCreateIdentifications to
store the success-reset timeout handle, clear any pending timeout before each
mutateAsync call, and clear it during unmount cleanup. Preserve the existing
delayed reset after successful requests while preventing stale timers from
resetting newer mutations.

---

Nitpick comments:
In `@ami/main/api/views.py`:
- Around line 2296-2325: In the bulk savepoint handler around
identification.save(), replace the broad DatabaseError catch with IntegrityError
and exceptions.ObjectDoesNotExist, and add the IntegrityError import alongside
the existing django.db imports. Preserve the current warning, error result, and
batch-continuation behavior for those documented failure modes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1ae36393-4cfc-424b-9020-383d034e9ec6

📥 Commits

Reviewing files that changed from the base of the PR and between 3f85d84 and 737f175.

📒 Files selected for processing (8)
  • ami/main/api/serializers.py
  • ami/main/api/views.py
  • ami/main/models.py
  • ami/main/test_bulk_identifications.py
  • docs/claude/planning/bulk-identifications-batched-write-followup.md
  • docs/claude/sessions/2026-07-17-bulk-identifications.md
  • ui/src/data-services/hooks/identifications/useCreateIdentification.ts
  • ui/src/data-services/hooks/identifications/useCreateIdentifications.ts

Comment thread docs/claude/planning/bulk-identifications-batched-write-followup.md Outdated
Comment thread ui/src/data-services/hooks/identifications/useCreateIdentifications.ts Outdated
Comment thread ui/src/data-services/hooks/identifications/useCreateIdentifications.ts Outdated
@mihow
mihow marked this pull request as draft July 23, 2026 03:13
mihow and others added 4 commits July 22, 2026 20:20
The bulk endpoint's resolve/validate/save loop now lives in
ami/main/models_future/identifications.py, keeping the viewset action thin:
parse the request, authorize the batch, delegate, shape the response. This
follows the models_future pattern (filters, occurrence, projects) and gives
the follow-up model-level agreement validation (#1378) a natural home.

Also, per review feedback, the per-item save handler catches IntegrityError
instead of the broader DatabaseError so connection-level failures surface as
errors instead of being reported as routine per-item rejections, and the
per-item error message no longer claims the occurrence was deleted when the
cause may be another related record. Inline comments are trimmed to the
repo's short-comment norm.

Co-Authored-By: Claude <noreply@anthropic.com>
…odule

The bulk tests lived in a separate ami/main/test_bulk_identifications.py
module; identification tests in this app live in ami/main/tests.py, so the
classes move there unchanged apart from a renamed endpoint constant.

Co-Authored-By: Claude <noreply@anthropic.com>
The batched-write follow-up plan and session notes are working documents,
not part of the feature; they are preserved in the branch history.

Co-Authored-By: Claude <noreply@anthropic.com>
Three changes from review:

1. The retry state now clears when the selected occurrence IDs change, not
   only when their count changes, so a retry can never resend items from a
   previously selected set of the same size.
2. The delayed success reset keeps its timeout handle; it is cancelled
   before each new submission and on unmount, so a stale timer can no
   longer reset a newer in-flight mutation.
3. The response wire types move to data-services/models/identification.ts
   as Server-prefixed interfaces, and the rejection messages go through the
   translation layer with count placeholders.

Co-Authored-By: Claude <noreply@anthropic.com>
@mihow

mihow commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Claude says: This branch went through a restructuring round after maintainer feedback, so a re-review should start from the current diff rather than the earlier commits:

  • The bulk resolve/validate/save logic moved out of ami/main/api/views.py into a new ami/main/models_future/identifications.py module; the viewset action is now a thin parse → authorize → delegate → respond wrapper (4dac382).
  • The tests moved from a separate ami/main/test_bulk_identifications.py module into ami/main/tests.py, where this app's identification tests live (8fdaf79).
  • The session planning notes were removed from the branch (1b0746e).
  • All eight review comments above are addressed and resolved: the frontend retry-state and stale-timer issues, the wire types and translation-layer items, and the backend error-handling narrowing (IntegrityError instead of DatabaseError, with a message that no longer asserts the occurrence was deleted).

Full ami.main suite passes after the move (321 tests), and the frontend lint, type-check, and formatting pass. The PR is set back to draft while the combined branch is re-verified on a dev box; it will be marked ready again after that.

@mihow
mihow marked this pull request as ready for review July 23, 2026 03:45
@mihow

mihow commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mihow

mihow commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ui/src/data-services/hooks/identifications/useCreateIdentifications.ts`:
- Around line 75-88: Update the error selection in the identification creation
hook so the current whole-request failure takes precedence over the stale
partial failure: when isError is true, use requestError; otherwise retain
partialError. Keep the existing partial-rejection translations unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d52c8ea-7ff5-43a8-babd-ff5cfe8ccda1

📥 Commits

Reviewing files that changed from the base of the PR and between 3f85d84 and f6a4868.

📒 Files selected for processing (9)
  • ami/main/api/serializers.py
  • ami/main/api/views.py
  • ami/main/models.py
  • ami/main/models_future/identifications.py
  • ami/main/tests.py
  • ui/src/data-services/hooks/identifications/useCreateIdentification.ts
  • ui/src/data-services/hooks/identifications/useCreateIdentifications.ts
  • ui/src/data-services/models/identification.ts
  • ui/src/utils/language.ts

Comment thread ui/src/data-services/hooks/identifications/useCreateIdentifications.ts Outdated
…message

After a partial failure, a retry rejected at the request level (permission
denied, network error) kept showing the previous "N/M updates were
rejected" message because the partial-failure state is only updated on a
successful response. The whole-request error now takes precedence, so the
newest failure is what the identifier sees. Retry behavior is unchanged:
the failed items from the last successful response are still what a retry
resends.

Co-Authored-By: Claude <noreply@anthropic.com>
@mihow

mihow commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Claude says: Ran a manual verification pass over HTTP on a development deployment (production-copy data), exercising every bulk-identification payload shape the UI can produce, not just "confirm". The ## Testing and ### Performance sections above were updated to match. Summary:

Shapes verified against a real project, each confirmed in the database, then rolled back:

Shape Result
Confirm / agree with own determination 200; identification created
Agree with a machine prediction as the target agreed_with_prediction persisted
Agree with an existing identification as the target agreed_with_identification persisted
Coarsen a selection to a family taxon (+ comment) determination flipped; comment stored
Coarsen to a computed common ancestor of the selection determination flipped to the ancestor
Apply a reject / exclude taxon determination flipped
Agreement target from a different occurrence reported as that item's error; rest of batch proceeds
Cross-project batch / duplicate occurrence / anonymous rejected whole with 400 / 403

At-cap performance (measured, worth a look before merge): a full MAX_BULK_IDENTIFICATIONS = 200 batch runs about 8.6 queries per item — roughly 1,700 queries and ~11 s in one synchronous request (measured in-process on a shared box, so a loose upper bound). The UI bounds a selection to the page currently displayed (25–100 rows), so a real request is a few seconds; the 200-item figure is only reachable by calling the API directly. Not a blocker given the page-bounding, but if the cap should stay callable at 200, lowering it, making a large batch async, or moving to a batched write are reasonable follow-ups (the request contract would not change).

@mihow
mihow merged commit 731cc2a into main Jul 23, 2026
9 checks passed
@mihow
mihow deleted the feat/bulk-identifications branch July 23, 2026 06:04
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