image repair on existing products
This commit is contained in:
@@ -539,6 +539,7 @@ def stage_6_images(row: Dict[str, Any], *, enabled: bool = True) -> Dict[str, An
|
||||
return row
|
||||
try:
|
||||
from app.core.catalog_engine import catalog_engine
|
||||
from app.services import produce_reference
|
||||
from app.services.image_search import find_all_image_urls
|
||||
|
||||
# "Own Products" is a bucket, not a brand, so it must not enter the
|
||||
@@ -547,15 +548,41 @@ def stage_6_images(row: Dict[str, Any], *, enabled: bool = True) -> Dict[str, An
|
||||
# name, and _select_best_images derives its distinctive tokens from the
|
||||
# whole name ({toor, dal}), which is exactly right for a commodity.
|
||||
brand = row.get("brand") or ""
|
||||
if brand == OWN_PRODUCTS_BRAND:
|
||||
is_produce = brand == OWN_PRODUCTS_BRAND
|
||||
if is_produce:
|
||||
brand = ""
|
||||
|
||||
product_name = row.get("product_name") or ""
|
||||
|
||||
# A reviewed photo beats anything a search can find, so take it and stop.
|
||||
#
|
||||
# NOT merged into the candidate list below: _select_best_images ranks by
|
||||
# whether the URL text names the product, and a Commons filename names
|
||||
# the cultivar ("Honeycrisp.jpg" for Apple). The curated image would
|
||||
# score zero relevance, sort below any searched URL containing "apple",
|
||||
# and index 0 - the one that becomes image_url - would still be wrong.
|
||||
#
|
||||
# Only the bucket may ask. `produce_reference.lookup` falls back through
|
||||
# shorter leading prefixes, so "Apple Cider Vinegar" would resolve to
|
||||
# "apple" and be handed a photo of fruit.
|
||||
if is_produce:
|
||||
curated = produce_reference.image_url(product_name)
|
||||
if curated:
|
||||
row["image_url"] = curated
|
||||
row["image_urls"] = [curated]
|
||||
return row
|
||||
|
||||
# produce=True suppresses the commodity-hint table (which turns "Apple"
|
||||
# into "Apple fruit juice"), skips the packaged-goods databases, and
|
||||
# drops the "-plant -tree -fish -botanical" Wikimedia exclusion block
|
||||
# that fights every produce query. Without it this stage recreates the
|
||||
# exact defect the Own Products image repair exists to undo.
|
||||
candidates = find_all_image_urls(
|
||||
row.get("product_name") or "", brand=brand or None, max_results=24
|
||||
product_name, brand=brand or None, max_results=24, produce=is_produce
|
||||
)
|
||||
if candidates:
|
||||
best = catalog_engine._select_best_images(
|
||||
candidates, row.get("product_name") or "", brand, max_images=10
|
||||
candidates, product_name, brand, max_images=10
|
||||
)
|
||||
if best:
|
||||
row["image_urls"] = list(best)
|
||||
|
||||
@@ -126,9 +126,46 @@ def usda_fdc_id(product_name: str) -> Optional[int]:
|
||||
return entry.get("usda_fdc_id") if entry else None
|
||||
|
||||
|
||||
# Words that make a name a DIFFERENT PRODUCT from the commodity it starts with,
|
||||
# rather than a variety of it.
|
||||
#
|
||||
# `lookup` falls back through shorter leading prefixes, which is what lets
|
||||
# "Mango Totapuri" and "Banana Robusta" find their base commodity - varieties of
|
||||
# the same thing, correctly sharing one photo. The same fallback turns "Coconut
|
||||
# Oil" into "Coconut" and "Apple Cider Vinegar" into "Apple", and a bottle of
|
||||
# oil is not a variety of coconut. Handing it a photo of the raw fruit is the
|
||||
# same class of error as the apple-juice image this table was built to fix, just
|
||||
# pointing the other way.
|
||||
#
|
||||
# No row in the catalogue trips this today - all 159 names are exact keys - so
|
||||
# this guards the case a future upload introduces, which is precisely when
|
||||
# nobody would be looking.
|
||||
_DERIVED_PRODUCT_WORDS = frozenset({
|
||||
"oil", "vinegar", "juice", "squash", "syrup", "jam", "jelly", "sauce",
|
||||
"ketchup", "puree", "paste", "pickle", "powder", "flour", "atta", "rava",
|
||||
"flakes", "chips", "crisps", "candy", "extract", "essence", "milkshake",
|
||||
"smoothie", "dried", "fried", "roasted", "pickled", "canned", "frozen",
|
||||
})
|
||||
|
||||
|
||||
def image_url(product_name: str) -> Optional[str]:
|
||||
"""The reviewed photo for a commodity, or None.
|
||||
|
||||
Stricter than `lookup` on purpose, and only here: `usda_fdc_id` keeps the
|
||||
plain prefix fallback, because this guard is about not showing a misleading
|
||||
PICTURE. Nutrition for a derived product is refused further upstream by the
|
||||
category dispatch in `nutrition_data_service`.
|
||||
"""
|
||||
entry = lookup(product_name)
|
||||
return entry.get("image_url") if entry else None
|
||||
if not entry:
|
||||
return None
|
||||
|
||||
matched = _normalize(entry.get("product_name") or "")
|
||||
leftover = set(_normalize(product_name).split()) - set(matched.split())
|
||||
if leftover & _DERIVED_PRODUCT_WORDS:
|
||||
return None
|
||||
|
||||
return entry.get("image_url")
|
||||
|
||||
|
||||
def all_entries() -> Dict[str, Dict[str, Any]]:
|
||||
|
||||
@@ -76,6 +76,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
|
||||
from app.infrastructure.settings import DB_HOST, DB_NAME # noqa: E402
|
||||
from app.services.brand_registry import resolve_parent_brand # noqa: E402
|
||||
from app.services.generic_products import OWN_PRODUCTS_BRAND # noqa: E402
|
||||
from app.services import produce_reference # noqa: E402
|
||||
from app.services.vector_store import ( # noqa: E402
|
||||
_connect,
|
||||
_list_brand_table_suffixes,
|
||||
@@ -440,18 +441,56 @@ def _is_own_products(brand: str) -> bool:
|
||||
return _sanitize_name(brand or "") == _sanitize_name(OWN_PRODUCTS_BRAND)
|
||||
|
||||
|
||||
def _search_replacement(product_name: str, brand: str, max_results: int) -> List[str]:
|
||||
"""Validated image URLs for one product, best first.
|
||||
def _search_replacement(product_name: str, brand: str, max_results: int,
|
||||
timeout: int = 8) -> List[str]:
|
||||
"""Image URLs for one product, best first.
|
||||
|
||||
find_all_image_urls validates every candidate internally, so what comes back
|
||||
is live by construction. Ranking then reuses the same _select_best_images
|
||||
the ingestion pipeline uses, so a repaired row is ranked by the rules
|
||||
For a loose commodity in the Own Products bucket the answer is already known
|
||||
and already reviewed - see the curated branch below. Everything else is
|
||||
searched, then ranked with the same _select_best_images the ingestion
|
||||
pipeline uses, so a repaired row is ranked by the rules
|
||||
tests/test_image_selection.py already pins.
|
||||
|
||||
On the searched path `find_all_image_urls(validate=True)` probes each
|
||||
candidate, but it falls back to returning UNVALIDATED candidates when every
|
||||
probe fails (image_search.py, the "validation killed everything" branch). So
|
||||
a URL from here is usually live, not live by construction.
|
||||
"""
|
||||
key = _search_key(product_name, brand)
|
||||
if key in _SEARCH_CACHE:
|
||||
return _SEARCH_CACHE[key]
|
||||
|
||||
# THE REVIEWED IMAGE WINS, AND IT HAS TO SHORT-CIRCUIT TO DO SO.
|
||||
#
|
||||
# These photos were picked by hand and signed off on a contact sheet.
|
||||
# Sending one through the rest of this function destroys it twice over. The
|
||||
# `_names_product` corroboration gate at the end keeps only URLs containing
|
||||
# a distinctive word of the product title, and the reviewed Commons photo
|
||||
# for "Apple" is `Honeycrisp.jpg` - a Commons filename names the cultivar,
|
||||
# not the catalogue word, so the CORRECT image is precisely the one that
|
||||
# gets rejected. Even past that, ranking sorts any searched URL containing
|
||||
# "apple" above it, and index 0 is what becomes `image_url`.
|
||||
#
|
||||
# Gating on the bucket is not a shortcut either: `produce_reference.lookup`
|
||||
# walks shorter and shorter leading prefixes, so "Apple Cider Vinegar" misses
|
||||
# twice and then hits "apple". This branch must never see another brand's
|
||||
# rows.
|
||||
if _is_own_products(brand):
|
||||
curated = produce_reference.image_url(product_name)
|
||||
if curated:
|
||||
# A failed probe WARNS and keeps the URL, rather than falling back
|
||||
# to a search. upload.wikimedia.org throttles a fast sweep and
|
||||
# validate_image_url_live treats 429 exactly like 404, so a
|
||||
# transient rate-limit would otherwise swap a reviewed photo for a
|
||||
# searched one - silently, and at scale. That is the exact failure
|
||||
# this table exists to prevent. A genuinely dead URL is a fix in
|
||||
# produce_reference.json, and this warning is what asks for it.
|
||||
if not _url_is_live(curated, timeout):
|
||||
logger.warning(" curated image did not verify, using anyway "
|
||||
"%s -> %s", product_name[:38], curated[:70])
|
||||
_SEARCH_CACHE[key] = [curated]
|
||||
return [curated]
|
||||
|
||||
from app.services.image_search import find_all_image_urls
|
||||
|
||||
# `stage_6_images` already blanks the brand for this bucket
|
||||
@@ -581,7 +620,7 @@ def repair(cur, conn, brands: List[str], apply: bool, timeout: int, workers: int
|
||||
totals["unrepairable"] += 1
|
||||
continue
|
||||
|
||||
found = _search_replacement(name, suffix, max_results)
|
||||
found = _search_replacement(name, suffix, max_results, timeout)
|
||||
if not found:
|
||||
logger.info(" no replacement found %s", name[:58])
|
||||
unrepairable.append(f"{suffix}/{row['image_id']} ({name})")
|
||||
|
||||
@@ -161,3 +161,237 @@ def test_the_bucket_is_recognised_under_either_spelling():
|
||||
assert _is_own_products("own_products") is True
|
||||
assert _is_own_products("Own Products") is True
|
||||
assert _is_own_products("Amul") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The curated table reaches the two write paths
|
||||
# ---------------------------------------------------------------------------
|
||||
# Building 159 reviewed images and then not wiring them in is the failure this
|
||||
# section exists to prevent. Until these tests existed, `produce_reference.
|
||||
# image_url` had NO runtime consumer at all: the repair script re-searched every
|
||||
# row, and the ingestion pipeline never even passed `produce=True`. So the
|
||||
# contact sheet showed one set of images and the code would have written another.
|
||||
|
||||
CURATED_COMMODITY = "Apple"
|
||||
UNCURATED_COMMODITY = "Avocado" # genuinely absent from produce_reference.json
|
||||
BUCKET = "own_products" # repair() passes the TABLE SUFFIX, not a name
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def repair_module(monkeypatch):
|
||||
"""The repair script with its process-lifetime search cache emptied.
|
||||
|
||||
`_SEARCH_CACHE` is module-level and never cleared, so without this the first
|
||||
test to run would answer for every test after it.
|
||||
"""
|
||||
from scripts import repair_brand_images
|
||||
|
||||
monkeypatch.setattr(repair_brand_images, "_SEARCH_CACHE", {})
|
||||
monkeypatch.setattr(repair_brand_images, "_url_is_live", lambda url, timeout: True)
|
||||
return repair_brand_images
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def no_search(monkeypatch):
|
||||
"""Record calls to the search, and fail loudly if one was not expected."""
|
||||
calls = []
|
||||
|
||||
def _fake(title, brand=None, country_hint=None, validate=True,
|
||||
max_results=24, produce=False):
|
||||
calls.append({"title": title, "brand": brand, "produce": produce})
|
||||
return []
|
||||
|
||||
monkeypatch.setattr(image_search, "find_all_image_urls", _fake)
|
||||
return calls
|
||||
|
||||
|
||||
def test_the_repair_returns_the_reviewed_image(repair_module, no_search):
|
||||
from app.services import produce_reference
|
||||
|
||||
found = repair_module._search_replacement(CURATED_COMMODITY, BUCKET, 8)
|
||||
|
||||
assert found == [produce_reference.image_url(CURATED_COMMODITY)]
|
||||
|
||||
|
||||
def test_the_repair_does_not_search_when_it_already_has_the_answer(
|
||||
repair_module, no_search):
|
||||
"""The short-circuit is the behaviour, not an optimisation.
|
||||
|
||||
A curated URL that went through the rest of `_search_replacement` would be
|
||||
thrown away by the `_names_product` corroboration gate: the reviewed Commons
|
||||
photo for Apple is `Honeycrisp.jpg`, and a Commons filename names the
|
||||
cultivar, not the catalogue word.
|
||||
"""
|
||||
repair_module._search_replacement(CURATED_COMMODITY, BUCKET, 8)
|
||||
|
||||
assert no_search == [], "the curated branch must return before searching"
|
||||
|
||||
|
||||
def test_a_pack_size_still_finds_the_commodity(repair_module, no_search):
|
||||
from app.services import produce_reference
|
||||
|
||||
assert repair_module._search_replacement("Apple 1kg", BUCKET, 8) == [
|
||||
produce_reference.image_url(CURATED_COMMODITY)]
|
||||
assert no_search == []
|
||||
|
||||
|
||||
def test_a_variety_shares_its_commodity_image(repair_module, no_search):
|
||||
"""`Mango Totapuri` and `Mango Alphonso` are the same fruit."""
|
||||
from app.services import produce_reference
|
||||
|
||||
assert repair_module._search_replacement("Mango Totapuri 1kg", BUCKET, 8) == [
|
||||
produce_reference.image_url("Mango")]
|
||||
|
||||
|
||||
def test_an_uncurated_commodity_falls_through_to_a_produce_search(
|
||||
repair_module, no_search):
|
||||
repair_module._search_replacement(UNCURATED_COMMODITY, BUCKET, 8)
|
||||
|
||||
assert len(no_search) == 1
|
||||
assert no_search[0]["produce"] is True
|
||||
assert no_search[0]["brand"] == "", "the bucket name must not enter the query"
|
||||
|
||||
|
||||
def test_a_branded_product_never_consults_the_curated_table(
|
||||
repair_module, no_search):
|
||||
"""`lookup` falls back through shorter leading prefixes, so asking it about
|
||||
every brand's rows would put a photo of fruit on Apple-branded anything."""
|
||||
repair_module._search_replacement("Britannia Good Day 200g", "britannia", 8)
|
||||
|
||||
assert len(no_search) == 1
|
||||
assert no_search[0]["produce"] is False
|
||||
|
||||
|
||||
def test_a_curated_url_that_fails_its_probe_is_used_anyway(
|
||||
repair_module, no_search, monkeypatch, caplog):
|
||||
"""upload.wikimedia.org throttles a fast sweep and the probe treats 429
|
||||
exactly like 404. Falling back to a search here would let a transient
|
||||
rate-limit quietly replace reviewed photos with searched ones."""
|
||||
from app.services import produce_reference
|
||||
|
||||
monkeypatch.setattr(repair_module, "_url_is_live", lambda url, timeout: False)
|
||||
|
||||
with caplog.at_level("WARNING"):
|
||||
found = repair_module._search_replacement(CURATED_COMMODITY, BUCKET, 8)
|
||||
|
||||
assert found == [produce_reference.image_url(CURATED_COMMODITY)]
|
||||
assert no_search == [], "a failed probe must not trigger a search"
|
||||
assert "did not verify" in caplog.text
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The derived-product guard
|
||||
# ---------------------------------------------------------------------------
|
||||
@pytest.mark.parametrize("name", [
|
||||
"Coconut Oil", "Apple Cider Vinegar", "Tomato Ketchup", "Corn Flour",
|
||||
])
|
||||
def test_a_derived_product_does_not_inherit_the_raw_commodity_photo(name):
|
||||
"""The prefix fallback that correctly resolves `Mango Totapuri` to `Mango`
|
||||
also resolves `Coconut Oil` to `Coconut`. A bottle of oil is not a variety
|
||||
of coconut, and showing the raw fruit for it is the apple-juice error
|
||||
pointing the other way."""
|
||||
from app.services import produce_reference
|
||||
|
||||
assert produce_reference.image_url(name) is None
|
||||
|
||||
|
||||
def test_the_guard_does_not_touch_nutrition_matching():
|
||||
"""`usda_fdc_id` keeps the plain fallback on purpose - the guard is about
|
||||
not showing a misleading picture, and nutrition has its own dispatch."""
|
||||
from app.services import produce_reference
|
||||
|
||||
assert produce_reference.usda_fdc_id("Apple 1kg") == 171688
|
||||
assert produce_reference.usda_fdc_id("Coconut Oil") is not None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Stage 6 of the ingestion pipeline
|
||||
# ---------------------------------------------------------------------------
|
||||
@pytest.fixture
|
||||
def stage_6(monkeypatch):
|
||||
"""`stage_6_images` plus a recorder on the ranking it must not reach."""
|
||||
from app.core import catalog_engine as catalog_engine_module
|
||||
from app.core import store_catalog_pipeline
|
||||
|
||||
ranked = []
|
||||
|
||||
def _fake_rank(urls, title, brand, max_images=20):
|
||||
ranked.append(title)
|
||||
return list(urls)
|
||||
|
||||
monkeypatch.setattr(catalog_engine_module.catalog_engine,
|
||||
"_select_best_images", _fake_rank)
|
||||
return store_catalog_pipeline, ranked
|
||||
|
||||
|
||||
def _produce_row(name):
|
||||
from app.services.generic_products import OWN_PRODUCTS_BRAND
|
||||
return {"product_name": name, "brand": OWN_PRODUCTS_BRAND}
|
||||
|
||||
|
||||
def test_an_uploaded_commodity_gets_the_reviewed_image(stage_6, no_search):
|
||||
from app.services import produce_reference
|
||||
pipeline, ranked = stage_6
|
||||
|
||||
row = pipeline.stage_6_images(_produce_row(CURATED_COMMODITY))
|
||||
|
||||
expected = produce_reference.image_url(CURATED_COMMODITY)
|
||||
assert row["image_url"] == expected
|
||||
assert row["image_urls"] == [expected]
|
||||
assert no_search == []
|
||||
|
||||
|
||||
def test_the_reviewed_image_is_not_put_through_the_ranking(stage_6, no_search):
|
||||
"""`_select_best_images` scores a URL by whether it names the product, so
|
||||
`Honeycrisp.jpg` scores zero and any searched URL containing "apple" sorts
|
||||
above it. Index 0 is what becomes `image_url`, so merging rather than
|
||||
short-circuiting would leave the wrong image on the card."""
|
||||
pipeline, ranked = stage_6
|
||||
|
||||
pipeline.stage_6_images(_produce_row(CURATED_COMMODITY))
|
||||
|
||||
assert ranked == []
|
||||
|
||||
|
||||
def test_uploaded_produce_with_no_curated_entry_still_gets_produce_mode(
|
||||
stage_6, no_search):
|
||||
"""The fix that was missing entirely: this stage never passed `produce=`, so
|
||||
every new commodity was searched against the packaged-goods databases with
|
||||
the "-plant -tree -fish" exclusion block that fights produce queries."""
|
||||
pipeline, ranked = stage_6
|
||||
|
||||
pipeline.stage_6_images(_produce_row(UNCURATED_COMMODITY))
|
||||
|
||||
assert len(no_search) == 1
|
||||
assert no_search[0]["produce"] is True
|
||||
assert no_search[0]["brand"] is None, "the bucket must not enter the query"
|
||||
|
||||
|
||||
def test_an_uploaded_branded_product_is_unchanged(stage_6, no_search):
|
||||
pipeline, ranked = stage_6
|
||||
|
||||
pipeline.stage_6_images({"product_name": "Britannia Good Day 200g",
|
||||
"brand": "Britannia"})
|
||||
|
||||
assert len(no_search) == 1
|
||||
assert no_search[0]["produce"] is False
|
||||
assert no_search[0]["brand"] == "Britannia"
|
||||
|
||||
|
||||
def test_a_row_that_already_has_images_is_left_alone(stage_6, no_search):
|
||||
pipeline, ranked = stage_6
|
||||
row = dict(_produce_row(CURATED_COMMODITY),
|
||||
image_urls=["https://example.com/already.jpg"])
|
||||
|
||||
assert pipeline.stage_6_images(row)["image_urls"] == [
|
||||
"https://example.com/already.jpg"]
|
||||
assert no_search == []
|
||||
|
||||
|
||||
def test_images_can_still_be_switched_off(stage_6, no_search):
|
||||
pipeline, ranked = stage_6
|
||||
|
||||
row = pipeline.stage_6_images(_produce_row(CURATED_COMMODITY), enabled=False)
|
||||
|
||||
assert "image_url" not in row
|
||||
assert no_search == []
|
||||
|
||||
Reference in New Issue
Block a user