Conversation
✅ Deploy Preview for antenna-preview canceled.
|
📝 WalkthroughWalkthroughIntroduces granular default filter flags, environment-backed default include/exclude taxa, project-level default filter initialization, filtered detection count methods, and signals to refresh cached counts when project default filters change. Changes
Sequence DiagramsequenceDiagram
actor Client
participant ViewSet as TaxonViewSet
participant Filters as build_occurrence_default_filters_q
participant Models as Project/Deployment/Event
participant Signals as Signal Handlers
Client->>ViewSet: retrieve(project_id)
ViewSet->>ViewSet: get_queryset()
ViewSet->>ViewSet: get_taxa_observed(apply_default_score_filter=True, apply_default_taxa_filter=False)
ViewSet->>Filters: build_occurrence_default_filters_q(project, ..., apply_default_score_filter=True, apply_default_taxa_filter=False)
Note over Filters: score filter applied (if enabled)\ntaxa filter skipped
Filters-->>ViewSet: Q object
ViewSet->>Models: query taxa/occurrences using Q
Models-->>ViewSet: taxa queryset
ViewSet-->>Client: response
Note over Signals: When project defaults change
Models->>Signals: pre_save (cache_old_threshold)
Signals->>Signals: store old threshold
Models->>Signals: post_save (threshold_updated)
Signals->>Models: refresh_cached_counts_for_project() [if changed]
Signals->>Models: update_related_calculated_fields()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… deployment, and source image
mohamedelabbas1996
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ami/main/signals.py (1)
8-11: Default-filter signals correctly trigger cache refresh; consider linter and perf tweaksThe new
refresh_cached_counts_for_project, threshold-change, and include/exclude-taxa m2m signals are wired correctly and line up withProject.update_related_calculated_fields(); behavior looks sound.Two small follow-ups:
- To satisfy Ruff’s ARG001 without changing behavior, you can rename unused args to underscore variants, e.g.:
def cache_old_threshold(_sender, instance, **_kwargs): ... def threshold_updated(_sender, instance, **_kwargs): ... def include_taxa_updated(_sender, instance: Project, action, **_kwargs): ... def exclude_taxa_updated(_sender, instance: Project, action, **_kwargs): ...project.update_related_calculated_fields()can be expensive for large projects (iterates all events/deployments and saves). If this becomes a bottleneck, consider deferring to a background job or at least wrapping with some timing/metrics to monitor impact.Also applies to: 133-191
ami/main/models_future/filters.py (1)
128-135: New per-component default-filter flags are well structured; docstring could mention themThe addition of
apply_default_score_filterandapply_default_taxa_filterpreserves existing behavior when left at their defaults and cleanly allows callers to toggle score vs taxa components independently. Early returns forproject is Noneandapply_defaults=falsekeep existing call sites safe.Consider updating the function docstring/Examples section to briefly describe these two flags and show a usage where only score or only taxa defaults are applied, so future readers don’t have to infer the new behavior from the implementation.
Also applies to: 191-213
ami/main/models.py (2)
153-162: Default taxa application to new “Scratch Project” is straightforward; consider logging unknown taxa
get_project_default_filters()and the updatedget_or_create_default_project()cleanly wire the env-configured taxa into a new default project via the include/exclude M2M fields; behavior on existing projects is unchanged.If you want to make ops misconfigurations easier to detect, a small enhancement would be to log any names in
settings.DEFAULT_INCLUDE_TAXA/DEFAULT_EXCLUDE_TAXAthat don’t resolve to aTaxon, e.g. by comparing the configured name lists to theTaxon.objects.filter(...).values_list("name", flat=True)results before returning.Also applies to: 164-186
704-715: Detection-count helpers correctly respect project defaults; align bulk updater semantics and return typesThe new
get_detections_countmethods onDeployment,Event, andSourceImagecorrectly centralize counting throughbuild_occurrence_default_filters_q, so cacheddetections_countnow reflects project default filters when a project is present, and falls back to unfiltered counts when not.Two follow-ups to consider:
- The type hints for
Deployment.get_detections_countandEvent.get_detections_countare-> int | None, but the implementations always return anint(count()), even when there is no project. Tightening these to-> intwould better match actual behavior.SourceImage.update_calculated_fields()now setsdetections_countusing the default-filtered helper, whileupdate_detection_counts()still bulk-updatesdetections_countas the raw number of detections (no default filters). BecauseEvent.timeline()usesupdate_detection_counts(qs, null_only=True)before aggregatingdetections_count, older rows (with null counts) will be populated with unfiltered counts, while newer rows updated viaget_detections_count()will be filtered. If the intent is thatdetections_counteverywhere represents the default-filtered value, it would be worth updatingupdate_detection_counts()to apply the same filtering (or documenting that it intentionally uses raw counts and only for legacy backfill).Also applies to: 919-922, 1085-1095, 1799-1813, 1970-1993
ami/main/api/views.py (1)
1478-1483: Taxon detail/list default-filter handling is consistent; consider documenting new flagsThe revised
get_taxa_observedflow looks good:
- List view: still filters taxa by
filter_by_project_default_taxa(...)and callsget_taxa_observedwith default flags, so both score and taxa defaults apply (unlessapply_defaults=falseis passed, in which casebuild_occurrence_default_filters_qreturnsQ()andbase_filteris unchanged).- Detail view: calls
get_taxa_observed(..., apply_default_score_filter=True, apply_default_taxa_filter=False), so the project’s score threshold now applies while taxa include/exclude are not re-applied inside the occurrence subqueries. That matches the intent of aligning detail counts with list behavior for included taxa while still allowing detail on excluded taxa.The use of
build_occurrence_default_filters_qwith the new flags keeps the semantics consistent with other parts of the codebase. As a small improvement, you might extend theget_taxa_observeddocstring to mention the two flags and how they’re used for list vs detail views, since the signature is now doing a bit more work than the current docstring reflects.Also applies to: 1499-1505, 1523-1529, 1537-1538
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ami/main/api/views.py(3 hunks)ami/main/models.py(6 hunks)ami/main/models_future/filters.py(2 hunks)ami/main/signals.py(2 hunks)config/settings/base.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ami/main/models_future/filters.py (1)
ami/utils/requests.py (1)
get_default_classification_threshold(124-145)
ami/main/api/views.py (1)
ami/main/models_future/filters.py (1)
build_occurrence_default_filters_q(128-214)
ami/main/models.py (1)
ami/main/models_future/filters.py (1)
build_occurrence_default_filters_q(128-214)
ami/main/signals.py (1)
ami/main/models.py (2)
Project(249-489)update_related_calculated_fields(331-341)
🪛 Ruff (0.14.5)
ami/main/signals.py
142-142: Unused function argument: sender
(ARG001)
142-142: Unused function argument: kwargs
(ARG001)
163-163: Unused function argument: sender
(ARG001)
163-163: Unused function argument: kwargs
(ARG001)
179-179: Unused function argument: sender
(ARG001)
179-179: Unused function argument: kwargs
(ARG001)
187-187: Unused function argument: sender
(ARG001)
187-187: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (1)
config/settings/base.py (1)
451-453: Env-configured default taxa lists look correctReading
DEFAULT_INCLUDE_TAXA/DEFAULT_EXCLUDE_TAXAviaenv.listwith list defaults is consistent with the existing settings usage; no issues from a settings perspective.
…d-cache Resolved SourceImage.get_detections_count conflict by combining both changes: apply project default filters AND exclude null-bbox placeholder detections (from #1093). Preserved get_was_processed method added in main. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-ssec canceled.
|
|
Claude says: (on @mihow's behalf) Merged Existing test classes pass on the merged branch (45 tests across Takeaway reviewRisks
Minimum test set to add before mergeThree tests, ~150 lines total, each protecting a specific claim:
Not recommended to add to this PRAdding post-processing refresh (auto-update counts after ML job completion) — different hook point (Job, not Project), different fan-out profile (many jobs complete → needs Celery, not sync), and this PR is already bundling three concerns. Tracked separately as a follow-up. Scope observationThree loosely-related concerns bundled (env-var taxa, signal refresh, view filter refactor). If this can't land quickly, the env-var piece is tiny and isolated — safe to split off and merge today. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ami/main/api/views.py (1)
1503-1515:⚠️ Potential issue | 🟠 MajorApply the same score filter to
example_occurrences.The retrieve annotations now ignore below-threshold occurrences, but the prefetch right below still pulls any matching occurrence. That can return a preview that was not counted, e.g.
occurrences_count=0whileexample_occurrencesis non-empty.🔧 Suggested change
Prefetch( "occurrences", - queryset=Occurrence.objects.filter(self.get_occurrence_filters(project))[:1], + queryset=Occurrence.objects.filter(self.get_occurrence_filters(project)) + .filter_by_score_threshold(project, self.request)[:1], to_attr="example_occurrences", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/api/views.py` around lines 1503 - 1515, The prefetch for example_occurrences can return occurrences that were excluded by the score filter applied in get_taxa_observed, causing mismatched counts; update the Prefetch queryset so it uses the same occurrence filter logic as get_taxa_observed (i.e., include the default score threshold) instead of the broader Occurrence.objects.filter(self.get_occurrence_filters(project)) used now. Concretely, change the Prefetch/queryset expression that builds example_occurrences to apply the same score-aware filters (for example by calling the same helper or by passing an argument like apply_score_filter=True to self.get_occurrence_filters or reusing the filter used by get_taxa_observed) so the prefetch only pulls occurrences that would be counted by get_taxa_observed.
🤖 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/models.py`:
- Around line 1935-1951: The stored detections_count writers must use the same
default-filter semantics as SourceImage.get_detections_count(); update
Project.update_related_calculated_fields(), update_detection_counts(), and any
backfill paths to compute counts through SourceImage.get_detections_count() (or
apply build_occurrence_default_filters_q with occurrence_accessor="occurrence")
instead of raw self.detections.count(), and ensure Project.default-filter
changes trigger recomputation of related cached counts so saved detections_count
never diverges from get_detections_count().
- Around line 174-185: The new scratch Project created via
Project.objects.get_or_create(...) is not running your custom creation logic so
related defaults (device, site, deployment, collection) are never created; after
detecting created is True in the block that calls get_project_default_filters()
and sets taxa, call create_related_defaults(project) immediately (before
project.save()) to fully initialize the project, and update the Project
docstring/comment if it still claims related objects are created on save to
reflect this explicit initialization step.
---
Outside diff comments:
In `@ami/main/api/views.py`:
- Around line 1503-1515: The prefetch for example_occurrences can return
occurrences that were excluded by the score filter applied in get_taxa_observed,
causing mismatched counts; update the Prefetch queryset so it uses the same
occurrence filter logic as get_taxa_observed (i.e., include the default score
threshold) instead of the broader
Occurrence.objects.filter(self.get_occurrence_filters(project)) used now.
Concretely, change the Prefetch/queryset expression that builds
example_occurrences to apply the same score-aware filters (for example by
calling the same helper or by passing an argument like apply_score_filter=True
to self.get_occurrence_filters or reusing the filter used by get_taxa_observed)
so the prefetch only pulls occurrences that would be counted by
get_taxa_observed.
🪄 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: e16a65b8-aad9-4a94-bf7b-17c3643ffe0d
📒 Files selected for processing (3)
ami/main/api/views.pyami/main/models.pyconfig/settings/base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- config/settings/base.py
Follow-up fixes on top of the cached-counts work:
1. Detection-count hierarchy consistency. Deployment.get_detections_count
and Event.get_detections_count now exclude NULL_DETECTIONS_FILTER
(null-bbox placeholder rows) to match SourceImage.get_detections_count.
Without this, sum(image counts) != event count != deployment count when
an image had been processed with no detections found.
2. Move the default-filter refresh signal off the request thread. The
refresh fans out update_calculated_fields across every Event and
Deployment in a project, which could stall a Project save with hundreds
of events. Wrap the call in transaction.on_commit and dispatch a new
Celery task (ami.main.tasks.refresh_project_cached_counts) so the save
returns immediately and the work only runs if the transaction commits.
3. Three targeted tests in TestCachedCountsDefaultFilters:
- taxa list/detail parity on occurrences_count under a score threshold
- Deployment/Event/SourceImage detection-count hierarchy consistency,
including an explicit null-bbox placeholder that must not be counted
- signal idempotency: Project.save without a threshold change enqueues
no refresh task; a threshold change enqueues exactly one.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude says: (on @mihow's behalf) Pushed
28 tests pass locally ( Auto-refresh on ML job completion stays out of this PR — tracked as a follow-up stub at |
…e cache - get_or_create_default_project now calls ProjectManager.create_related_defaults after a get_or_create, since get_or_create bypasses the manager's create() override. New scratch projects get the default device, site, deployment, collection, and processing service instead of only taxa defaults. - update_detection_counts accepts an optional project kwarg and applies the project's default-filter Q so the bulk subquery matches SourceImage.get_detections_count(). - Project.update_related_calculated_fields now refreshes SourceImage cached detection counts in addition to Event and Deployment, keeping the hierarchy consistent after a default-filters change. - New regression test verifies SourceImage.detections_count tracks get_detections_count() across threshold changes. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Summary
Makes sure project-level default filters (score threshold plus include/exclude taxa) are applied consistently wherever the UI shows counts — taxa pages, sessions, stations, and images — so users see the same totals for the same project regardless of which view they're on.
This PR continues the default filters follow-up work from #1029 by adding environment-variable–based default include and exclude taxa to projects.
It also ensures that all cached count fields (detections, occurrences, and taxa counts) consistently respect the project's default filters across the
Deployment,Event, andSourceImagemodels.Finally, this PR fixes an inconsistency in the Taxon Detail view, where the project's default score threshold was not being applied causing the direct occurrence count on the detail page to diverge from the count shown in the Taxon List view.
List of Changes
Added
DEFAULT_INCLUDE_TAXAandDEFAULT_EXCLUDE_TAXAenvironment variables and updatedget_or_create_default_project()to attach these default taxa to newly created default projects.Fixed
get_or_create_default_project()to explicitly callProject.objects.create_related_defaults(project)—get_or_createbypasses the customProjectManager.create()override, so the default device, site, deployment, collection, and processing service were not being created.Added signals to refresh cached counts when:
default_filters_score_thresholdchangesMoved the cached-count fan-out off the request/save path: the signals now enqueue a new
refresh_project_cached_countsCelery task viatransaction.on_commit.Applied project default filters to detection counting in:
Deployment.get_detections_count()
Event.get_detections_count()
SourceImage.get_detections_count()
Aligned
Deployment.get_detections_count()andEvent.get_detections_count()withSourceImage.get_detections_count()by excluding null-bbox placeholder rows (NULL_DETECTIONS_FILTER).Extended
update_detection_counts()to accept an optionalprojectkwarg and updatedProject.update_related_calculated_fields()to refresh the cachedSourceImage.detections_countin addition to Events and Deployments, so the stored count stays consistent with the filter-aware getter.Fixed get_taxa_observed() to allow separate application of score vs taxa filters
Related Issues
#994
Detailed Description
This PR completes the default taxa filter follow-up tasks by addressing three issues:
1. Environment-based default include/exclude taxa
Default include/exclude taxa can now be provided via environment variables (DEFAULT_INCLUDE_TAXA, DEFAULT_EXCLUDE_TAXA).
These are automatically assigned to default projects during creation.
2. Cached count updates more efficiently
Cached fields for occurrences, detections, and taxa counts in
Deployment,Event, andSourceImageare now updated only when the project's default filter values change.Signals detect changes to: default score threshold, default include taxa and default exclude taxa and only then trigger recalculation of cached counts. The recalculation itself runs in a Celery task (
refresh_project_cached_counts) dispatched on transaction commit, so the request/save path is not blocked by the fan-out across every event, deployment, and source image in the project.3. Taxon Detail view filter consistency fix
The Taxon Detail view previously skipped the default score threshold, causing mismatched occurrence counts compared to the Taxon List view.
This has now been corrected, ensuring consistent filtering across both views.
How to Test the Changes
call
get_or_create_default_project(user)Confirm include/exclude taxa are assigned, and that the project also has a default device, site, deployment, collection, and processing service.
2.
a. Update the project's default score threshold (default_filters_score_threshold) from the Project admin page antenna ui.
Verify that detections, occurrences, and taxa counts update consistently in:
Session (Event) list view
Session detail view
Stations (Deployment) list view
Images list (cached
detections_counton source images)b. Modify the project's default include or exclude taxa (add/remove taxa).
Again, verify that all cached counts update correctly across:
Event list & detail views
Deployment (stations) list view
Images list
Screenshots
N/A
Deployment Notes
Make sure to set the environment variables for default project taxa:
DEFAULT_INCLUDE_TAXADEFAULT_EXCLUDE_TAXAChecklist
Summary by CodeRabbit
New Features
Improvements