Skip to content

fix: classify inventory by ad_format/sizes when the name gives no signal - #105

Merged
Sirajmx merged 6 commits into
mainfrom
AI-15
Oct 28, 2026
Merged

Sirajmx merged 6 commits into
mainfrom
AI-15

Conversation

@Kishore-2572

@Kishore-2572 Kishore-2572 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

The defect

classify_inventory_type only looked at item.name, keyword-matching for
ctv/video/native/etc. and defaulting to "display" otherwise. Rows
whose name has no matching keyword got misclassified even when the source
data already stated the correct type — e.g. ctv_streaming/inventory.csv
rows like "Sports Pre-Roll :15/:30" (hyphenated, so the preroll
substring never matches) fell through to "display" even though the CSV's
own inventory_type column says "ctv".

This matters because rate_card_service._find_entry matches a rate card
entry 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's
recognised, since it's declared by the source system, not guessed. Only
fall through to the name-keyword ladder, then ad_formats/sizes, when
nothing is declared. The name ladder still wins over ad_formats when
both are present, so no prior "name wins" behavior changes.

Round 2, per @atc964's review — the first version fell back to
ad_formats/sizes instead of the declared type, which got CTV rows
wrong in a different way (ad_formats=video priced as video, not
ctv). Also dropped the 0x0 sizes -> native rule: no real inventory
exercises 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 fell
through 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_types had no entries for the three,
so they fell to a [PREFERRED_DEAL]-only default — a real behavior
change, since e.g. digital_video rows would have silently stopped
accepting programmatic-guaranteed proposals they accepted before this
PR (as video). Added entries mirroring each type's closest canonical
counterpart (linear→linear_tv, digital_video→video); audio is
a fresh decision, mapped the same as video since the sample data's
audio rows are premium sponsorship placements, not open marketplace
inventory.

Verification

  • test_classify_inventory_type.py: every row in all 3 shipped CSV
    samples (ctv_streaming, web_display, aws_workshop — 35 rows
    total) 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 and
    pins the resolved price off the correct rate-card entry for a
    reclassified product.
  • Deal-type coverage pinned for linear/digital_video/audio,
    including against the real shipped aws_workshop rows.
  • Negative control at every round: reverting just that round's fix
    fails the new tests it added with a clear message; restored, all
    green.
  • Full unit suite: 1557 passed, 0 regressions. ruff check / ruff format --check: clean.
  • Cross-repo: buyer-agent doesn't branch on a specific product's
    inventory_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 already
drifted from each other and don't all cover every recognised type.

Test plan

  • uv run pytest tests/unit -q
  • uv run ruff check / ruff format --check
  • Negative control at every round (revert -> fail, restore -> pass)
  • Cross-repo check against buyer-agent

@Kishore-2572
Kishore-2572 marked this pull request as ready for review September 21, 2026 11:22
@Kishore-2572
Kishore-2572 requested a review from atc964 September 21, 2026 11:40

@atc964 atc964 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Check raw.get("inventory_type") before the ad_formats and sizes heuristics, returning it when present and recognised. The heuristics stay as the fallback for items that genuinely carry no type, which is the real gap.
  2. A test over the shipped CTV sample asserting the pre-roll and mid-roll rows classify as ctv. Six of ten rows in ctv_streaming are the case in question.
  3. 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.
  4. Either justify the 0x0 sizes -> native rule in the PR body or drop it. GAM out-of-page units report 1x1, not 0x0, 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.

@Kishore-2572

Copy link
Copy Markdown
Contributor Author

@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 inventory_type=ctv and ad_formats=video on inv-ctv-sports-preroll, the adapter really does attach inventory_type into raw alongside ad_formats, and _find_entry's exact-match means my fallback really would have silently repriced it. Good catch.

Pushed 69a7997 addressing all four points:

  1. raw["inventory_type"] is now checked first, trusted when it's one of the six recognised types. The name ladder and ad_formats/sizes stay as fallback for items with no declared type at all.
  2. Added a parametrized test over all 6 affected rows in ctv_streaming/inventory.csv, asserting each classifies as ctv, not video.
  3. Added a test that runs the real pipeline (classify_inventory_type → product_from_inventory_item) and pins the resolved price off a stored rate card, not just the label — so a future classifier change that moves a booked price fails here.
  4. Dropped the 0x0 sizes → native rule rather than justifying it. Checked the sample data myself: "Article Inline" (the only row it ever fired on) already declares inventory_type=native in its own CSV column, so it's handled correctly by Fix NameError crash: move helper functions before first use in Depends() #1 now — the rule was dead weight once that's in place.

Also checked the two side effects you mentioned — is_featured and device targeting both come out correct automatically now, since both derive from classify_inventory_type's return value in product_setup_flow.py.

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 atc964 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Kishore-2572

Copy link
Copy Markdown
Contributor Author

@atc964 Confirmed — _RECOGNISED_INVENTORY_TYPES really did discard linear/digital_video/audio, and the three linear rows really did split inconsistently exactly as you describe (SportsPulse Live Broadcasts via the name keyword vs. the other two via the ad_formats=video fallback).

One thing I checked that's worth stating explicitly: data/csv/samples/aws_workshop/rate_card.json isn't actually wired into rate_card_service today — get_rate_card_entries() reads from operator-set storage (update_rate_card), not this file. So the pricing consequence you describe is the shape of the risk if that file's values are ever loaded via update_rate_card, not something currently live. Doesn't change the fix — the inconsistent-split bug is real regardless of whether this particular file is wired up.

