Add include/exclude mode to mass transfer filters#336
Add include/exclude mode to mass transfer filters#336NumericalAdvantage wants to merge 2 commits intomainfrom
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
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 48 seconds. ⌛ 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 changes introduce include and exclude filter modes to mass transfer functionality. The Changes
Sequence DiagramsequenceDiagram
actor User
participant Form as FilterSchema<br/>(Validation)
participant Processor as _discover_series<br/>(Discovery)
participant PacsDB as PACS Database
participant Matcher as _series_matches_filter<br/>(Matching)
User->>Form: Submit filters with modes
Form->>Form: Validate mode field<br/>(include/exclude)
Form->>Form: Check exclude filters<br/>have criteria
Form->>Processor: Pass validated FilterSpecs
Processor->>PacsDB: Query studies using<br/>include-mode filters only
PacsDB-->>Processor: Return studies & series
loop For each series discovered
Processor->>Matcher: Check against<br/>include specs
alt Matches include spec
Matcher-->>Processor: Include series
else Does not match
Matcher-->>Processor: Skip series
end
end
loop Post-filter against exclude specs
Processor->>Matcher: Check against<br/>exclude specs
alt Matches exclude spec
Matcher-->>Processor: Mark for removal
Processor->>Processor: Remove matched series
end
end
Processor-->>User: Return filtered series
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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.
1 participant
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
Release Notes