feat(audience): expose device filter for filtered audiences#674
feat(audience): expose device filter for filtered audiences#674RapidPoseidon wants to merge 4 commits into
Conversation
The backend now supports a device audience filter, so DeviceFilter can be used when deriving a filtered audience via RapidataAudience.filter(). - DeviceFilter._to_audience_model() now emits IAudienceFilterDeviceAudienceFilter instead of raising NotImplementedError. - BackendFilterMapper decodes the device audience filter back to a DeviceFilter so reading an audience's filters round-trips. - Docs + docstring list DeviceFilter (and the previously-missing DemographicFilter) as supported audience filters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: lino <lino@rapidata.ai>
ReviewOverview: Small, focused change that enables Code quality
Potential issues
Test coverage
Docs
Security / performance
Overall: clean, well-scoped change that follows established conventions. The one thing I'd tighten up is a short comment on the |
| |---|---| | ||
| | `CountryFilter` | ISO-3166 country code (e.g. `["US", "CA"]`) | | ||
| | `LanguageFilter` | Spoken / device language (e.g. `["en", "de"]`) | | ||
| | `DemographicFilter` | Demographic attribute (e.g. `DemographicFilter("age", ["18-29"])`) | |
There was a problem hiding this comment.
why did we expose this as well?
There was a problem hiding this comment.
if demographic filters are indeed already supported. then please at least put them in an enum
LinoGiger
left a comment
There was a problem hiding this comment.
didn't mean to approve
Keep this PR focused on the device filter per review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: lino <lino@rapidata.ai>
|
Good call — the On the underlying point: demographic identifiers/values are currently free strings ( |
ReviewOverviewSmall, well-scoped PR that finishes wiring Code quality
Potential issue: decode defensivenesselif isinstance(actual_instance, BackendDeviceAudienceFilter):
from rapidata.rapidata_client.filter.models.device_type import DeviceType
return ClientDeviceFilter(
[DeviceType(getattr(dt, "value", dt)) for dt in actual_instance.device_types]
)
Docs discrepancyThe PR description says "I added the previously-missing Test coverageNo automated tests were added (matches this repo's existing convention of no unit test suite under Security / performanceNo concerns — this is a pure data-mapping change with no new I/O, external input handling, or hot-path performance implications. Overall: solid, minimally-scoped change that follows existing patterns closely. Only the docs table gap above seems worth resolving before merge. |
Address review: demographic identifiers were free strings (and the occupation identifier is a versioned, unguessable value). DemographicFilter now takes a DemographicIdentifier enum (AGE / GENDER / OCCUPATION) instead of a raw string, and the backend->client mapper decodes it back to the enum. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: lino <lino@rapidata.ai>
|
Done — addressed both comments:
Note this is a breaking change for any caller currently passing a string ( |
Review: feat(audience): expose device filter for filtered audiencesOverviewThis PR completes the The core Potential issues1. Scope creep:
2. Decode-side robustness regression for
3. Minor: unnecessary defensive coercion for device types
4. Minor style: ad-hoc imports inside
Test coverageNo automated tests are added (the repo doesn't appear to have a test suite at all, matching existing convention), and verification is described as a manual encode-decode round trip. Given the enum-based decode paths introduced here can now raise on previously-valid data (see item 2), a unit test for Nits
Overall the |
| |---|---| | ||
| | `CountryFilter` | ISO-3166 country code (e.g. `["US", "CA"]`) | | ||
| | `LanguageFilter` | Spoken / device language (e.g. `["en", "de"]`) | | ||
| | `DemographicFilter` | Demographic attribute (e.g. `DemographicFilter(DemographicIdentifier.AGE, ["18-29"])`) | |
There was a problem hiding this comment.
no but i want the whole thing to be enum - so also the 2nd values since the ranges are predefined in the backend
…generic DemographicFilter Per review: demographic audience filtering should be fully enum-based, and only age and gender for now. AgeFilter and GenderFilter (which already use the AgeGroup / Gender enums) now implement _to_audience_model, emitting a DemographicAudienceFilter with the 'age' / 'gender' identifier. The mapper decodes those back to the respective filter. Removes the free-string DemographicFilter and the DemographicIdentifier enum: occupation is deferred, and the per-dimension filters cover age + gender in a type-safe way. RapidataFilters now exposes Age / Gender instead of Demographic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: lino <lino@rapidata.ai>
|
Reworked per your call — age and gender only, fully enum:
pyright clean, encode/decode round-trips verified for age, gender, and device. Device filter (this PR's original purpose) is unchanged. |
ReviewOverall this is a clean, well-scoped follow-up — the encode/decode symmetry between Potential bug: unsupported demographic identifiers break audience listing, not just decodingIn raise ValueError(
f"Unsupported demographic filter identifier '{identifier}'. Only 'age' and 'gender' are supported."
)Before this PR,
If there's any chance production data already has demographic filters with identifiers outside Minor
Nits
No security concerns — this is all client-side model translation with no new I/O, secrets, or untrusted input handling. |
What
Enables
DeviceFilterwhen deriving a filtered audience — the SDK follow-up to backend rapidata-backend#4697, now that the audience device-filter model is in the generated client.Why
DeviceFilterworked for orders/jobs but._to_audience_model()raisedNotImplementedError, soRapidataAudience.filter([DeviceFilter(...)])failed client-side. The backend now accepts a device audience filter, so the SDK can emit it.How
DeviceFilter._to_audience_model()now returnsIAudienceFilter(IAudienceFilterDeviceAudienceFilter(_t="DeviceFilter", deviceTypes=...))(mirrors the existing_to_model()order path and the other_to_audience_model()filters).BackendFilterMapperdecodesIAudienceFilterDeviceAudienceFilterback into aDeviceFilter, so reading an audience's filters round-trips.audiences.md) and thefilter()docstring listDeviceFilteras supported — and I added the previously-missingDemographicFilterrow while there.Verification
pyright src/rapidata/rapidata_client— 0 errors.black— clean.mkdocs build— succeeds.DeviceFilter([PHONE, TABLET])→ audience model → back to an equalDeviceFilter.Usage:
🔗 Session: https://session-35b46b6a.poseidon.rapidata.internal/