Let admins mask classifier predictions to a species list#999
Open
mohamedelabbas1996 wants to merge 8 commits into
Open
Let admins mask classifier predictions to a species list#999mohamedelabbas1996 wants to merge 8 commits into
mohamedelabbas1996 wants to merge 8 commits into
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
mohamedelabbas1996
force-pushed
the
feat/postprocessing-class-masking
branch
from
0b77504 to
88ffba8
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
Implements class masking as a post-processing task that recalculates classifications by masking out classes not present in a provided taxa list and updates occurrences accordingly.
- Adds ClassMaskingTask to the post-processing framework and registers it.
- Filters and recalculates logits/scores per taxa list, creates new terminal classifications, and updates occurrences.
- Minor logging update in job runner.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| ami/ml/post_processing/class_masking.py | New class masking task and supporting functions to filter classifications by taxa list and recompute softmax. |
| ami/ml/post_processing/init.py | Registers the new class_masking task module. |
| ami/jobs/models.py | Improves log line to print only the task config for post-processing. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mihow
reviewed
Oct 15, 2025
| # Update the occurrence determinations | ||
| logger.info(f"Updating the determinations for {len(occurrences_to_update)} occurrences") | ||
| for occurrence in occurrences_to_update: | ||
| occurrence.save(update_determination=True) |
Collaborator
There was a problem hiding this comment.
@mohamedelabbas1996 here is how I updated all of the determinations previously
✅ Deploy Preview for antenna-ssec canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ChangesClass masking post-processing feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as Django Admin User
participant Form as ClassMaskingActionForm
participant Task as ClassMaskingTask
participant Fn as make_classifications_filtered_by_taxa_list
participant DB as Database
Admin->>Form: select occurrence/collection, submit action
Form-->>Admin: render algorithm, taxa_list, reweight fields
Admin->>Task: confirm (creates Job with ClassMaskingConfig)
Task->>Task: resolve scope, get/create masking algorithm
Task->>Fn: run rescoring on scoped classifications
Fn->>DB: bulk_update demote originals
Fn->>DB: bulk_create new terminal classifications
Fn->>DB: save occurrences (update_determination=True)
Fn-->>Task: metrics (masked, occurrences_updated)
Task-->>Admin: job progress and completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
ami/ml/post_processing/class_masking.py (2)
149-161: Variable shadowing:logitsandscoresare reassigned, hiding the originals from line 126.
logitsis first bound fromclassification.logits(line 126), then rebound tologits_np.tolist()(line 151). Similarlyscoresis rebound at line 161. This makes the code confusing—use distinct names likemasked_logitsandmasked_scores.Proposed fix
- logits: list[float] = logits_np.tolist() + masked_logits: list[float] = logits_np.tolist() ... - scores: list = scores_np.tolist() + masked_scores: list[float] = scores_np.tolist()Then update all downstream references accordingly (lines 163, 166, 174, 186-188).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` around lines 149 - 161, The code rebinds existing variables logits and scores (originally from classification.logits) which causes variable shadowing and confusion; rename the converted/filtered values (e.g., change logits_np.tolist() to masked_logits and scores_np.tolist() to masked_scores), keep logits_np and scores_np as-is, and update all downstream uses that currently reference logits or scores to use masked_logits and masked_scores (also check any other places using logits or scores after the masking block and adjust them to the new names).
48-75:jobparameter is accepted but unused.
update_occurrences_in_collectionaccepts ajobparameter (line 55) but never uses it. Either remove it or pass it along for progress tracking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` around lines 48 - 75, The function update_occurrences_in_collection currently accepts a job parameter but never uses it; either remove the unused parameter or propagate it for progress tracking—preferably pass job through to downstream processing so callers can monitor progress. Modify update_occurrences_in_collection (and any TODO block that creates the AlgorithmCategoryMap) to accept and forward job to make_classifications_filtered_by_taxa_list by adding job=job to that call and, if needed, update make_classifications_filtered_by_taxa_list's signature (and any other helper functions) to accept and use job for progress/log updates; alternatively, if you choose to remove it, delete the job parameter from update_occurrences_in_collection and update all callers accordingly.ami/ml/post_processing/rank_rollup.py (1)
104-106: Remove commented-out debugger import.Proposed fix
threshold = thresholds.get(rank, 1.0) - # import pdb - - # pdb.set_trace() candidates = {t: s for t, s in taxon_scores.items() if t.rank == rank}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/rank_rollup.py` around lines 104 - 106, Remove the leftover commented debugger lines in the rank rollup module: delete the lines containing "# import pdb" and "# pdb.set_trace()" in ami/ml/post_processing/rank_rollup.py (look for the commented debugger import and trace calls near the post-processing/rank rollup logic) so no commented debugger statements remain; ensure no other pdb references are left in functions or methods within that module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/main/admin.py`:
- Around line 738-757: The DEFAULT_THRESHOLDS in the admin action
run_rank_rollup uses lowercase keys ("species","genus","family") but
RankRollupTask expects uppercase keys via RankRollupTask.ROLLUP_ORDER; update
DEFAULT_THRESHOLDS to use the same uppercase keys (e.g.,
"SPECIES","GENUS","FAMILY") or transform keys before creating the Job params so
they match RankRollupTask.ROLLUP_ORDER/lookup; ensure the symbol
DEFAULT_THRESHOLDS and the params payload in run_rank_rollup are consistent with
RankRollupTask.ROLLUP_ORDER used in rank_rollup.py.
- Around line 429-474: The admin action update_with_newfoundland_species
hardcodes TaxaList and Algorithm names and uses a bare except Exception; change
it to accept configurable taxa_list and algorithm (e.g., read from settings,
admin action parameters, or a constant at module top) instead of using the
literal "Newfoundland Species" and "Quebec & Vermont Species Classifier - Apr
2024", and add a TODO or link to your tracking issue to avoid shipping hardcoded
values; also replace the broad except in the loop with a narrower exception (or
catch Exception only to log full traceback via logging.exception and then
re-raise or continue) so programming errors aren't silently swallowed—update
references: update_with_newfoundland_species, TaxaList.objects.get,
Algorithm.objects.get, and update_single_occurrence accordingly.
In `@ami/ml/post_processing/class_masking.py`:
- Line 129: Move the numpy imports out of the loop and to module level: add
"import numpy as np", "from numpy import exp", and "from numpy import sum as
np_sum" at the top of the file, then delete the in-loop statements "import numpy
as np", "from numpy import exp", and "from numpy import sum" (the imports
currently inside the for loop). Ensure subsequent code uses np, exp, and np_sum
consistently after the change.
- Around line 170-171: Remove the two debug print statements that output "Top
taxon" and "Top index" in ami/ml/post_processing/class_masking.py; either delete
the lines that call print(...) or replace them with a proper logger.debug(...)
call (using the module/class logger) so the variables
category_map_with_taxa[top_index] and top_index are not printed to stdout in
production.
- Around line 206-208: Replace the root-logger call logging.info(...) with the
module-level logger call logger.info(...) where the message "Adding new
classification for Taxon {top_taxon} to occurrence
{new_classification.detection.occurrence}" is emitted; locate this call in the
function that creates new_classification and ensure the module-level logger
(logger defined at top of the module) is used so module-specific logging
configuration is respected.
- Around line 183-197: The new Classification is being created with
category_map=None which breaks methods like predictions() and top_n() because
Classification.save() (which auto-populates category_map from
algorithm.category_map) isn't called when later using bulk_create; update the
creation logic in class_masking.py so the new Classification's category_map is
explicitly set from new_algorithm.category_map (or otherwise ensure
Classification.save() runs before bulk_create) when constructing
new_classification (referencing Classification, new_classification,
new_algorithm, and Classification.save()) so downstream calls to
.predictions()/.top_n() have a valid category_map.
- Around line 84-112: The queryset taxa_in_list is iterated with "not in"
causing repeated DB hits and len(classifications) forces full evaluation; change
taxa_in_list = taxa_list.taxa.all() to a concrete set (e.g., a set of taxon PKs
or Taxon instances) and update the exclusion check in the comprehension that
builds excluded_category_map_with_taxa to test membership against that set (use
category["taxon"].pk or category["taxon"] depending on which identity you
store), and replace len(classifications) with classifications.count() to avoid
loading the whole QuerySet into memory; update any related variables like
category_map_with_taxa and excluded_category_indices to work with the chosen
identity (pk vs instance).
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 37-38: DEFAULT_THRESHOLDS and ROLLUP_ORDER use uppercase rank keys
while external callers pass lowercase, causing thresholds.get(rank, 1.0) to miss
matches; fix by normalizing incoming threshold keys to the same case as
ROLLUP_ORDER/DEFAULT_THRESHOLDS (e.g., convert provided thresholds to upper()
with something like thresholds = {k.upper(): v for k, v in thresholds.items()}
before any lookups/merging), and ensure any merging logic that references
DEFAULT_THRESHOLDS (and places where thresholds.get(rank, 1.0) is used) operates
on the normalized dict so rollup comparisons use the intended values.
- Around line 138-143: The code assumes clf.detection.occurrence is always
present and will raise when it's None; update the block around
updated_occurrences.append(occurrence) and the self.logger.info call to guard
against a missing occurrence: retrieve occurrence = clf.detection.occurrence,
then if occurrence is not None append it and log the rolled-up message using
occurrence.pk, otherwise handle the null case (e.g., skip appending and log a
different message referencing clf.detection.pk or clf.pk and the
new_taxon/new_score) so no AttributeError is raised when occurrence is None.
- Around line 70-71: The log formatting crashes when Classification.score is
None; update the logger.info call inside the loop (the for i, clf in
enumerate(qs.iterator(), start=1) block) to handle a nullable clf.score by
rendering a safe value—e.g., compute a short_score variable that is
f"{clf.score:.3f}" if clf.score is not None else "N/A" (or 0.0) and use that in
the message instead of directly formatting clf.score with : .3f so None never
raises TypeError.
- Around line 82-96: The per-label DB hit comes from
Taxon.objects.filter(name=label).first() inside the loop over
clf.scores/clf.category_map.labels; build a single mapping of labels to Taxon
before iterating classifications by collecting all labels (from
clf.category_map.labels across classifications), querying
Taxon.objects.filter(name__in=labels) once to produce a dict label_to_taxon,
then replace Taxon.objects.filter(...).first() with label_to_taxon.get(label)
and keep the existing handling (log skip when None) while leaving the rest of
the logic (rollup_order, find_ancestor_by_parent_chain, taxon_scores
accumulation, and debug logging) unchanged.
In `@ami/ml/tests.py`:
- Around line 973-1009: The thresholds dict keys are case-sensitive causing
lookups in RankRollupTask to miss lowercase keys and fall back to 1.0; fix this
by normalizing threshold keys or lookup to be case-insensitive — e.g., in the
RankRollupTask initializer or wherever you read thresholds (the thresholds
parameter and the code that uses thresholds.get(...)), convert keys to a
canonical form (upper-case) or use thresholds.get(taxon.rank.upper(),
default_threshold) when computing aggregate scores; after fixing, update the
test inputs if needed so genus/species scores exercise the intended non-1.0
thresholds.
---
Nitpick comments:
In `@ami/ml/post_processing/class_masking.py`:
- Around line 149-161: The code rebinds existing variables logits and scores
(originally from classification.logits) which causes variable shadowing and
confusion; rename the converted/filtered values (e.g., change logits_np.tolist()
to masked_logits and scores_np.tolist() to masked_scores), keep logits_np and
scores_np as-is, and update all downstream uses that currently reference logits
or scores to use masked_logits and masked_scores (also check any other places
using logits or scores after the masking block and adjust them to the new
names).
- Around line 48-75: The function update_occurrences_in_collection currently
accepts a job parameter but never uses it; either remove the unused parameter or
propagate it for progress tracking—preferably pass job through to downstream
processing so callers can monitor progress. Modify
update_occurrences_in_collection (and any TODO block that creates the
AlgorithmCategoryMap) to accept and forward job to
make_classifications_filtered_by_taxa_list by adding job=job to that call and,
if needed, update make_classifications_filtered_by_taxa_list's signature (and
any other helper functions) to accept and use job for progress/log updates;
alternatively, if you choose to remove it, delete the job parameter from
update_occurrences_in_collection and update all callers accordingly.
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 104-106: Remove the leftover commented debugger lines in the rank
rollup module: delete the lines containing "# import pdb" and "#
pdb.set_trace()" in ami/ml/post_processing/rank_rollup.py (look for the
commented debugger import and trace calls near the post-processing/rank rollup
logic) so no commented debugger statements remain; ensure no other pdb
references are left in functions or methods within that module.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
ami/ml/post_processing/class_masking.py (2)
51-75:jobandparamsparameters are accepted but never used.
jobis accepted (line 58) but never referenced for progress reporting, andparams(line 55) is only logged. If progress tracking is intended, wirejobinto the loop. Otherwise, remove unused parameters to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` around lines 51 - 75, The function update_occurrences_in_collection accepts job and params but never uses them; either remove these parameters or wire them up: if progress reporting is intended, pass the job into make_classifications_filtered_by_taxa_list (or use job.update_progress / task_logger within the loop inside make_classifications_filtered_by_taxa_list) and use params to control behavior (e.g., filter options or batch sizes) when building classifications; otherwise remove job and params from the signature and all call sites to avoid dead API surface. Ensure you update references to update_occurrences_in_collection and make_classifications_filtered_by_taxa_list so their signatures match and document the chosen behavior.
184-186:assertstatements will be stripped underpython -O; use explicit checks.Lines 184–186 use
assertfor runtime validation of detection/occurrence presence. These are silently removed when Python runs with optimizations enabled. Since this guards data integrity before bulk operations, use explicitif/raiseinstead.Proposed fix
- assert new_classification.detection is not None - assert new_classification.detection.occurrence is not None - occurrences_to_update.add(new_classification.detection.occurrence) + if new_classification.detection is None: + raise ValueError(f"New classification for {classification.pk} has no detection.") + if new_classification.detection.occurrence is None: + raise ValueError(f"Detection {new_classification.detection.pk} has no occurrence.") + occurrences_to_update.add(new_classification.detection.occurrence)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` around lines 184 - 186, Replace the runtime asserts that check new_classification.detection and new_classification.detection.occurrence with explicit checks that raise a clear exception (e.g., ValueError or RuntimeError) so they are not stripped under python -O; specifically, in the block where new_classification is processed (the code referencing new_classification.detection, new_classification.detection.occurrence and occurrences_to_update.add(...)), change the two assert lines to if-not checks that raise including contextual information (e.g., include new_classification id or index) before adding to occurrences_to_update.ami/ml/tests.py (1)
1182-1187: Missinglogits__isnull=Falsefilter — inconsistent with production code path.
update_occurrences_in_collectioninclass_masking.py(line 67) filters withlogits__isnull=False, but this test queryset at lines 1182–1187 omits it. While the test data always has logits, adding the filter would keep the test closer to the production query and help catch regressions.Proposed fix
classifications = Classification.objects.filter( detection__occurrence=occ, terminal=True, algorithm=self.algorithm, scores__isnull=False, + logits__isnull=False, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/tests.py` around lines 1182 - 1187, The test queryset using Classification.objects.filter(detection__occurrence=occ, terminal=True, algorithm=self.algorithm, scores__isnull=False) should mirror production by also filtering out null logits; update the filter call in the test to include logits__isnull=False so the query matches the one used in update_occurrences_in_collection in class_masking.py and will catch regressions related to missing logits.ami/ml/post_processing/rank_rollup.py (2)
13-26: Parent-chain traversal triggers a DB query per hop.Each call to
current.parentis a lazy FK lookup — one query per ancestor level. When called inside the nested loop (line 95), this multiplies with the number of labels × ranks × classifications.Consider prefetching or caching the parent chain. For example, if
Taxonstoresparents_jsonor a materialized path, use that instead. Otherwise, a single upfront query building ataxon_id → parentmap would eliminate the repeated hits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/rank_rollup.py` around lines 13 - 26, The parent-chain traversal in find_ancestor_by_parent_chain causes a DB query per current.parent access; modify the implementation to avoid lazy FK hops by either using Taxon.parents_json or a materialized path if available, or change the function signature to accept a prebuilt mapping (e.g., taxon_id → parent_id/object) and traverse that map instead of current.parent; update callers (the nested loop that invokes find_ancestor_by_parent_chain) to pass the preloaded map or parents_json after performing a single upfront query that fetches all taxa and parent relationships, so ancestor lookups are O(depth) in-memory rather than multiple DB queries.
70-152: Long-running transaction wraps the entire classification loop including per-row DB writes.The
transaction.atomic()block spans the full iteration over potentially thousands of classifications, each performing anUPDATE(line 126) and aCREATE(line 127). This holds a database transaction open for the entire duration, which can cause lock contention and connection pool exhaustion under load.Consider batching: accumulate the updates/creates and perform them in smaller atomic batches (similar to how
class_masking.pyusesbulk_update/bulk_createoutside the loop).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/rank_rollup.py` around lines 70 - 152, The transaction.atomic() currently wraps the entire loop causing long transactions; change to process classifications in small batches: iterate the queryset without a global atomic, accumulate pending Classification updates (e.g., mark non-terminal) and new Classification instances (for create) into lists (reference the Classification.objects.filter(...).update(...) and Classification.objects.create(...) usages), and every N items (configurable batch_size) run a short transaction.atomic() that performs bulk_update or bulk_create for the accumulated items and clears the buffers; ensure you still append updated occurrences to updated_occurrences and call update_progress at the same batch boundary so behavior remains the same while avoiding one long transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/ml/post_processing/class_masking.py`:
- Around line 238-250: self.algorithm.category_map is only updated in memory
(assignment of algorithm.category_map to self.algorithm.category_map) so the
Algorithm DB row stays stale; after the in-memory assignment you should persist
the change by saving the Algorithm model (e.g., call
self.algorithm.save(update_fields=["category_map"])) or alternatively avoid
mutating self.algorithm and pass algorithm.category_map explicitly into
update_occurrences_in_collection/Classification creation; update the code around
the assignment and before calling update_occurrences_in_collection to ensure the
Algorithm DB record's category_map FK is saved.
- Around line 148-155: The equality check using classification.scores == scores
and classification.logits == logits is fragile due to floating-point
differences; replace it with a tolerance-based comparison (e.g., numpy.allclose
or an elementwise isclose with a small rtol/atol) when comparing
classification.scores to scores and classification.logits to logits inside the
block around top_index/top_taxon; alternatively or additionally, check whether
any indices that would be masked had non-negligible original logits (e.g.,
abs(logit) > tiny_threshold) before deciding to skip updating to avoid spurious
updates from rounding noise.
- Around line 109-113: The logger.info call is forcing materialization of the
classifications QuerySet by using len(classifications); replace that with the
previously computed classification_count (or call classifications.count()) so
the QuerySet is not evaluated into memory—update the f-string in the logging
statement where logger.info is called (referencing category_map_with_taxa,
excluded_category_map_with_taxa, and classifications/classification_count) to
use classification_count instead of len(classifications).
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 59-63: The query building variable qs uses
Classification.objects.filter(...) joining through
detection__source_image__collections which can produce duplicate Classification
rows; update the query in rank_rollup.py (the qs assignment using
Classification.objects.filter with terminal=True, taxon__isnull=False,
detection__source_image__collections__id=collection_id) to call .distinct()
after the filter to deduplicate results (match the pattern used by
update_occurrences_in_collection in class_masking.py).
In `@ami/ml/tests.py`:
- Around line 942-971: The category map is built from species_taxa using the
model's default ordering which can differ from the tests that explicitly use
.order_by("name") — update the query in _create_category_map_with_algorithm to
fetch species_taxa =
list(self.project.taxa.filter(rank=TaxonRank.SPECIES.name).order_by("name")[:3])
(and apply the same .order_by("name") change to any other similar query that
builds category maps or compares indices, e.g., the other occurrence around
lines 1025-1037) so the AlgorithmCategoryMap.data and labels indices align with
the test's expected species ordering; keep building AlgorithmCategoryMap and
Algorithm exactly as before.
---
Duplicate comments:
In `@ami/main/admin.py`:
- Around line 429-474: The admin action update_with_newfoundland_species
currently swallows all exceptions with a broad "except Exception as e" around
update_single_occurrence calls; when you generalize this action replace that
broad handler with a narrower one such as "except (ValueError, RuntimeError,
TaxaList.DoesNotExist) as e" and keep the existing self.message_user(request,
f'Error processing occurrence {occurrence.pk}: {str(e)}', level='error') pattern
(or add logging/traceback) so single failures still report but you avoid hiding
unexpected exceptions from update_single_occurrence and related code paths.
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 84-98: The loop issues an N+1 by calling
Taxon.objects.filter(name=label).first() for every label; instead, gather the
set of all labels used by clf.category_map.labels before iterating
classifications and fetch all matching Taxon rows once (e.g.,
Taxon.objects.filter(name__in=labels) and build a dict mapping name→Taxon), then
replace Taxon.objects.filter(name=label).first() with a lookup into that mapping
(handle missing names by logging and continuing). Update references in the
function containing clf.scores and clf.category_map.labels so taxon lookup uses
the preloaded dict.
---
Nitpick comments:
In `@ami/ml/post_processing/class_masking.py`:
- Around line 51-75: The function update_occurrences_in_collection accepts job
and params but never uses them; either remove these parameters or wire them up:
if progress reporting is intended, pass the job into
make_classifications_filtered_by_taxa_list (or use job.update_progress /
task_logger within the loop inside make_classifications_filtered_by_taxa_list)
and use params to control behavior (e.g., filter options or batch sizes) when
building classifications; otherwise remove job and params from the signature and
all call sites to avoid dead API surface. Ensure you update references to
update_occurrences_in_collection and make_classifications_filtered_by_taxa_list
so their signatures match and document the chosen behavior.
- Around line 184-186: Replace the runtime asserts that check
new_classification.detection and new_classification.detection.occurrence with
explicit checks that raise a clear exception (e.g., ValueError or RuntimeError)
so they are not stripped under python -O; specifically, in the block where
new_classification is processed (the code referencing
new_classification.detection, new_classification.detection.occurrence and
occurrences_to_update.add(...)), change the two assert lines to if-not checks
that raise including contextual information (e.g., include new_classification id
or index) before adding to occurrences_to_update.
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 13-26: The parent-chain traversal in find_ancestor_by_parent_chain
causes a DB query per current.parent access; modify the implementation to avoid
lazy FK hops by either using Taxon.parents_json or a materialized path if
available, or change the function signature to accept a prebuilt mapping (e.g.,
taxon_id → parent_id/object) and traverse that map instead of current.parent;
update callers (the nested loop that invokes find_ancestor_by_parent_chain) to
pass the preloaded map or parents_json after performing a single upfront query
that fetches all taxa and parent relationships, so ancestor lookups are O(depth)
in-memory rather than multiple DB queries.
- Around line 70-152: The transaction.atomic() currently wraps the entire loop
causing long transactions; change to process classifications in small batches:
iterate the queryset without a global atomic, accumulate pending Classification
updates (e.g., mark non-terminal) and new Classification instances (for create)
into lists (reference the Classification.objects.filter(...).update(...) and
Classification.objects.create(...) usages), and every N items (configurable
batch_size) run a short transaction.atomic() that performs bulk_update or
bulk_create for the accumulated items and clears the buffers; ensure you still
append updated occurrences to updated_occurrences and call update_progress at
the same batch boundary so behavior remains the same while avoiding one long
transaction.
In `@ami/ml/tests.py`:
- Around line 1182-1187: The test queryset using
Classification.objects.filter(detection__occurrence=occ, terminal=True,
algorithm=self.algorithm, scores__isnull=False) should mirror production by also
filtering out null logits; update the filter call in the test to include
logits__isnull=False so the query matches the one used in
update_occurrences_in_collection in class_masking.py and will catch regressions
related to missing logits.
Collaborator
Browser Testing: Class Masking & Admin Intermediate Page1. Management Command Test (end-to-end)Ran Dry-run mode ( 2. Admin Intermediate Page (replaces hardcoded action)The hardcoded
The dropdowns are populated dynamically from the database, so any new taxa lists or algorithms are automatically available. 3. Classifications After Masking (admin view)After running class masking, the Classification admin shows the new entries:
Changes in this update
|
Collaborator
Browser Testing: Class Masking in the UIManagement CommandRan class masking on real data:
Admin: Intermediate PageReplaced the hardcoded Occurrence Detail — Identifications TabTested affected occurrences (e.g., What works well:
Observations / potential improvements:
Sample Results
|
Collaborator
Browser Testing: Identification Tab After Class MaskingTested occurrence detail pagesNavigated to the occurrence detail page (Identifications tab) for occurrences affected by class masking. Occurrence 43498 — determination changed from Campaea perlata to Protoboarmia porcelaria: The Identifications tab shows three predictions in order:
Occurrence 43501 — determination stayed the same (Platynota exasperatana): Key observationsWhat works well:
Issues / improvement ideas:
Admin intermediate pageThe hardcoded
Screenshots taken locally at |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ami/main/admin.py (3)
763-782:DEFAULT_THRESHOLDSis a hidden magic constant buried inside the action.The uppercase keys now match what
RankRollupTaskexpects (the previous bug is fixed). However, definingDEFAULT_THRESHOLDSas a local variable makes the knobs invisible to operators who want to tune rollup behaviour without touching code. Promoting it to a module-level constant (or a class attribute ofSourceImageCollectionAdmin) makes it immediately discoverable.♻️ Suggested improvement
+RANK_ROLLUP_DEFAULT_THRESHOLDS = {"SPECIES": 0.8, "GENUS": 0.6, "FAMILY": 0.4} + ... `@admin.action`(description="Run Rank Rollup post-processing task (async)") def run_rank_rollup(self, request: HttpRequest, queryset: QuerySet[SourceImageCollection]) -> None: """Trigger the Rank Rollup post-processing job asynchronously.""" jobs = [] - DEFAULT_THRESHOLDS = {"SPECIES": 0.8, "GENUS": 0.6, "FAMILY": 0.4} for collection in queryset: job = Job.objects.create( ... - params={"task": "rank_rollup", "config": {"source_image_collection_id": collection.pk, "thresholds": DEFAULT_THRESHOLDS}}, + params={"task": "rank_rollup", "config": {"source_image_collection_id": collection.pk, "thresholds": RANK_ROLLUP_DEFAULT_THRESHOLDS}}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/admin.py` around lines 763 - 782, DEFAULT_THRESHOLDS is defined locally inside the run_rank_rollup admin action, making tuning invisible; move it to a discoverable location (either a module-level constant named DEFAULT_THRESHOLDS or as a class attribute on SourceImageCollectionAdmin) and update run_rank_rollup to reference that constant instead of the local variable so operators can adjust thresholds without editing the method; ensure the constant uses the same uppercase keys expected by RankRollupTask.
478-484: N+1 queries: missingselect_related("category_map")on the algorithms queryset.
Algorithm.objects.filter(category_map__isnull=False)does not prefetchcategory_map. The loop on line 483 therefore issues one additionalSELECTper algorithm to lazy-load itsAlgorithmCategoryMap. Addingselect_relatedcollapses this to a single JOIN.⚡ Proposed fix
- algorithms = Algorithm.objects.filter(category_map__isnull=False).order_by("name") + algorithms = Algorithm.objects.filter(category_map__isnull=False).select_related("category_map").order_by("name")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/admin.py` around lines 478 - 484, The algorithms queryset is causing N+1 queries because Algorithm.objects.filter(category_map__isnull=False) does not fetch the related AlgorithmCategoryMap; update the queryset used when building alg_list to use select_related("category_map") so the category_map is joined in the same query, then keep the loop that assigns alg.labels_count = len(alg.category_map.labels) if alg.category_map else 0 (using the already-fetched relation) to avoid per-algorithm lazy loads; look for the variables/functions Algorithm, category_map, algorithms, alg_list and the loop assigning labels_count to implement this change.
503-506: Ruff RUF012: mutable class-level attributes should be annotated withClassVar.Both
inlinesandactionsare plain mutable lists assigned as class attributes, which Ruff flags. The standard Django admin idiom uses plain lists, but the warning can be silenced cleanly with aClassVarannotation.🛠️ Proposed fix
+from typing import ClassVar ... # Add classifications as inline - inlines = [DetectionInline] + inlines: ClassVar[list] = [DetectionInline] - actions = [run_class_masking] + actions: ClassVar[list] = [run_class_masking]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/admin.py` around lines 503 - 506, Ruff flags the mutable class-level attributes inlines and actions; update the admin class to annotate these attributes as ClassVar to silence RUF012: add "from typing import ClassVar, List" (or appropriate typing imports) and change the annotations for inlines (e.g., inlines: ClassVar[List[type[DetectionInline]]]) and actions (e.g., actions: ClassVar[List] or ClassVar[List[callable]]/List[str] as appropriate) while keeping the assigned values (inlines = [DetectionInline], actions = [run_class_masking]).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/templates/admin/main/class_masking_confirmation.html`:
- Around line 21-30: The markup currently nests a <ul> inside a <p> in
class_masking_confirmation.html which is invalid; fix by either replacing the
surrounding <p> with a block-level container (e.g., <div>) so the intro sentence
and the list are both within valid block content, or split into a standalone <p>
for the intro sentence followed by the <ul>; update the element(s) containing
"This will:" and the following list accordingly to preserve the same text and
semantics.
---
Duplicate comments:
In `@ami/main/admin.py`:
- Around line 466-471: The current except block in the admin handler uses a bare
"except Exception as e" which can swallow programming errors; update the
exception handling around the occurrence processing (the block that calls
self.message_user with occurrence.pk) to either catch only the specific expected
exceptions (e.g., ValueError, DatabaseError, etc.) or, if you intend a broad
catch, log the full traceback before reporting to the user (use
logging.exception or capture traceback.format_exc()) and then re-raise
unexpected exceptions as needed; ensure you still call
self.message_user(request, f"Error processing occurrence {occurrence.pk}: {e}",
level="error") after logging the detailed error.
---
Nitpick comments:
In `@ami/main/admin.py`:
- Around line 763-782: DEFAULT_THRESHOLDS is defined locally inside the
run_rank_rollup admin action, making tuning invisible; move it to a discoverable
location (either a module-level constant named DEFAULT_THRESHOLDS or as a class
attribute on SourceImageCollectionAdmin) and update run_rank_rollup to reference
that constant instead of the local variable so operators can adjust thresholds
without editing the method; ensure the constant uses the same uppercase keys
expected by RankRollupTask.
- Around line 478-484: The algorithms queryset is causing N+1 queries because
Algorithm.objects.filter(category_map__isnull=False) does not fetch the related
AlgorithmCategoryMap; update the queryset used when building alg_list to use
select_related("category_map") so the category_map is joined in the same query,
then keep the loop that assigns alg.labels_count = len(alg.category_map.labels)
if alg.category_map else 0 (using the already-fetched relation) to avoid
per-algorithm lazy loads; look for the variables/functions Algorithm,
category_map, algorithms, alg_list and the loop assigning labels_count to
implement this change.
- Around line 503-506: Ruff flags the mutable class-level attributes inlines and
actions; update the admin class to annotate these attributes as ClassVar to
silence RUF012: add "from typing import ClassVar, List" (or appropriate typing
imports) and change the annotations for inlines (e.g., inlines:
ClassVar[List[type[DetectionInline]]]) and actions (e.g., actions:
ClassVar[List] or ClassVar[List[callable]]/List[str] as appropriate) while
keeping the assigned values (inlines = [DetectionInline], actions =
[run_class_masking]).
mihow
force-pushed
the
feat/postprocessing-class-masking
branch
from
f9ca78f to
fc3f9e1
Compare
Collaborator
Proposed
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
ami/ml/post_processing/class_masking.py (3)
233-235:logging.exceptionalready includes the exception message — the{e}interpolation is redundant.
self.logger.exception(...)automatically appends the current exception info. Interpolating{e}duplicates it.♻️ Proposed fix
- self.logger.exception(f"Failed to load objects: {e}") + self.logger.exception("Failed to load objects")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` around lines 233 - 235, The log call in the exception handler redundantly interpolates the exception because self.logger.exception already logs the exception info; in the except block that currently does self.logger.exception(f"Failed to load objects: {e}") simply remove the "{e}" interpolation and call self.logger.exception("Failed to load objects") so the message and full traceback are logged once (update the except block where self.logger.exception is used for loading objects).
129-129: Downgrade per-iteration sum logs from INFO to DEBUG.These execute once per classification (potentially thousands) and will flood logs at INFO level. Use
logger.debugfor both.♻️ Proposed fix
- logger.info(f"Previous totals: {sum(scores)} scores, {sum(logits)} logits") + logger.debug(f"Previous totals: {sum(scores)} scores, {sum(logits)} logits")- logger.info(f"New totals: {sum(scores)} scores, {sum(logits)} logits") + logger.debug(f"New totals: {sum(scores)} scores, {sum(logits)} logits")Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` at line 129, Change the two per-iteration log calls that currently use logger.info to logger.debug so they don't flood INFO logs: replace the logger.info(f"Previous totals: {sum(scores)} scores, {sum(logits)} logits") call and the other similar logger.info call (at the second occurrence around line 145) with logger.debug using the same message formatting; keep the sums computation and formatting identical, only change the logging level.
51-58:jobparameter is accepted but never used.The
jobargument is passed at the call-site (line 248 ofrun()) but never referenced inupdate_occurrences_in_collection. Either forward it tomake_classifications_filtered_by_taxa_listif needed for progress reporting, or remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/class_masking.py` around lines 51 - 58, The job parameter on update_occurrences_in_collection is accepted but unused; either remove it entirely or forward it into the downstream call that likely needs it — update_occurrences_in_collection(collection, taxa_list, algorithm, params, new_algorithm, task_logger=logger, job=None) should pass job through to make_classifications_filtered_by_taxa_list (or whatever internal call performs progress reporting) so the progress reporter receives the same job object; locate the call to make_classifications_filtered_by_taxa_list inside update_occurrences_in_collection and add job as an argument (and update that callee signature if necessary), or if job is genuinely unused, remove the job parameter from update_occurrences_in_collection and from the run() call site to keep signatures consistent.ami/ml/tests.py (2)
919-940: Rename unused loop variableito_i.Ruff B007:
iis never referenced in the loop body.♻️ Proposed fix
- for i in range(num): + for _i in range(num):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/tests.py` around lines 919 - 940, The loop in _create_occurrences_with_classifications uses an unused variable named i which triggers Ruff B007; rename i to _i in the for loop header (for _i in range(num):) to indicate the variable is intentionally unused, leaving the loop body (Detection.objects.create, Occurrence.objects.create, Classification.objects.create and occurrences.append) unchanged.
1034-1034: Moveimport mathto the top of the file.
import mathappears inside four separate test methods (lines 1034, 1144, 1217, 1266). Python caches module imports, so there's no runtime cost, but it's non-idiomatic. A singleimport mathat the top of the file is cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/tests.py` at line 1034, Move the duplicate "import math" statements out of the individual test functions and add a single "import math" at the top-level imports of the test module; remove the four in-function imports so all tests use the top-level math import (search for the in-function "import math" occurrences to update them). Ensure no other name conflicts occur and run tests to confirm no behavioral changes.ami/ml/post_processing/rank_rollup.py (3)
37-38: Mutable class-level defaults should be annotated withClassVar.♻️ Proposed fix
+ from typing import ClassVar + - DEFAULT_THRESHOLDS = {"SPECIES": 0.8, "GENUS": 0.6, "FAMILY": 0.4} - ROLLUP_ORDER = ["SPECIES", "GENUS", "FAMILY"] + DEFAULT_THRESHOLDS: ClassVar[dict[str, float]] = {"SPECIES": 0.8, "GENUS": 0.6, "FAMILY": 0.4} + ROLLUP_ORDER: ClassVar[list[str]] = ["SPECIES", "GENUS", "FAMILY"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/rank_rollup.py` around lines 37 - 38, DEFAULT_THRESHOLDS and ROLLUP_ORDER are mutable class-level defaults and should be declared as ClassVar to avoid accidental per-instance mutation; update the class that defines DEFAULT_THRESHOLDS and ROLLUP_ORDER to annotate them like DEFAULT_THRESHOLDS: ClassVar[Dict[str, float]] and ROLLUP_ORDER: ClassVar[List[str]] (or the appropriate typing aliases), add the necessary "from typing import ClassVar, Dict, List" import if missing, and keep the existing constant values unchanged.
116-118:t.rank == rankshould normalize case to matchfind_ancestor_by_parent_chain.
find_ancestor_by_parent_chaindefensively calls.upper()oncurrent.rankbefore comparing (line 22). The candidates filter uses a direct equality check, creating a silent failure if anyTaxon.rankvalue is ever lowercase — rollup candidates would always be empty.♻️ Proposed fix
- candidates = {t: s for t, s in taxon_scores.items() if t.rank == rank} + candidates = {t: s for t, s in taxon_scores.items() if t.rank.upper() == rank}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/rank_rollup.py` around lines 116 - 118, The candidates filter silently fails on case mismatch because it compares Taxon.rank directly; update the comprehension that builds candidates (using rollup_order, thresholds, taxon_scores) to normalize case before comparing (e.g., compare t.rank.upper() to rank or normalize both sides) so it matches the defensive behavior used in find_ancestor_by_parent_chain; ensure you only change the equality check in the comprehension that constructs candidates = {t: s for t, s in taxon_scores.items() if ... } to use the normalized rank comparison.
83-157: Multiple N+1 FK accesses insideqs.iterator()—select_relatedis not effective here.
clf.category_map(lines 90, 97),clf.detection(lines 138–140), andclf.detection.occurrence(line 149) each trigger a separate DB query per iteration when usingiterator(), sinceselect_related()is not compatible withiterator()in older Django.Suggested mitigations:
- Category map: Build a
{pk: AlgorithmCategoryMap}cache from the already-computedcat_map_ids(see previous comment) and look up byclf.category_map_id.- Detection FK: Use
clf.detection_id(the raw FK column, no DB hit) in thefilter(detection_id=...)andcreate(detection_id=...)calls.- Occurrence: Pre-fetch occurrence PKs or consolidate occurrence updates after the loop.
♻️ Illustrative patch for category_map cache and detection_id usage
+ category_map_cache = {cm.pk: cm for cm in AlgorithmCategoryMap.objects.filter(pk__in=cat_map_ids)} with transaction.atomic(): for i, clf in enumerate(qs.iterator(), start=1): ... - if not clf.category_map: + cm = category_map_cache.get(clf.category_map_id) + if not cm: self.logger.info(f"Skipping classification #{clf.pk}: no category_map assigned") continue ... for idx, score in enumerate(clf.scores): - label = clf.category_map.labels[idx] + label = cm.labels[idx] ... ... - Classification.objects.filter(detection=clf.detection).update(terminal=False) + Classification.objects.filter(detection_id=clf.detection_id).update(terminal=False) Classification.objects.create( - detection=clf.detection, + detection_id=clf.detection_id, ... )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/post_processing/rank_rollup.py` around lines 83 - 157, The loop over qs.iterator() causes N+1 DB hits for clf.category_map, clf.detection and clf.detection.occurrence because select_related() is ineffective with iterator(); fix by 1) building an in-memory cache of AlgorithmCategoryMap objects keyed by pk from the precomputed cat_map_ids and replace accesses to clf.category_map with a lookup using clf.category_map_id, 2) avoid resolving the Detection FK by using clf.detection_id in DB calls (replace Classification.objects.filter(detection=clf.detection).update(...) with filter(detection_id=clf.detection_id) and pass detection_id=clf.detection_id to Classification.objects.create), and 3) stop accessing clf.detection.occurrence inside the loop — instead collect detection_ids (or classification ids) during iteration and after the loop bulk-fetch occurrences (or occurrence pks) and append to updated_occurrences or perform consolidated updates to avoid per-row queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/ml/post_processing/class_masking.py`:
- Around line 162-174: The new Classification created in class_masking.py is
missing the applied_to field, losing post-processing lineage; update the
new_classification instantiation to include applied_to referencing the original
classification (e.g., applied_to=classification) similar to how RankRollupTask
sets applied_to=clf so the created classification preserves its source lineage.
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 68-78: qs.only("category_map") still triggers N+1 because
accessing clf.category_map performs a deferred fetch per row; instead, collect
distinct category_map_id from the queryset (use
qs.values_list("category_map_id", flat=True).distinct()), fetch all
AlgorithmCategoryMap objects for those ids in one query, then iterate over the
pre-fetched maps to gather labels (avoid accessing clf.category_map directly).
Populate all_labels from the cached AlgorithmCategoryMap instances, build
label_to_taxon as before, and log using the same self.logger.info call;
reference qs.only("category_map"), clf.category_map, and AlgorithmCategoryMap to
locate and replace the logic.
---
Nitpick comments:
In `@ami/ml/post_processing/class_masking.py`:
- Around line 233-235: The log call in the exception handler redundantly
interpolates the exception because self.logger.exception already logs the
exception info; in the except block that currently does
self.logger.exception(f"Failed to load objects: {e}") simply remove the "{e}"
interpolation and call self.logger.exception("Failed to load objects") so the
message and full traceback are logged once (update the except block where
self.logger.exception is used for loading objects).
- Line 129: Change the two per-iteration log calls that currently use
logger.info to logger.debug so they don't flood INFO logs: replace the
logger.info(f"Previous totals: {sum(scores)} scores, {sum(logits)} logits") call
and the other similar logger.info call (at the second occurrence around line
145) with logger.debug using the same message formatting; keep the sums
computation and formatting identical, only change the logging level.
- Around line 51-58: The job parameter on update_occurrences_in_collection is
accepted but unused; either remove it entirely or forward it into the downstream
call that likely needs it — update_occurrences_in_collection(collection,
taxa_list, algorithm, params, new_algorithm, task_logger=logger, job=None)
should pass job through to make_classifications_filtered_by_taxa_list (or
whatever internal call performs progress reporting) so the progress reporter
receives the same job object; locate the call to
make_classifications_filtered_by_taxa_list inside
update_occurrences_in_collection and add job as an argument (and update that
callee signature if necessary), or if job is genuinely unused, remove the job
parameter from update_occurrences_in_collection and from the run() call site to
keep signatures consistent.
In `@ami/ml/post_processing/rank_rollup.py`:
- Around line 37-38: DEFAULT_THRESHOLDS and ROLLUP_ORDER are mutable class-level
defaults and should be declared as ClassVar to avoid accidental per-instance
mutation; update the class that defines DEFAULT_THRESHOLDS and ROLLUP_ORDER to
annotate them like DEFAULT_THRESHOLDS: ClassVar[Dict[str, float]] and
ROLLUP_ORDER: ClassVar[List[str]] (or the appropriate typing aliases), add the
necessary "from typing import ClassVar, Dict, List" import if missing, and keep
the existing constant values unchanged.
- Around line 116-118: The candidates filter silently fails on case mismatch
because it compares Taxon.rank directly; update the comprehension that builds
candidates (using rollup_order, thresholds, taxon_scores) to normalize case
before comparing (e.g., compare t.rank.upper() to rank or normalize both sides)
so it matches the defensive behavior used in find_ancestor_by_parent_chain;
ensure you only change the equality check in the comprehension that constructs
candidates = {t: s for t, s in taxon_scores.items() if ... } to use the
normalized rank comparison.
- Around line 83-157: The loop over qs.iterator() causes N+1 DB hits for
clf.category_map, clf.detection and clf.detection.occurrence because
select_related() is ineffective with iterator(); fix by 1) building an in-memory
cache of AlgorithmCategoryMap objects keyed by pk from the precomputed
cat_map_ids and replace accesses to clf.category_map with a lookup using
clf.category_map_id, 2) avoid resolving the Detection FK by using
clf.detection_id in DB calls (replace
Classification.objects.filter(detection=clf.detection).update(...) with
filter(detection_id=clf.detection_id) and pass detection_id=clf.detection_id to
Classification.objects.create), and 3) stop accessing clf.detection.occurrence
inside the loop — instead collect detection_ids (or classification ids) during
iteration and after the loop bulk-fetch occurrences (or occurrence pks) and
append to updated_occurrences or perform consolidated updates to avoid per-row
queries.
In `@ami/ml/tests.py`:
- Around line 919-940: The loop in _create_occurrences_with_classifications uses
an unused variable named i which triggers Ruff B007; rename i to _i in the for
loop header (for _i in range(num):) to indicate the variable is intentionally
unused, leaving the loop body (Detection.objects.create,
Occurrence.objects.create, Classification.objects.create and occurrences.append)
unchanged.
- Line 1034: Move the duplicate "import math" statements out of the individual
test functions and add a single "import math" at the top-level imports of the
test module; remove the four in-function imports so all tests use the top-level
math import (search for the in-function "import math" occurrences to update
them). Ensure no other name conflicts occur and run tests to confirm no
behavioral changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ami/main/api/serializers.py (1)
877-884:ClassificationAppliedToSerializerembeds the fullAlgorithmSerializerdespite being described as "lightweight"
AlgorithmSerializerexposes 13+ fields including nestedcategory_map(itself a nested serializer),uri,version_name,task_type,description, etc. For the stated purpose of showing "Derived from [algorithm]" lineage, a minimal inline would be more appropriate and avoids pulling unnecessary data into every nested classification response.♻️ Suggested minimal algorithm representation
+class MinimalAlgorithmSerializer(serializers.ModelSerializer): + class Meta: + model = Algorithm + fields = ["id", "name", "key", "version"] + class ClassificationAppliedToSerializer(serializers.ModelSerializer): """Lightweight nested representation of the parent classification this was derived from.""" - algorithm = AlgorithmSerializer(read_only=True) + algorithm = MinimalAlgorithmSerializer(read_only=True) class Meta: model = Classification fields = ["id", "created_at", "algorithm"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/api/serializers.py` around lines 877 - 884, ClassificationAppliedToSerializer currently embeds the full AlgorithmSerializer (too heavy); create a lightweight nested serializer (e.g., MinimalAlgorithmSerializer) with only the minimal fields needed for lineage (such as id and a human label like name or version_name and optionally uri) and replace algorithm = AlgorithmSerializer(read_only=True) with algorithm = MinimalAlgorithmSerializer(read_only=True) inside the same module; ensure the new MinimalAlgorithmSerializer references the Algorithm model and is listed before ClassificationAppliedToSerializer so imports/resolution succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/main/api/serializers.py`:
- Around line 877-884: This is a RUF012 false positive for the mutable default
class attribute on the Meta.fields list in ClassificationAppliedToSerializer; if
CI enforces RUF012, suppress it by adding a local noqa comment on the
Meta.fields line (i.e., add "# noqa: RUF012" to the fields = ["id",
"created_at", "algorithm"] line in the ClassificationAppliedToSerializer.Meta
block) so the linter ignores this diff-scoped artifact.
In `@ami/main/api/views.py`:
- Around line 1228-1238: OccurrenceListSerializer.get_determination_details() is
triggering N+1 queries by accessing Detection.best_prediction (a cached_property
that calls Detection.predictions().order_by(...).first()) without eager-loading
applied_to and applied_to.algorithm; fix by ensuring the queryset used when
building detections for list view (or inside Detection.predictions()) prefetches
applied_to__algorithm for Classification objects, or alternatively adjust
OccurrenceListSerializer to avoid serializing applied_to in list action (e.g.,
use a lightweight nested serializer for list views); target symbols:
OccurrenceListSerializer.get_determination_details, Detection.best_prediction,
Detection.predictions, and ClassificationNestedSerializer (and the
applied_to__algorithm prefetch) so the top prediction’s applied_to and its
algorithm are loaded eagerly.
---
Nitpick comments:
In `@ami/main/api/serializers.py`:
- Around line 877-884: ClassificationAppliedToSerializer currently embeds the
full AlgorithmSerializer (too heavy); create a lightweight nested serializer
(e.g., MinimalAlgorithmSerializer) with only the minimal fields needed for
lineage (such as id and a human label like name or version_name and optionally
uri) and replace algorithm = AlgorithmSerializer(read_only=True) with algorithm
= MinimalAlgorithmSerializer(read_only=True) inside the same module; ensure the
new MinimalAlgorithmSerializer references the Algorithm model and is listed
before ClassificationAppliedToSerializer so imports/resolution succeed.
Member
|
Here is a summary of the plan we discussed @mihow, let me know if I remember it wrong :)
|
Collaborator
|
Claude says: Non-blocking follow-up note from the tracking-revival branch ( The Benefits:
Not a blocker for this PR — the pattern lands here, and class masking can fold in later. |
6 tasks
mihow
added a commit
that referenced
this pull request
May 1, 2026
Adds the shared infrastructure that PRs #999 (class masking) and #1272 (tracking) each grew independently. Uses SmallSizeFilterTask as the migration consumer to prove the pattern without carving up another open PR. Changes: - BasePostProcessingTask gains required pydantic config_schema class attr; validates Job.params['config'] at task __init__ via self.config_schema(**config). self.config is now a BaseModel instance (was: freeform dict). - SmallSizeFilterTask: adds SmallSizeFilterConfig schema (size_threshold + source_image_collection_id, validators for 0<x<1, extra=forbid). Existing default 0.0008 preserved. - New admin form base BasePostProcessingActionForm with to_config() contract, plus SmallSizeFilterActionForm exposing size_threshold knob (currently unreachable — admin trigger hardcoded empty config). - Parameterized confirmation template at admin/post_processing/confirmation.html with overridable {% block intro %} and a _form_fieldset.html partial. - run_small_size_filter admin action rewrites onto new pattern: intermediate confirmation page on first POST, validate + enqueue on confirm. Per-collection Job creation preserved (each Job gets correct project FK). - 17 tests covering schema contract, form validation, intermediate page render, multi-collection partitioning by project FK. Pydantic v1 syntax throughout (repo pins pydantic<2.0). Memory note: container is v1, .dict() / .__fields__ are correct here. Out of scope (explicitly deferred): - Project-partitioning helper (defer to whichever multi-scope adopter lands first, likely #1272's tracking which partitions events across projects) - REST API trigger surface (eventual primary; admin not future-primary) - Rank rollup, class masking, tracking tasks themselves — stay in their PRs Design doc: docs/claude/planning/2026-05-01-post-processing-admin-scaffolding-design.md Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 5, 2026
Address review feedback on the repetitive run_/render pattern. The confirm -> render -> validate -> enqueue flow is now built by make_post_processing_action() in ami/ml/post_processing/admin/actions.py, so each task declares only what varies: its task class, knob form, and how a selected row maps to a Job (scope_resolver / project_resolver / name_resolver). Tasks whose row->Job mapping isn't one-Job-per-row (e.g. PR #1272's per-project event partitioning) pass a custom build_jobs callable; PR #999 likewise inherits the form/template instead of hand-rolling HTML. Config validation now has a single source of truth: the task's pydantic config_schema. The knob form no longer re-encodes the (0, 1) bound, and the FloatField min/max that contradicted the exclusive interval is gone. Schema errors raised while building Jobs are mapped back onto the form so the operator sees them inline on the confirmation page (non-field errors now render too). This also removes the near-dead per-row try/except in the old admin action. Adds test_action_factory.py (action registration, default builder, schema mapping, all-or-nothing, build_jobs override hook) and updates the form tests to reflect that range enforcement moved to the schema layer. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
|
Claude says: (rebase guidance drafted on @mihow's behalf) Rebase path onto #1289 (post-processing admin scaffolding)#1289 landed a shared admin-trigger pattern for post-processing tasks (head 1. Schemas. Add 2. Form. Replace the hand-rolled 3. Admin action. Replace the hand-written action in run_class_masking = make_post_processing_action(
ClassMaskingTask,
ClassMaskingActionForm,
scope_resolver=lambda obj: {"<scope_field>": obj.pk},
)If a row maps to more than one Job (or needs grouping), pass a custom 4. Template. 5. Expected conflicts (small): The rank-rollup task is unaffected beyond adding its |
mihow
added a commit
that referenced
this pull request
Jun 8, 2026
Make post-processing filters easier to develop against and observe. Per-occurrence trigger (the dev/spot path): SmallSizeFilterConfig now accepts either source_image_collection_id or occurrence_id (discriminated scope, exactly-one enforced by a pydantic root_validator), and the task resolves its detection queryset from whichever scope is set. OccurrenceAdmin gains a run_small_size_filter action via the same make_post_processing_action factory, so an operator can run a filter on a single selected occurrence without standing up a whole capture set. This discriminated-scope shape is the pattern future per-occurrence / per-event tasks (#999, #1272) copy. Job stage metrics (run observability): BasePostProcessingTask.report_stage_metrics writes named counters onto the job's post_processing stage (falling back to a log line when run without a job). The small size filter emits detections_checked, detections_flagged and occurrences_updated at each flush and on completion, so the Jobs admin page shows what a run examined and changed. Tests: schema scope cases (occurrence-only valid, both-scopes raise, no-scope raises); the OccurrenceAdmin action enqueues a job scoped to occurrence_id; run-time occurrence-scope isolation (sibling occurrences untouched); and stage-metric reporting on a real job. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added
the
post-processing
mihow
added a commit
that referenced
this pull request
Jun 25, 2026
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 25, 2026
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 25, 2026
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 25, 2026
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 26, 2026
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 26, 2026
…1289) * docs(post-processing): design spec for admin scaffolding precursor PR Context: PRs #999 (class masking) and #1272 (tracking) each grew their own admin-trigger plumbing (registry registration, intermediate confirmation page with form, freeform dict config). This precursor extracts the shared pattern using SmallSizeFilterTask as the migration consumer — proves the abstraction without carving up another contributor's open PR. Spec covers: pydantic config_schema contract on BasePostProcessingTask, BasePostProcessingActionForm class, parameterized confirmation template + partial, SmallSizeFilterTask migration (exposes existing size_threshold knob via admin), test plan, rebase impact for #999 + #1272. Co-Authored-By: Claude <noreply@anthropic.com> * feat(post-processing): admin scaffolding precursor Adds the shared infrastructure that PRs #999 (class masking) and #1272 (tracking) each grew independently. Uses SmallSizeFilterTask as the migration consumer to prove the pattern without carving up another open PR. Changes: - BasePostProcessingTask gains required pydantic config_schema class attr; validates Job.params['config'] at task __init__ via self.config_schema(**config). self.config is now a BaseModel instance (was: freeform dict). - SmallSizeFilterTask: adds SmallSizeFilterConfig schema (size_threshold + source_image_collection_id, validators for 0<x<1, extra=forbid). Existing default 0.0008 preserved. - New admin form base BasePostProcessingActionForm with to_config() contract, plus SmallSizeFilterActionForm exposing size_threshold knob (currently unreachable — admin trigger hardcoded empty config). - Parameterized confirmation template at admin/post_processing/confirmation.html with overridable {% block intro %} and a _form_fieldset.html partial. - run_small_size_filter admin action rewrites onto new pattern: intermediate confirmation page on first POST, validate + enqueue on confirm. Per-collection Job creation preserved (each Job gets correct project FK). - 17 tests covering schema contract, form validation, intermediate page render, multi-collection partitioning by project FK. Pydantic v1 syntax throughout (repo pins pydantic<2.0). Memory note: container is v1, .dict() / .__fields__ are correct here. Out of scope (explicitly deferred): - Project-partitioning helper (defer to whichever multi-scope adopter lands first, likely #1272's tracking which partitions events across projects) - REST API trigger surface (eventual primary; admin not future-primary) - Rank rollup, class masking, tracking tasks themselves — stay in their PRs Design doc: docs/claude/planning/2026-05-01-post-processing-admin-scaffolding-design.md Co-Authored-By: Claude <noreply@anthropic.com> * docs(post-processing): add PR #1289 admin smoke screenshots Confirmation page (intermediate page rendering with size_threshold form field) and success message (after submitting form, Job 1571 enqueued with typed config payload). Co-Authored-By: Claude <noreply@anthropic.com> * chore: host PR screenshots on S3 instead of committing to repo Screenshots for the PR description now live in the ami-media-staging object store rather than docs/claude/screenshots/. Keeps binary assets out of the repo history. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(post-processing): extract shared admin-action factory Address review feedback on the repetitive run_/render pattern. The confirm -> render -> validate -> enqueue flow is now built by make_post_processing_action() in ami/ml/post_processing/admin/actions.py, so each task declares only what varies: its task class, knob form, and how a selected row maps to a Job (scope_resolver / project_resolver / name_resolver). Tasks whose row->Job mapping isn't one-Job-per-row (e.g. PR #1272's per-project event partitioning) pass a custom build_jobs callable; PR #999 likewise inherits the form/template instead of hand-rolling HTML. Config validation now has a single source of truth: the task's pydantic config_schema. The knob form no longer re-encodes the (0, 1) bound, and the FloatField min/max that contradicted the exclusive interval is gone. Schema errors raised while building Jobs are mapped back onto the form so the operator sees them inline on the confirmation page (non-field errors now render too). This also removes the near-dead per-row try/except in the old admin action. Adds test_action_factory.py (action registration, default builder, schema mapping, all-or-nothing, build_jobs override hook) and updates the form tests to reflect that range enforcement moved to the schema layer. Co-Authored-By: Claude <noreply@anthropic.com> * fix(post-processing): refuse "select all across pages" in admin trigger Addresses review feedback. When an operator uses Django admin's "select all across pages", the action receives the entire filtered table as its queryset, which would serialize every pk into hidden _selected_action inputs on the confirmation page (a very large POST body, potentially over request limits). This trigger is for explicit, bounded selections, so the across-pages case is now refused with a clear message instead of rendering an unbounded form. Also folds in two cosmetic review nits: replace the Unicode multiplication sign in the size-threshold help text with a plain "x", and add a language tag to the module-layout code fence in the design doc. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(post-processing): address re-review on the action factory Follow-up to the head-commit CodeRabbit re-review: - Wrap the Job-creation loop in default_build_jobs in transaction.atomic(). Admin requests are already atomic (ATOMIC_REQUESTS=True), but this helper can also be called outside a request (e.g. a management command), so the explicit block keeps creation all-or-nothing there too. Job.enqueue() uses transaction.on_commit, so the (millisecond) block only guards row creation; the long-running task still runs asynchronously in the worker after commit. - Guard BasePostProcessingTask.__init_subclass__ with inspect.isabstract so an abstract intermediary task class isn't forced to declare key/name/config_schema. - Resolve the selection once in render_confirmation (len of the materialized pk list) instead of a separate .count() query. - Omit scope_resolver from the build_jobs kwargs when it isn't set, so a custom build_jobs supplied without one never receives a None it might call. - Derive the admin action's labels (title, submit, dropdown description) from task.name instead of hardcoding title-case strings, so the operator-facing label and the Job name stay consistent and there are fewer per-task strings. - Make the build_jobs override-hook test actually invoke the action and assert the custom builder ran (and that scope_resolver was not forwarded as None). 24/24 post_processing tests green. Co-Authored-By: Claude <noreply@anthropic.com> * test(post-processing): prune redundant tests, cover atomicity and abstract guard End-of-PR test audit (separate from the TDD scaffolding): - Remove test_default_threshold_applies_when_form_uses_initial: size_threshold is a required field and can't be omitted, so it just re-POSTed the default value — identical mechanism to the existing valid-post test. The default is already covered by the form-initial and schema-default unit tests. - Rewrite the mislabeled "all-or-nothing" test into a real one. The old version gave every row the same out-of-range threshold, so both failed at validation and the creation loop was never reached. The new test passes validation, forces a failure while creating the second Job, and asserts the first is rolled back — actually exercising the transaction.atomic() wrap. - Add coverage for the inspect.isabstract guard in __init_subclass__: an abstract intermediary may defer key/name/config_schema to its concrete subclasses. 24 post_processing tests green. Co-Authored-By: Claude <noreply@anthropic.com> * feat(post-processing): per-occurrence trigger and job stage metrics Make post-processing filters easier to develop against and observe. Per-occurrence trigger (the dev/spot path): SmallSizeFilterConfig now accepts either source_image_collection_id or occurrence_id (discriminated scope, exactly-one enforced by a pydantic root_validator), and the task resolves its detection queryset from whichever scope is set. OccurrenceAdmin gains a run_small_size_filter action via the same make_post_processing_action factory, so an operator can run a filter on a single selected occurrence without standing up a whole capture set. This discriminated-scope shape is the pattern future per-occurrence / per-event tasks (#999, #1272) copy. Job stage metrics (run observability): BasePostProcessingTask.report_stage_metrics writes named counters onto the job's post_processing stage (falling back to a log line when run without a job). The small size filter emits detections_checked, detections_flagged and occurrences_updated at each flush and on completion, so the Jobs admin page shows what a run examined and changed. Tests: schema scope cases (occurrence-only valid, both-scopes raise, no-scope raises); the OccurrenceAdmin action enqueues a job scoped to occurrence_id; run-time occurrence-scope isolation (sibling occurrences untouched); and stage-metric reporting on a real job. Co-Authored-By: Claude <noreply@anthropic.com> * fix(post-processing): dedup occurrences_updated across flush batches The metric incremented from a batch-local set that is cleared each flush, so an occurrence whose detections span more than one batch was counted once per batch. Track occurrence ids in a persistent set and report its size instead. Detections never recur across batches, so detections_flagged was already correct. Addresses CodeRabbit review on beacb3f. Co-Authored-By: Claude <noreply@anthropic.com> * test: cut post-processing test fixture cost Replace per-test setup_test_project calls with minimal Project/Collection/ Occurrence rows and class-level setUpTestData in the admin-action and factory tests; the admin flow only reads FKs, so the full fixture (storage source, deployment, processing service per call) was wasted cost. Convert the pure form tests to SimpleTestCase (no DB) and drop one redundant valid-path form test already covered end to end by the admin and factory flows. TestPostProcessingTasks now builds its images/events/collection once per class instead of once per test. Module wall-clock roughly halves locally (8.8s -> 4.5s warm), and the double setup_test_project in the multi-collection admin test that intermittently hit a statement timeout in CI is gone. Co-Authored-By: Claude <noreply@anthropic.com> * fix(post-processing): render admin field errors as errorlist, clarify select_across refusal The post-processing confirmation form rendered each field's validation errors with the page-level `errornote` banner style. Switch the form-fieldset partial to Django's native `{{ field.errors }}` and `{{ form.non_field_errors }}` so errors render inline as `errorlist`, matching Django admin conventions. The invalid-threshold test now asserts the inline `errorlist` class. Also reframe the "select all across pages" comment: the reason to refuse it is that the action would apply to the entire filtered table rather than the rows the operator explicitly selected. The oversized POST body is a secondary symptom, not the primary concern. Co-Authored-By: Claude <noreply@anthropic.com> * fix(post-processing): count only occurrences whose determination changed The small size filter reported `occurrences_updated` as the number of occurrences it re-saved, but re-saving recomputes the determination in place. An occurrence pinned to a human identification keeps its taxon when its detection is flagged "Not identifiable", so it was counted even though nothing about it actually changed. Capture each occurrence's determination before the save and count it only when the determination changes after. Add a regression test pairing an occurrence that carries a human identification (determination unchanged, not counted) with an un-identified one (determination flips, counted). Co-Authored-By: Claude <noreply@anthropic.com> * feat(post-processing): link the admin action result to the created job(s) After an operator runs a post-processing task from the admin, the success message now links each created Job to its admin change page instead of just printing the raw pks. From there the operator can follow the job's progress and read any failure reason, which is otherwise only visible in the worker log. The link points at the admin change page because that page is always reachable from this admin action; the public UI host is not reliably known in this context. The message is built with format_html so the links render as anchors. Co-Authored-By: Claude <noreply@anthropic.com> * perf(admin): speed up the occurrence changelist and add id search The occurrence admin changelist is the entry point for the per-occurrence post-processing trigger, but on a large table (~1.3M occurrences) it took ~15s to load a single page. Two causes, both fixed here: - get_queryset counted detections with a JOIN + GROUP BY. A grouped count must aggregate the whole occurrence x detection join before the changelist's ORDER BY ... LIMIT can take a page, so it scanned every row just to show 25. Replaced with a correlated subquery (Coalesced to 0 for occurrences with no detections, matching the old JOIN count), which runs only for the rows on the page. Measured 14.9s -> 0.04s for one page on the large table. - The list ordered by -created_at, which has no index, forcing a full sort of the table to find the newest page. Switched to -id, the indexed primary key, which increases with insertion time so newest-first is preserved. Also let an all-digit search term act as an exact occurrence-id lookup, so an operator can jump straight to an occurrence by id; non-numeric terms still use the determination-name search. Co-Authored-By: Claude <noreply@anthropic.com> * perf(admin): speed up the detection changelist and link detections from occurrences The detection admin changelist counted classifications with Count() + GROUP BY and ordered by an unindexed -created_at. On a large table (~1.4M detections, ~2.2M classifications) the grouped aggregate runs over the whole detection x classification join before the page LIMIT applies — slow enough to exhaust work_mem and error out. Replaced with a correlated subquery (Coalesced to 0) and -id ordering, mirroring the occurrence changelist fix. Also set show_change_link on the detection inline of the occurrence page, so an operator can open a detection's change page — where the classifications inline shows which algorithms were applied, including post-processing — without searching for its id. Co-Authored-By: Claude <noreply@anthropic.com> * feat(admin): search detections by id in the admin Add a search box to the detection changelist: an all-digit term is an exact id lookup (jump straight to a detection by id), other terms search the source image path. Mirrors the occurrence changelist search. Co-Authored-By: Claude <noreply@anthropic.com> * feat(admin): speed up the classification admin and recompute determination More admin niceties for reviewing post-processing results on a large database: - Classification change page: FK fields (taxon, detection, algorithm, category_map, applied_to) render as autocomplete widgets instead of <select>s preloaded with every row, which made the page unusable. - Classification changelist: count the scores/logits arrays in SQL (cardinality) and defer the arrays, so the list no longer transfers thousands of floats per row; order by -id; add an id / taxon-name search box. - Link the detection page's classification inline rows to their change page. - New OccurrenceAdmin action "Recompute determination": editing classifications by hand doesn't refresh the occurrence determination (only Occurrence and Identification saves do), so this re-derives it for the selected occurrences. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(admin): share id search in a mixin, skip the full changelist count, guard huge ids Three review follow-ups on the admin niceties added for reviewing post-processing: - Extract the duplicated digit-term id search from the Occurrence, Detection, and Classification admins into a shared IdSearchAdminMixin. The three copies were identical; the mixin is now the single place that behavior lives. - Guard an out-of-range numeric term. A digit string larger than the bigint primary key raised a database DataError (500) before; it now returns no results. - Set show_full_result_count = False on the three large-table admins, so the changelist no longer runs a second, unfiltered COUNT(*) over the whole table for its footer total -- as expensive as the page query itself on millions of rows. Tests: add the out-of-range id case. The shared mixin is now tested once in OccurrenceAdminChangelistTest, so the redundant per-admin copy on the Detection admin is removed (its docstring points to the canonical test). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
mihow
added a commit
that referenced
this pull request
Jun 26, 2026
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
feat/postprocessing-class-masking
branch
from
daed538 to
6e8eac8
Compare
…dmin framework Rework of #999 onto the #1289 post-processing framework. The branch was cut from an old main with its own hand-rolled admin action; ClassMaskingTask and RankRollupTask now subclass BasePostProcessingTask with pydantic config schemas and are triggered through make_post_processing_action (collection scope on SourceImageCollection, single-occurrence scope on Occurrence for class masking). Correctness fixes from the review threads: - Class masking selects the top class from an -inf-masked softmax, so a class excluded by the taxa list can never win even when it had the highest logit; raises when the taxa list excludes every class in the category map. Stored logits stay raw (JSON-safe) and the mask is captured in scores (excluded -> 0). - The masked-output Algorithm is one per (source algorithm, taxa list) and its category map is persisted (previously set in memory only, so masked classifications referenced a null map). - applied_to is populated on new masked classifications (the provenance the API exposes was left blank). - Rank rollup preloads category-map labels in two queries and select_relates the per-row relations instead of dereferencing category_map per classification. Surfaces provenance in the API: applied_to is added to the Classification serializers, and applied_to__algorithm is prefetched in the occurrence list/detail prefetch and the classification viewset to avoid an N+1 on render. Tests: pydantic config validation, admin trigger for both scopes, the masking maths (including the excluded-class guarantee and the all-excluded error), ClassMaskingTask.run() end to end for both scopes, and rank rollup. 20 new tests; full post_processing suite and occurrence query-count tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
The roll-up picks the global argmax over every taxon at each rank, not just ancestors of the source classification's taxon. On a diffuse, low-confidence distribution this can reparent a detection to a family outside its own lineage. Document the behavior at the candidate-pick loop and record the open design choice (lineage-constrained vs distribution roll-up) as a TODO; no behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
…ection When an operator triggers class masking on an occurrence (or collection), the "Source classifier" dropdown now lists only the classification algorithms that actually produced classifications within the selected scope. Masking any other algorithm would be a no-op for those rows, so offering every classifier was misleading. The admin action hands the knob form the selected queryset (a small generic seam on the form base, ignored by forms that don't need it), and ClassMaskingActionForm uses it to filter the algorithm field by classifications__detection__occurrence / source_image__collections. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
feat/postprocessing-class-masking
branch
from
6e8eac8 to
ba796c1
Compare
mihow
changed the title
Collaborator
|
Claude says: This branch has been reworked and force-pushed, so the earlier 49-commit history and its inline review threads are now outdated. It is a clean port onto the post-processing framework that merged in #1289, rebased onto current The correctness findings from the earlier review rounds are addressed in the rework:
One item is intentionally not fixed here and is documented as a follow-up: rank rollup picks the global argmax across all taxa at each rank rather than constraining to ancestors of the source taxon, so a diffuse, low-confidence distribution can roll a detection up to a family outside its own lineage. The trade-off is recorded as a Re-review is welcome on the new diff. |
…llections The class-mask admin form narrows its "Source classifier" dropdown to the algorithms that actually produced classifications in the selected scope, so an operator cannot pick a classifier whose masking would be a no-op. On the occurrence path that lookup is cheap (a handful of classifications), but on the collection path it became an unbounded DISTINCT over every classification in the collection. On a large collection that join runs for tens of seconds or times out while the intermediate form is still rendering, before the operator can do anything. Scope the dropdown only for the occurrence path. A collection scope now keeps the full classifier list. Offering a classifier that produced nothing in the collection is harmless — masking it changes no rows — so the narrowing was a convenience, not a correctness guard, and is not worth a query that can hang the page. Co-Authored-By: Claude <noreply@anthropic.com>
Rank roll-up moves out of this PR so class masking can merge on its own. Class masking is validated end to end against a real classifier and is well covered by tests; rank roll-up still has an open design question (whether the per-rank pick should be constrained to the source taxon's ancestors or remain a global distribution roll-up) and thinner tests. Keeping them together would force reviewers to either block ready class-masking work or sign off on the less-settled roll-up. This removes the rank-roll-up task, its admin form and action, its registry entry, and its tests. The roll-up feature returns in its own PR, stacked on this one, where the lineage decision and fuller tests can land before it merges. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
changed the title
Collaborator
|
Claude says: Two follow-up changes coming to the class-masking task, both surfaced by testing it against a large (~29k-class) classifier at scale:
|
…le, add reweight toggle Class masking now commits in batches and reports progress and stage metrics per batch, instead of building the whole scope in memory and committing once at the end. On a large classifier (tens of thousands of classes, thousands of classifications) the single final commit could exhaust memory, and run through the admin/Job path the task was revoked by the job health-check reaper before it committed — so nothing persisted and the occurrences-changed metrics never reached the job page. It now mirrors SmallSizeFilterTask: process in batches, commit each batch in its own transaction, and call update_progress() plus report_stage_metrics() per batch. Memory stays bounded (the source cursor and the per-batch buffers are capped at batch_size), the heartbeat keeps the reaper satisfied, and the live counts appear on the job page. occurrences_updated now counts only occurrences whose determination actually changed, matching the size filter. Also add a reweight toggle (default on) to the config and the admin form. Renormalising the kept classes to sum to 1 does not change which species is chosen — argmax over the kept classes is identical either way — so it affects only the stored confidence. With reweighting off, the kept classes keep their original absolute scores, which is useful for evaluating the model's raw confidence. Co-Authored-By: Claude <noreply@anthropic.com>
… spares long runs The stale-job reaper (check_stale_jobs) revokes running jobs whose updated_at is older than STALLED_JOBS_MAX_MINUTES (10). The post-processing progress heartbeat saved with update_fields=["progress"], and Django does not auto-add auto_now fields to a narrowed update_fields, so updated_at never moved. Any post-processing run longer than 10 minutes looked frozen and was revoked mid-flight even while streaming progress and stage metrics — observed on a large class-masking run (revoked at 42%, ~1600/3814 classifications). Add updated_at to both save paths in BasePostProcessingTask.update_progress and report_stage_metrics, mirroring the async-job progress save in jobs/models.py that already does this for the same reason. Add a regression test that freezes updated_at past the cutoff and asserts each heartbeat path drags it forward. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ami/ml/management/commands/run_class_masking.py (1)
29-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider chaining the original exception.
raise CommandError(...)inside eachexcept ... DoesNotExistblock discards the original traceback. Chaining withfrom errpreserves it for debugging, independent of whether the repo's lint config enforces B904.♻️ Proposed fix
try: collection = SourceImageCollection.objects.get(pk=collection_id) - except SourceImageCollection.DoesNotExist: - raise CommandError(f"SourceImageCollection {collection_id} does not exist.") + except SourceImageCollection.DoesNotExist as err: + raise CommandError(f"SourceImageCollection {collection_id} does not exist.") from err try: taxa_list = TaxaList.objects.get(pk=taxa_list_id) - except TaxaList.DoesNotExist: - raise CommandError(f"TaxaList {taxa_list_id} does not exist.") + except TaxaList.DoesNotExist as err: + raise CommandError(f"TaxaList {taxa_list_id} does not exist.") from err try: algorithm = Algorithm.objects.get(pk=algorithm_id) - except Algorithm.DoesNotExist: - raise CommandError(f"Algorithm {algorithm_id} does not exist.") + except Algorithm.DoesNotExist as err: + raise CommandError(f"Algorithm {algorithm_id} does not exist.") from err🤖 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/ml/management/commands/run_class_masking.py` around lines 29 - 42, The exception handling in run_class_masking’s object lookups drops the original traceback when SourceImageCollection, TaxaList, or Algorithm is missing. Update each except ... DoesNotExist block to chain the caught exception when raising CommandError, using the existing collection, taxa_list, and algorithm lookup sections so the original traceback is preserved for debugging.Source: Linters/SAST tools
ami/ml/post_processing/base.py (1)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect fix —
updated_atnow explicitly included inupdate_fields.This matches Django's semantics:
pre_save()(which bumpsauto_nowfields) only runs for fields present inupdate_fields, so omittingupdated_atpreviously left it untouched during these narrowed saves. Verified by the accompanyingtest_progress_save_bumps_updated_at_for_reapertest.Both call sites duplicate the same
save(update_fields=["progress", "updated_at"])line and near-identical comment. Consider a tiny shared helper to avoid drift if a third heartbeat call site is added later:♻️ Optional: extract shared heartbeat save
+ def _save_job_heartbeat(self, extra_fields: list[str]) -> None: + """Persist ``extra_fields`` alongside ``updated_at`` so the stale-job + reaper (``check_stale_jobs``) sees an actively-progressing run.""" + self.job.save(update_fields=[*extra_fields, "updated_at"]) + def update_progress(self, progress: float): if self.job: self.job.progress.update_stage(self.job.job_type_key, progress=progress) - self.job.save(update_fields=["progress", "updated_at"]) + self._save_job_heartbeat(["progress"])Also applies to: 103-108
🤖 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/ml/post_processing/base.py` around lines 76 - 82, The heartbeat save logic in the post-processing base flow is duplicated across both call sites, which can drift if another progress update is added later. Extract the shared “bump progress and updated_at” behavior in the base post-processing path into a small helper or private method in the same class, and have both call sites use it while keeping the `self.job.save(update_fields=["progress", "updated_at"])` behavior and the stale-job reaper comment aligned.ami/ml/post_processing/tests/test_class_masking.py (1)
253-269: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
.get()/count assertion instead of.first()to catch duplicate-creation regressions.Elsewhere in this file (e.g. lines 145, 173, 198) the new terminal classification is fetched with
.get(...), which fails loudly if more than one row matches. Here,.exclude(algorithm=self.algorithm).first()will silently pass even if masking accidentally creates multiple new terminal classifications for the same detection.💡 Proposed fix
- new_clf = Classification.objects.filter(detection=det, terminal=True).exclude(algorithm=self.algorithm).first() - self.assertIsNotNone(new_clf) + new_clf = Classification.objects.get(detection=det, terminal=True, algorithm__ne=self.algorithm) + # or, if `__ne` lookup isn't available: + new_clfs = Classification.objects.filter(detection=det, terminal=True).exclude(algorithm=self.algorithm) + self.assertEqual(new_clfs.count(), 1) + new_clf = new_clfs.get()🤖 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/ml/post_processing/tests/test_class_masking.py` around lines 253 - 269, In test_task_run_occurrence_scope, replace the lenient Classification.objects.filter(...).exclude(...).first() check with a stricter assertion pattern like the other tests in this file that use .get(), so duplicate terminal classifications cannot slip by unnoticed. Use the same detection/terminal filters on Classification and identify the non-original row by algorithm exclusion, then assert exactly one result and validate its taxon via the fetched object from the ClassMaskingTask flow.
🤖 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 `@ami/ml/management/commands/run_class_masking.py`:
- Around line 49-58: The dry-run/validation count in run_class_masking.py is
using a broader filter than the actual task scope, so it can report
classifications that ClassMaskingTask._scoped_classifications will skip. Update
the classification_count query to match the same constraints as
_scoped_classifications by requiring both scores and logits to be present, and
keep the guard/--dry-run reporting aligned with that shared scope.
In `@ami/ml/post_processing/class_masking.py`:
- Around line 231-249: The masking algorithm identity in
_get_or_create_masking_algorithm currently only distinguishes source_algorithm
and taxa_list, so reweight=True and reweight=False reuse the same Algorithm row.
Update the Algorithm.objects.get_or_create key (and any related name/description
if needed) to include the reweight mode, so the provenance stored for masked
outputs uniquely reflects whether scores were renormalized or preserved. Use the
existing _get_or_create_masking_algorithm symbol to locate the change and make
sure both modes create or reuse separate algorithm records.
- Around line 129-147: The `class_masking` reweight path is recomputing
kept-class confidence from `logits`, but `reweight=True` should renormalize the
already stored score vector instead. Update the masking logic in `mask_classes`
so the renormalized values are derived from `scores` (with excluded indices
zeroed and the remainder normalized to sum to 1), while preserving the existing
winner selection behavior via the relevant `top_index`/`score` handling.
---
Nitpick comments:
In `@ami/ml/management/commands/run_class_masking.py`:
- Around line 29-42: The exception handling in run_class_masking’s object
lookups drops the original traceback when SourceImageCollection, TaxaList, or
Algorithm is missing. Update each except ... DoesNotExist block to chain the
caught exception when raising CommandError, using the existing collection,
taxa_list, and algorithm lookup sections so the original traceback is preserved
for debugging.
In `@ami/ml/post_processing/base.py`:
- Around line 76-82: The heartbeat save logic in the post-processing base flow
is duplicated across both call sites, which can drift if another progress update
is added later. Extract the shared “bump progress and updated_at” behavior in
the base post-processing path into a small helper or private method in the same
class, and have both call sites use it while keeping the
`self.job.save(update_fields=["progress", "updated_at"])` behavior and the
stale-job reaper comment aligned.
In `@ami/ml/post_processing/tests/test_class_masking.py`:
- Around line 253-269: In test_task_run_occurrence_scope, replace the lenient
Classification.objects.filter(...).exclude(...).first() check with a stricter
assertion pattern like the other tests in this file that use .get(), so
duplicate terminal classifications cannot slip by unnoticed. Use the same
detection/terminal filters on Classification and identify the non-original row
by algorithm exclusion, then assert exactly one result and validate its taxon
via the fetched object from the ClassMaskingTask flow.
🪄 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: c9112f8f-e20b-4fb9-98f2-c6904c0c27a8
📒 Files selected for processing (15)
ami/main/admin.pyami/main/api/serializers.pyami/main/api/views.pyami/main/models_future/occurrence.pyami/ml/management/commands/run_class_masking.pyami/ml/post_processing/__init__.pyami/ml/post_processing/admin/actions.pyami/ml/post_processing/admin/class_masking_form.pyami/ml/post_processing/admin/forms.pyami/ml/post_processing/base.pyami/ml/post_processing/class_masking.pyami/ml/post_processing/registry.pyami/ml/post_processing/tests/test_class_masking.pyami/ml/post_processing/tests/test_class_masking_admin.pyami/ml/tests.py
…ing, reweight in algorithm identity, dry-run filter Three CodeRabbit findings on the class-masking task: - Derive all masked scores from `logits` alone. The reweight=False branch and the no-change short-circuit still read the stored `scores` field; recompute the model's full softmax from logits instead. The `scores` field is being retired, so masking must not depend on it. Mathematically identical while `scores == softmax(logits)` (the winner is unchanged), and it drops the last read of `scores` from the masking math. (Argmax stays on the masked logits — an excluded class still cannot win.) - Include the reweight mode in the masking algorithm's identity. reweight=True and reweight=False persist different score semantics, so keying the output Algorithm on (source algorithm, taxa list) alone made both modes share one row and lose provenance. The key/name now carry a "reweighted" / "absolute" suffix; a new test pins that the two modes resolve to distinct algorithms. - Match the management command's dry-run count to the task scope by also requiring `logits__isnull=False`, so `--dry-run` and the "no classifications" guard no longer overstate what the run will process. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This adds class masking, a post-processing task an admin can run to clean up classifier output after a pipeline has finished.
Class masking is for the common case where you know which species are actually possible at a site (a regional checklist) but the classifier was trained on a much larger label set. Given a taxa list, it re-weights a classifier's predictions so that only species on the list can contribute, then writes new terminal classifications and recomputes the affected occurrence determinations.
It runs through the post-processing framework that merged in #1289, so it is triggered the same way as the existing small-size filter: select rows in the Django admin, pick the task, fill in its options, and the work runs as a background Job you can follow from the admin. Class masking can be run on a single occurrence (the fast path for iterating on a checklist) or on a whole capture set.
This PR is a clean rework of the original class-masking work (which predated the framework and carried a hand-rolled admin action). The branch was reset onto current
main, so the diff shows only the class-masking additions.List of Changes
ClassMaskingTask(BasePostProcessingTask)+make_post_processing_actionwiring onOccurrenceAdmin(single occurrence) andSourceImageCollectionAdmin(capture set)logits, excluded classes are set to exactly 0, and the top of what remains wins; raises if the taxa list excludes every class in the category mapAlgorithmper(source algorithm, taxa list)with its category map persisted (was set in memory only, leaving a null map)applied_topopulated on new masked classifications and added to the Classification serializers, withapplied_to__algorithmprefetched in the occurrence list/detail and classification viewset (no N+1)ClassMaskingActionFormscopes the algorithm field for the occurrence path via a small generic seam on the action form base. The capture-set path keeps the full list on purpose: the equivalent lookup is an unboundedDISTINCTover every classification in the collection and can time out while the form renders — an over-broad option is harmless because masking a classifier that produced nothing in scope changes nothingrun_class_maskingmanagement commanditerator()to bound memory. Mirrors the small-size filter.occurrences_updatedcounts only occurrences whose determination actually changed, not every row re-saved…_reweighted/…_absolute) so a masked classification's provenance records which mode produced itJob.updated_at. The stale-job reaper revokes running jobs whoseupdated_atis older than 10 minutes; a narrowedsave(update_fields=["progress"])did not move theauto_nowupdated_at, so any run over 10 minutes was reaped mid-flight even while streaming progress. This also fixes the same latent issue for the small-size filterDetailed Description
applied_toalready exists onClassification, and the task runs under the existingpost_processingjob type, so the registry addition does not touch any field definition.makemigrations --checkis clean.logitsalone. The masked score vector is recomputed from the storedlogits(softmax over all classes, then excluded classes zeroed and optionally renormalised); the task never reads the storedscoresfield, which is on a path to being retired. The persistedlogitsare written back unchanged (JSON-safe) so the masked classification keeps the raw model output; the mask is reflected in the newscores(excluded classes → 0). Whilescores == softmax(logits)this is numerically identical to the previous behaviour, so the chosen taxon and confidence are unchanged.SUCCESS.BasePostProcessingTask.update_progressandreport_stage_metricsnow saveupdate_fields=["progress", "updated_at"], mirroring the async-job progress save inami/jobs/models.pythat already does this for the same reason. Django does not auto-addauto_nowfields to a narrowedupdate_fields, so the timestamp had to be listed explicitly. A regression test freezesupdated_atpast the reaper cutoff and asserts each heartbeat path moves it forward.How to test
Class masking on a single occurrence (fast path)
logits.Class masking on a capture set
SourceImageCollection) → select one → choose the Class Masking action → fill in options → run.Verify provenance in the API
Under
detections[].classifications[], the new masked classification carries anapplied_toobject describing the source classification it was derived from.Related