Repository navigation
Conversation
atc964
left a comment
There was a problem hiding this comment.
The classifier is worth improving, but this infers a value the data already states, and infers it wrong. Every sample CSV carries an inventory_type column, csv_adapter attaches it into item.raw, and this function already reads raw. inv-ctv-sports-preroll declares ctv; this PR classifies it video.
That changes what we bill. rate_card_service._find_entry is an exact string match on type. Apex Premium Series currently matches the display entry at $12.00, clamps up to its $38.25 floor, and books at $38.25. Reclassified to video there is no matching entry, it falls through to catalog CPM, and books at $45.00. That is +17.6% on a live booked price, with no test covering it and no textual conflict for CI to catch.
Required before merge:
- Check
raw.get("inventory_type")before thead_formatsandsizesheuristics, returning it when present and recognised. The heuristics stay as the fallback for items that genuinely carry no type, which is the real gap. - A test over the shipped CTV sample asserting the pre-roll and mid-roll rows classify as
ctv. Six of ten rows inctv_streamingare the case in question. - A test pinning rate-card entry selection for a reclassified product, so a future type change that moves a booked price fails CI instead of shipping.
- Either justify the
0x0 sizes -> nativerule in the PR body or drop it. GAM out-of-page units report1x1, not0x0, so no real inventory was found that it fires on, and where it does fire it moves "Article Inline" into a package whose floor sits $2.00 below that row's own declared floor.
Getting the type from the column also corrects device targeting, currently [2, 4, 5] instead of [3, 7], and is_featured, currently false for six rows the CSV says are CTV.
|
@atc964 Thanks for catching this — you're right, and I confirmed all of it before pushing the fix. Verified independently: the CSV really does carry both Pushed
Also checked the two side effects you mentioned — Reverted just the fix and reran to confirm the new tests actually catch the regression (they do, with a clear message each time), then restored it. Full suite: 1544 passed. Ready for another look. |
atc964
left a comment
There was a problem hiding this comment.
The precedence fix is right: declared raw inventory_type wins, unrecognized values fall through to the heuristics instead of being trusted blindly, and the fall-through is pinned by a test. The rate-card selection test runs the real pipeline and the 0x0 -> native rule is dropped with a pinning test. Good.
Required before merge: _RECOGNISED_INVENTORY_TYPES rejects vocabulary our own shipped sample uses. data/csv/samples/aws_workshop/inventory.csv declares linear (3 rows), digital_video (3 rows), and audio (2 rows), so 8 of 15 rows have their declared type silently discarded. The three linear rows then split inconsistently: SportsPulse Live Broadcasts becomes linear_tv via the name keyword, while GNN Primetime News and Crestline Entertainment Block become video via the ad_formats fallback. The same sample's rate_card.json keys base_rates by linear, digital_video, and audio, which by this PR's own exact-string rationale would never match. Please either (a) add aliases or entries for the sample vocabulary, linear mapping to linear_tv at minimum, plus a decision on digital_video and audio, with a test over the aws_workshop rows, or (b) state explicitly in the docstring which declared vocabularies are in scope and log a warning when a declared value is discarded, so it fails loudly instead of silently.
Optional hardening: the six-row CTV test mirrors the sample rows with synthetic items. A variant that parses data/csv/samples/ctv_streaming/inventory.csv through CsvAdServerClient would also catch drift in the shipped file.
|
@atc964 Confirmed — One thing I checked that's worth stating explicitly: Went with (a), and specifically trusting the three values verbatim rather than aliasing them ( Pushed
Ready for another look. |
8da3fd1 to
e6584d3
Compare
atc964
left a comment
There was a problem hiding this comment.
Round 3: both mandated items from the last review are done and I verified them locally. All 55 tests in the two touched test files pass on this head; with main's classifier restored, all 9 aws_workshop tests fail, including the one that parses the real data/csv/samples/aws_workshop/inventory.csv through CSVAdServerClient. The verbatim design is also coherent on the pricing surface: RateCardEntry.inventory_type is a free string, so an operator can store entries keyed linear, digital_video, audio, and _find_entry's exact match now lines up with the sample's own rate_card.json vocabulary. Where no entry matches, products fall back to their own per-row CSV price. Good.
One thing remains, and it is the same class of defect this PR exists to remove: the three new strings are recognized by the classifier but unknown to the downstream type-to-attribute maps, which all fall to defaults silently.
Required before merge: entries for linear, digital_video, and audio in infer_deal_types. Today they fall to the [PREFERRED_DEAL] default, and proposal_handling_flow.py rejects any proposal whose deal type is not in supported_deal_types. Concretely: GNN.com Pre-Roll Video classified as video before this PR and accepted programmatic guaranteed; it now classifies as digital_video and a PG proposal is rejected. SportsPulse Live Broadcasts drops from PG+PD to PD only. That is a new, untested behavior change to live negotiation on shipped sample data. Suggest linear mirroring linear_tv, digital_video mirroring video, and an explicit decision for audio, plus a test pinning that a PG proposal on a digital_video row is accepted.
Strongly preferred in the same pass: matching entries in product_setup_flow's _estimate_base_cpm, _classify_ad_formats_from_type, and _classify_device_types_from_type. Today the synced linear package advertises base $10.00/floor $7.00 against rows whose declared floors run $22.00 to $32.00, tagged banner with device type [2], for what is TV, video, and audio inventory. Discovery-level rather than billing-level, so a follow-up issue is acceptable here, but the deal-types map is not severable from this PR's own correctness standard.
Everything else here is approve-quality.
|
@atc964 Confirmed — Pushed
On the strongly-preferred follow-up ( Ready for another look. |
atc964
left a comment
There was a problem hiding this comment.
Approving, thanks for the thorough follow-through on all three rounds. The declared-type-first design with the verbatim whitelist is the right shape: it fixes the aws_workshop vocabulary drop and the linear split in one move, and the real-CSV tests in test_classify_inventory_type.py mean drift in the sample files themselves will now fail loudly. I also checked the one classification flip outside the targeted rows ("Mobile Web Banner" moving from mobile_app to display because its declared value now wins) and it's both benign and more correct, and GAM/FreeWheel paths are untouched since they carry no raw.
Two optional nits, neither blocking: (1) the test class docstring (and commit message) say proposal_handling_flow rejects unsupported deal types, but line 156 only appends a warning; the real teeth are the PG availability gating in discovery, so a small wording fix would help future readers (that phrasing came from my review, so mea culpa). (2) Consider a debug log when a declared inventory_type is discarded as unrecognised; a silent discard is exactly how the aws_workshop gap stayed hidden in the first place.
classify_inventory_type only ever looked at item.name, keyword-matching
for ctv/video/native/mobile_app/linear_tv and defaulting straight to
"display" whenever the name didn't contain one of those substrings. Real
placements whose name carries no such keyword (e.g. "Article Inline", a
native unit, or "Sports Pre-Roll :15/:30", a video slot whose hyphenated
name doesn't match the unhyphenated "preroll" substring) silently fell
through to "display" even though their own ad_format/sizes said
otherwise.
Add a fallback, used only when the name gives zero signal: check
ad_format ("video"/"native") when available, then sizes as a last
resort (all-zero sizes read as native, matching the no-fixed-dimension
convention for native/audio formats). ad_format lives on item.raw,
which only the CSV adapter populates; GAM/FreeWheel items carry no raw
at all, so the fallback degrades to sizes-only for those without
raising. The existing name-based ladder runs first and returns
immediately on a match, so every already-correct classification is
unchanged — verified with a name-vs-ad_format conflict case (a
"Mobile App Interstitial" whose ad_formats includes "video") to guard
the ordering.
Verification: new unit coverage in test_classify_inventory_type.py
reproduces the real defect using the exact placements from
data/csv/samples/ (Article Inline, Sports Pre-Roll, Apex Premium
Series, etc.), confirmed to fail against the unfixed code and pass
with the fix; full unit (1590) and integration (39) suites pass with
zero regressions; ruff check/format clean; live-server verification
against the ctv_streaming sample catalog shows 7 placements moving
from the wrong "display" default to their correct type while the 2
genuinely-display placements are unchanged.
Addresses review feedback: the classifier ignored raw["inventory_type"] (the source's own declared type) and guessed from ad_formats instead, misclassifying CTV rows as "video" -- which silently changes rate-card pricing since _find_entry matches by exact inventory_type string. Now checks raw["inventory_type"] first, falling back to the name ladder and ad_formats/sizes only when no type is declared. Also drops the unjustified "0x0 sizes -> native" rule (no real row exercises it). Added tests pinning the 6 real ctv_streaming rows as "ctv" and the rate-card entry a reclassified product prices off of. Verified by reverting the fix: all 9 new/changed tests fail correctly. Full suite: 1544 passed.
…ng it linear/digital_video/audio aren't in the recognised set, so 8 of 15 rows in the aws_workshop sample had their declared inventory_type silently discarded and fell through inconsistently -- rows with the identical declared value ended up split between two different guessed types. Trusting these three values verbatim (not aliased) fixes the split and keeps the declared value exact, matching the sample's own rate_card.json keys if that ever gets wired into rate_card_service.
…defaulting infer_deal_types had no entries for the three declared types this PR trusts, so they fell to the [PREFERRED_DEAL]-only default -- a real behavior change on shipped sample data, since proposal_handling_flow rejects any proposal whose deal_type isn't in supported_deal_types. GNN.com Pre-Roll Video, for example, accepted programmatic guaranteed before this PR (as "video") and would have silently rejected it after (as "digital_video"). linear and digital_video mirror their closest existing canonical type (linear_tv, video). audio is a new decision: mapped the same as video since the sample data's audio rows (podcast sponsorships) are sold as premium placements, not open marketplace inventory.
…heir declared type
There was a problem hiding this comment.
Thanks @Kishore-2572. I checked out dbd41cc (rebased on main at bcc22d4) and ran it against all three sample datasets.
What I ran
- Unit: 1833 passed. Integration: 39 passed, 28 skipped.
ruff check src/andruff format --check src/ tests/clean. CI green. - The order-dependent failure is fixed.
agentcore/test_fastapi_background.pyfollowed bytest_inventory_type_override_applied.py(2 failures on the previous head) now passes, as does the AI-14 file alone and in reversed order. Picking a target type that differs from the product's own type is the right fix. - Classified every row of the three sample
inventory.csvfiles onmainand on this branch: 21 of 35 rows change, all to their declared type. Most are clear fixes. 10 rows that declarectvwere being served asdisplay. - FreeWheel is unaffected: its normalizer attaches no
raw, so it still classifies by name. GAM items have noraweither.
1. Synced packages for the new types fall back to desktop banner (blocking)
classify_inventory_type now returns linear, digital_video and audio verbatim, which matches the intent in 4d4b10d (keeping the sample's own rate_card.json keys). But ProductSetupFlow still maps types to package attributes with three tables that only know display / video / ctv / mobile_app / native / linear_tv: _classify_ad_formats_from_type, _classify_device_types_from_type and _estimate_base_cpm. Unknown types get ["banner"], devices [2] (PC only) and the fallback base price.
Syncing aws_workshop (AD_SERVER_TYPE=csv, default settings):
| Rows (declared type) | main |
this PR |
|---|---|---|
4 × ctv |
Display package: banner, [2, 4, 5], $12 |
Ctv: video, [3, 7], $35 |
3 × digital_video |
Video: video, [2, 4, 5], $25 |
Digital Video: banner, [2], $10 |
3 × linear |
1 in Linear Tv (video, [3, 7], $40); 2 in Display (banner, $12) |
Linear: banner, [2], $10 |
2 × audio |
Display: banner, $12 |
Audio: banner, [2], $10 |
The CTV rows are a clear improvement. But the digital_video rows regress from a video package to a desktop-only banner at the fallback price, and linear / audio land on the same fallback. The product-level classification is right; the package step just doesn't know the new spellings.
Suggested fix: add the three types to those three tables (linear like linear_tv, digital_video like video, audio as ["audio"] with [2, 4, 5, 6], matching media_kit_service._classify_ad_formats / _classify_device_types, which already know audio). Add a test that syncs aws_workshop and asserts no package with a declared type gets the banner / [2] fallback. The same tables are duplicated in media_kit_service.py (~635) and routers/media_kit.py (~495), which don't know linear or digital_video either, so they're worth updating in the same pass.
2. Changelog (non-blocking)
This moves which rate-card entry a CSV/S3 product resolves to (your new TestReclassificationChangesRateCardSelection pins exactly that), so existing CSV sellers will see prices change on upgrade. A line under Unreleased → Fixed would flag it.
3. Smaller notes
_RECOGNISED_INVENTORY_TYPESnow holds bothlinearandlinear_tv, and bothdigital_videoandvideo. Keeping them verbatim is defensible for rate-card matching. A short comment on the set saying the near-duplicates are intentional would help the next reader, since the per-type tables in item 1 are exactly where they get missed.- The
ad_formatsfallback only checksvideoandnative. A row withad_formats: ["audio"]and no declared type still becomesdisplay. Fine to leave if no data needs it.
…s, devices and base CPM ProductSetupFlow's type tables only knew the canonical spellings, so the declared linear/digital_video/audio types synced as desktop banner packages at the fallback price. Add them there and to the duplicated tables in media_kit_service and the media kit router. Test syncs the real aws_workshop CSV and pins each package's attributes. Also: changelog entry for the CSV price change on upgrade, a debug log when a declared type is discarded, and a docstring correction.
|
@Sirajmx, addressed in :
Also picked up @atc964's two optional nits: a debug log when a declared type is discarded, and the test docstring no longer says proposal_handling_flow rejects unsupported deal types. Unit tests pass on 3.11 and 3.12 (1,834 each), integration 39 passed, ruff clean. |
Sirajmx
left a comment
There was a problem hiding this comment.
Thanks @Kishore-2572, verified at a74cca3.
- Synced
aws_workshopwith default settings: Linearvideo/[3, 7]/$40, Digital Videovideo/[2, 4, 5]/$25, Audioaudio/[2, 4, 5, 6]/$15, CTVvideo/[3, 7]/$35. Nothing falls back to banner/desktop. - Your new sync test fails against the old tables ("Linear package fell back to defaults") and passes here.
- Unit 1834 passed, integration 39 passed, ruff clean. The previously order-dependent AI-14 tests pass in the polluting order.
Changelog and the near-duplicates comment look good. Approving.
The defect
classify_inventory_typeonly looked atitem.name, keyword-matching forctv/video/native/etc. and defaulting to"display"otherwise. Rowswhose name has no matching keyword got misclassified even when the source
data already stated the correct type — e.g.
ctv_streaming/inventory.csvrows like
"Sports Pre-Roll :15/:30"(hyphenated, so theprerollsubstring never matches) fell through to
"display"even though the CSV'sown
inventory_typecolumn says"ctv".This matters because
rate_card_service._find_entrymatches a rate cardentry by exact string equality on this value — a wrong guess silently
changes what a placement bills.
The fix
Check
raw["inventory_type"]first and trust it verbatim when it'srecognised, since it's declared by the source system, not guessed. Only
fall through to the name-keyword ladder, then
ad_formats/sizes, whennothing is declared. The name ladder still wins over
ad_formatswhenboth are present, so no prior "name wins" behavior changes.
Round 2, per @atc964's review — the first version fell back to
ad_formats/sizesinstead of the declared type, which got CTV rowswrong in a different way (
ad_formats=videopriced asvideo, notctv). Also dropped the0x0 sizes -> nativerule: no real inventoryexercises it, and the one row it used to catch (
"Article Inline")already declares
inventory_type=native.Round 3, per @atc964's review — the recognised set rejected
aws_workshop/inventory.csv's own vocabulary (linear/digital_video/audio), so 8 of 15 rows had their declared value discarded and fellthrough inconsistently (same declared value, different guessed
results depending on the row's name). Added all three as recognised,
trusted verbatim rather than aliased to an existing type, so a declared
value always round-trips to an identical rate-card key. That then
exposed a second gap:
infer_deal_typeshad no entries for the three,so they fell to a
[PREFERRED_DEAL]-only default — a real behaviorchange, since e.g.
digital_videorows would have silently stoppedaccepting programmatic-guaranteed proposals they accepted before this
PR (as
video). Added entries mirroring each type's closest canonicalcounterpart (
linear→linear_tv,digital_video→video);audioisa fresh decision, mapped the same as
videosince the sample data'saudio rows are premium sponsorship placements, not open marketplace
inventory.
Verification
test_classify_inventory_type.py: every row in all 3 shipped CSVsamples (
ctv_streaming,web_display,aws_workshop— 35 rowstotal) verified to resolve its declared type verbatim, including
direct parses of the real shipped CSV files, not just synthetic items.
test_rate_card_pricing.py: runs the real production pipeline andpins the resolved price off the correct rate-card entry for a
reclassified product.
linear/digital_video/audio,including against the real shipped
aws_workshoprows.fails the new tests it added with a clear message; restored, all
green.
ruff check/ruff format --check: clean.buyer-agentdoesn't branch on a specific product'sinventory_type/name, so unaffected.Follow-up filed separately, not in this PR (discovery-level, not
billing-level, per @atc964's own scoping): three independently
duplicated type-to-attribute maps (
product_setup_flow.py,engines/media_kit_service.py,routers/media_kit.py) have alreadydrifted from each other and don't all cover every recognised type.
Test plan
uv run pytest tests/unit -quv run ruff check/ruff format --check