Went with (a), and specifically trusting the three values verbatim rather than aliasing them (linear→linear_tv, etc.) — aliasing would have fixed the internal inconsistency but still left a mismatch against rate_card.json's own keys (linear, not linear_tv). Trusting verbatim fixes both: no more split between siblings, and if rate_card.json's vocabulary is ever loaded into the live rate card, an exact-string match against it would actually work.

Pushed e749504:

  • _RECOGNISED_INVENTORY_TYPES now also includes linear, digital_video, audio.
  • New test class covers all 8 affected rows, including one that parses the real shipped aws_workshop/inventory.csv through CSVAdServerClient (not synthetic items), so drift in the sample file itself would fail the test.
  • Negative control: reverted just this change, all 9 new tests failed with a clear message naming the mismatch; restored, green.
  • Checked the two downstream consumers you'd expect to care (infer_deal_types, _classify_device_types_from_type) — both already .get(..., default) safely for an unrecognised key, so no crash risk for these new values; they just fall to the same sensible default any other type would.
  • Full suite: 1553 passed. Lint/format clean.

Ready for another look.

@Kishore-2572
Kishore-2572 force-pushed the AI-15 branch 2 times, most recently from 8da3fd1 to e6584d3 Compare September 23, 2026 09:31
@Kishore-2572
Kishore-2572 requested a review from atc964 September 24, 2026 07:07

@atc964 atc964 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Kishore-2572

Copy link
Copy Markdown
Contributor Author

@atc964 Confirmed — infer_deal_types had no entries for linear/digital_video/audio, so they fell to the [PREFERRED_DEAL]-only default, and proposal_handling_flow.py:156 does exactly what you describe: rejects any proposal whose deal_type isn't in product.supported_deal_types. Verified the specific regressions you named — GNN.com Pre-Roll Video and SportsPulse Live Broadcasts both lose PG support exactly as you traced.

Pushed 3ef46c8:

  • linear and digital_video mirror their closest canonical type (linear_tv, video) as you suggested.
  • audio: mapped the same as video ([PROGRAMMATIC_GUARANTEED, PREFERRED_DEAL]). The sample data's audio rows are podcast sponsorships — premium, typically sold as guaranteed placements rather than open marketplace inventory — so PG-eligible seemed like the right default rather than falling back to the narrower native-style treatment.
  • New test class pins PG acceptance for all three types, including one that parses the real shipped aws_workshop/inventory.csv (not synthetic items), so this can't silently regress again.
  • Negative control: reverted just this change, all 4 new tests fail with a clear message naming the lost deal type; restored, green.
  • Full suite: 1557 passed. Lint/format clean.

On the strongly-preferred follow-up (_estimate_base_cpm, _classify_ad_formats_from_type, _classify_device_types_from_type) — agreed this is real and out of scope here. For what it's worth, I'd independently found the same gap in my own adversarial pass on this PR before your round 3 landed: there are actually three separate duplicated type-to-attribute maps across the codebase (product_setup_flow.py, engines/media_kit_service.py, and routers/media_kit.py), and they'd already drifted from each other before this PR touched anything (linear_tv itself is missing from one of the three). Happy to file that as a tracked follow-up issue covering all three maps at once rather than patching them one at a time, if that's useful — let me know.

Ready for another look.

@Kishore-2572
Kishore-2572 requested a review from atc964 October 5, 2026 05:16
atc964
atc964 previously approved these changes Oct 27, 2026

@atc964 atc964 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Sirajmx Sirajmx left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ and ruff format --check src/ tests/ clean. CI green.
  • The order-dependent failure is fixed. agentcore/test_fastapi_background.py followed by test_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.csv files on main and on this branch: 21 of 35 rows change, all to their declared type. Most are clear fixes. 10 rows that declare ctv were being served as display.
  • FreeWheel is unaffected: its normalizer attaches no raw, so it still classifies by name. GAM items have no raw either.

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_TYPES now holds both linear and linear_tv, and both digital_video and video. 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_formats fallback only checks video and native. A row with ad_formats: ["audio"] and no declared type still becomes display. 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.
@Kishore-2572

Copy link
Copy Markdown
Contributor Author

@Sirajmx, addressed in :

  1. linear, digital_video and audio are now in all three ProductSetupFlow tables (linear like linear_tv, digital_video like video, audio as ["audio"]/[2, 4, 5, 6] at $15), and in the duplicated tables in media_kit_service.py and routers/media_kit.py (which also gain linear_tv). New test syncs the real aws_workshop/inventory.csv and asserts the Linear, Digital Video, Audio and CTV packages get their own formats, devices and base CPM; with the old tables it fails on Linear package fell back to defaults: ['banner'], [2].
  2. Added a CHANGELOG line under Unreleased → Fixed flagging that CSV prices can change on upgrade.
  3. Added a comment on _RECOGNISED_INVENTORY_TYPES noting the near-duplicates are intentional. Left the ad_formats: ["audio"] fallback as is, since no sample data needs it.

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 Sirajmx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Kishore-2572, verified at a74cca3.

  • Synced aws_workshop with default settings: Linear video/[3, 7]/$40, Digital Video video/[2, 4, 5]/$25, Audio audio/[2, 4, 5, 6]/$15, CTV video/[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.

@Sirajmx
Sirajmx merged commit 902e3f5 into main Oct 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants