Add include/exclude mode to mass transfer filters#336
Conversation
Each filter in filters_json now supports an optional mode field
("include" or "exclude", default "include"). After the include
discovery loop builds its DiscoveredSeries set, any series that
matches at least one exclude filter is dropped.
Exclude filters reuse the existing per-criterion matching logic
via a new _series_matches_filter helper and do not issue their
own C-FIND queries — all criteria are evaluated client-side
against the DiscoveredSeries already in memory.
Validation rejects jobs with no include filter and exclude
filters with no criteria. apply_institution_on_study is
documented as include-only and silently ignored on excludes.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds include/exclude semantics to mass-transfer filters: ChangesMass-transfer filter flow
sequenceDiagram
actor User
participant Form as FilterSchema
participant Processor as _discover_series
participant PacsDB as PACS Database
participant Matcher as _series_matches_filter
User->>Form: Submit filters with modes
Form->>Form: Validate mode and require criteria for excludes
Form->>Processor: Pass validated FilterSpecs
Processor->>PacsDB: Query studies & series (include filters only)
PacsDB-->>Processor: Return studies & series
loop For each discovered series
Processor->>Matcher: Evaluate include specs (control institution/age behavior)
alt Matches an include
Matcher-->>Processor: Accept series
else
Matcher-->>Processor: Skip series
end
end
loop Post-filter by exclude specs
Processor->>Matcher: Evaluate exclude specs (age_permissive=True)
alt Matches any exclude
Matcher-->>Processor: Mark for removal
Processor->>Processor: Remove matched series
end
end
Processor-->>User: Return final filtered series list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces "exclude" filters to the mass transfer system, allowing users to filter out specific DICOM series after an initial inclusion. Key changes include updates to the FilterSchema and FilterSpec models to support a mode field, new validation logic to ensure at least one include filter is present, and the centralization of filtering logic into a _series_matches_filter helper function. Feedback was provided to use a Literal type for the mode field in the processor to improve type safety and maintain consistency with the form schema.
| Built from a plain dict from the job's filters_json field. | ||
| """ | ||
|
|
||
| mode: str = "include" |
There was a problem hiding this comment.
For better type safety and consistency with the FilterSchema defined in forms.py, consider using a Literal for the mode field. This helps static analysis tools like pyright (which is used in this repository) to catch invalid values.
| mode: str = "include" | |
| mode: Literal["include", "exclude"] = "include" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adit/mass_transfer/processors.py`:
- Around line 796-801: Add a processor-side guard in _discover_series() to
enforce the include-filter invariant: after computing include_filters = [mf for
mf in filters if mf.mode != "exclude"] and exclude_filters = [...], check that
include_filters is not empty (i.e., filters_json isn't all-exclude) and if it
is, raise a clear exception (e.g., ValidationError or ValueError) with a message
referencing filters_json/filters so the job fails fast instead of returning no
series; this complements clean_filters_json() and prevents jobs created outside
the form from silently succeeding.
🪄 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: 8029f450-d991-40ff-a286-ef58422292f7
📒 Files selected for processing (4)
adit/mass_transfer/forms.pyadit/mass_transfer/processors.pyadit/mass_transfer/tests/test_forms.pyadit/mass_transfer/tests/test_processor.py
- Type FilterSpec.mode as Literal["include", "exclude"] to match FilterSchema and let pyright catch invalid values. - Validate mode in FilterSpec.from_dict so jobs persisted outside the form (e.g. admin, direct ORM) can't smuggle in an unknown mode string. - Add a _discover_series guard that raises DicomError when no include filter is present, turning a silent empty-result into a loud failure.
Flip age_permissive in both call sites so an unverifiable PatientBirthDate fails inclusion and matches exclusion — in either direction, a series we can't age-check is dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
adit/mass_transfer/processors.py (1)
801-890: ⚡ Quick winAdd structured logs for include/exclude discovery transitions.
This block introduces important state transitions (filter split, post-exclude pruning) but currently emits no structured telemetry. Please add structured log events with include/exclude counts and prune totals.
Proposed logging points
include_filters = [mf for mf in filters if mf.mode != "exclude"] exclude_filters = [mf for mf in filters if mf.mode == "exclude"] + logger.info( + "Mass transfer discovery filter split", + extra={ + "task_id": self.mass_task.pk, + "include_filter_count": len(include_filters), + "exclude_filter_count": len(exclude_filters), + }, + ) if not include_filters: raise DicomError("Mass transfer requires at least one include filter.") @@ - if not exclude_filters: - return list(found.values()) + if not exclude_filters: + logger.info( + "Mass transfer discovery complete", + extra={ + "task_id": self.mass_task.pk, + "discovered_series_count": len(found), + "excluded_series_count": 0, + }, + ) + return list(found.values()) - return [ + filtered = [ series for series in found.values() if not any( _series_matches_filter(series, mf, age_permissive=True) for mf in exclude_filters ) ] + logger.info( + "Mass transfer discovery complete", + extra={ + "task_id": self.mass_task.pk, + "discovered_series_count": len(found), + "excluded_series_count": len(found) - len(filtered), + }, + ) + return filteredAs per coding guidelines,
**/*.{js,ts,py}: Use structured logging for agent actions and state transitions.🤖 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 `@adit/mass_transfer/processors.py` around lines 801 - 890, The code splits filters into include_filters and exclude_filters and builds found (DiscoveredSeries) but lacks structured logging; add structured logs (using the module's standard logger) after the split to emit counts for include_filters and exclude_filters, and another structured event before returning to report total discovered, excluded_count (pruned by exclude_filters) and final_count; place the first log immediately after building include_filters/exclude_filters and the second just before the final return where found is filtered (reference symbols: include_filters, exclude_filters, found, DiscoveredSeries, _series_matches_filter) and include filter identifiers (e.g. mf names or modes) and numeric counts in the log payload.
🤖 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 `@adit/mass_transfer/processors.py`:
- Around line 801-803: There is a dangerous case where exclude_filters (built
from filters in the existing code) may have no criteria and cause
_series_matches_filter(...) to return True for all series; update
_discover_series to detect any exclude filter with empty criteria and fail fast:
iterate the exclude_filters list (same objects created where
include_filters/exclude_filters are derived) and if any filter has no criteria
(e.g., empty attributes/conditions field used by _series_matches_filter), raise
a clear ValueError (or similar exception) naming the offending filter(s) instead
of proceeding; ensure the guard is checked before the pruning loop that calls
_series_matches_filter so the code in _discover_series (and the pruning logic
around _series_matches_filter) cannot silently remove all series.
---
Nitpick comments:
In `@adit/mass_transfer/processors.py`:
- Around line 801-890: The code splits filters into include_filters and
exclude_filters and builds found (DiscoveredSeries) but lacks structured
logging; add structured logs (using the module's standard logger) after the
split to emit counts for include_filters and exclude_filters, and another
structured event before returning to report total discovered, excluded_count
(pruned by exclude_filters) and final_count; place the first log immediately
after building include_filters/exclude_filters and the second just before the
final return where found is filtered (reference symbols: include_filters,
exclude_filters, found, DiscoveredSeries, _series_matches_filter) and include
filter identifiers (e.g. mf names or modes) and numeric counts in the log
payload.
🪄 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: 522f1d54-31a4-48ef-8419-291bcc4cd903
📒 Files selected for processing (2)
adit/mass_transfer/processors.pyadit/mass_transfer/tests/test_processor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- adit/mass_transfer/tests/test_processor.py
| include_filters = [mf for mf in filters if mf.mode != "exclude"] | ||
| exclude_filters = [mf for mf in filters if mf.mode == "exclude"] | ||
|
|
There was a problem hiding this comment.
Enforce non-empty exclude criteria in processor-level runtime checks.
If an exclude filter with no criteria bypasses form validation, _series_matches_filter(...) returns True for all series, and Line 887 will prune everything silently. Add a fail-fast guard for empty exclude filters in _discover_series().
Proposed fix
include_filters = [mf for mf in filters if mf.mode != "exclude"]
exclude_filters = [mf for mf in filters if mf.mode == "exclude"]
if not include_filters:
raise DicomError("Mass transfer requires at least one include filter.")
+ for mf in exclude_filters:
+ has_criterion = any(
+ [
+ bool(mf.modality),
+ bool(mf.institution_name),
+ bool(mf.study_description),
+ bool(mf.series_description),
+ mf.series_number is not None,
+ mf.min_age is not None,
+ mf.max_age is not None,
+ mf.min_number_of_series_related_instances is not None,
+ ]
+ )
+ if not has_criterion:
+ raise DicomError("Exclude filters must specify at least one criterion.")
found: dict[str, DiscoveredSeries] = {}Also applies to: 887-889
🤖 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 `@adit/mass_transfer/processors.py` around lines 801 - 803, There is a
dangerous case where exclude_filters (built from filters in the existing code)
may have no criteria and cause _series_matches_filter(...) to return True for
all series; update _discover_series to detect any exclude filter with empty
criteria and fail fast: iterate the exclude_filters list (same objects created
where include_filters/exclude_filters are derived) and if any filter has no
criteria (e.g., empty attributes/conditions field used by
_series_matches_filter), raise a clear ValueError (or similar exception) naming
the offending filter(s) instead of proceeding; ensure the guard is checked
before the pruning loop that calls _series_matches_filter so the code in
_discover_series (and the pruning logic around _series_matches_filter) cannot
silently remove all series.
2 participants
Summary
filters_jsoncan now setmode: "include" | "exclude"(defaultinclude). A series is kept if it matches at least one include filter and no exclude filter._series_matches_filterhelper and does not issue its own C-FIND queries — all checks run client-side against theDiscoveredSeriesalready collected by the include pass, so excludes add zero PACS round-trips._discover_seriesare replaced by a single call to the helper, which is also used for the post-filter that applies excludes.Validation
mode=include(an all-excludes job is rejected).apply_institution_on_studyis documented as include-only and silently ignored on excludes; institution is always evaluated at series level on theDiscoveredSeries.Why this approach
C-FIND has no anti-match operator, so any exclusion logic has to run client-side.
DiscoveredSeriesalready carries every field the existing filter criteria reference (modality, institution, descriptions, series number, age frompatient_birth_date+study_datetime, instance count), so excludes can be implemented as a pure post-filter without new queries, without a new schema change, and without touching the transfer phase.Test plan
ruff checkpassespyrightpasses_series_matches_filter(modality, institution, series number, age strict vs permissive,check_institution=False,min_instances)_discover_seriesexclude behavior (single-exclude removes, multi-exclude OR semantics, exclude-by-institution, and a call-count assertion that excludes issue no extra C-FINDs)_discover_seriestests still pass (the refactor is behavior-preserving for include-only filters)Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests