feat: add SerpAPI dataset source by pythoryn · Pull Request #312 · forecastingresearch/forecastbench

pythoryn · 2026-09-28T03:52:28Z

Adds a SerpAPI dataset source with 150 configured questions covering Amazon
prices, Walmart food and drink prices, and flight departure delays.

Includes daily collection, persistent observation histories, question curation,
resolution, and Prophet baseline forecasts.

Retail prices require matching products and verified retailer sellers.
Daily snapshots allow a seven-day fallback for missing comparison dates,
with baselines selected separately for each horizon.
Flight delays require an exact date, flight, and route match and confirmed
departure. Resolution compares the target delay with the preceding 14-day
median, counting early/on-time departures as zero and omitting missing days.
Question-specific resolution criteria are preserved during curation.
Horizons lacking required measurements remain unresolved and unscored.

Deployment prerequisite: Before the first nightly run that includes SerpAPI, create an empty 'serpapi_questions.jsonl'
at the root of the production question-bank bucket:

Closes #49

houtanb

Looks great. Some small changes here. Need to look more in depth later, but this is something for now

nikbpetrov · 2026-09-28T11:42:33Z

I still need more time to dig in, but probably useful for you to address the above first.

I'd also recommend:

Get your favourite agent swarm to do a red-team code review. Feel free to use your favourite grilling/red-teaming skills, or just a "spin up 5-15 agents to red-team review this PR and collate the feedback" should be a good start. Ask your agents to review other sources implementation as well and compare - each deviation must be worth its keep.
On that note, I'm finding that I am spending a decent chunk of time retracing the logic behind some decisions. I trust that you have reasons why you put something where you put it but with these more compliated PRs, laying out some of the critical decisions you've had to make just makes me a tad faster during review (also helps my agents answer my Qs quickly rather than guess/infer, which sometimes sinks even more time). To be clear, this does not mean that the code is bad or unclear, it could very well be a tad of a skill issue on my part, but I hope you get the point.

pythoryn · 2026-09-29T03:10:07Z

@nikbpetrov
Thanks, that makes sense. I’ll expand the PR description with the rationale for the main differences.

houtanb

(This was posted by Fable 5.1 in my name)

@pythoryn I went through each comment below and elaborated on them, noting what seemed important.

Full review of the SerpAPI source at 42a0e92. Well-structured and thoroughly tested; nothing here is a hard blocker.

Main risk (1 comment): a systematic failure in one category (e.g. a payload shape change for one engine) is only logged per id. Every question in that category silently retires from curation and, after the fallback window, resolves to NaN. Only the all-requests-failed case raises.

Maintainability (3 comments): per-category behavior (date filling, snapshot fallback, flight handling) is hardcoded in four places instead of on the spec, with one latent divergence between update() and _resolve(); growth-threshold arithmetic is implemented twice; the update job forks three _source_io helpers in a way that will drop fetch_datetime if anyone later unifies them.

Minor (5 comments): secret access pattern, a test that pins private call shapes, an O(n²) loop in _resolve, a dead explanation string, and an unused browser_url() helper.

Generated by Claude Code

houtanb · 2026-09-29T19:08:08Z

+        snapshot_names = {
+            "amazon_minimum_product_price",
+            "walmart_food_drink_price",
+            "youtube_channel_max_video_views",
+        }
+        assert all(
+            name in snapshot_names
+            for name, spec in QUESTION_SPECS.items()
+            if spec["engine"] in TODAY_ONLY_ENGINES
+        ), "Every today-only question category must have snapshot resolution support."
+        dfr = dfr[["id", "date", "value"]].copy()
+        numeric = pd.to_numeric(dfr["value"], errors="coerce")
+        dfr["value"] = numeric.where(np.isfinite(numeric), np.nan)
+        # Finance histories retain raw observations and missing values. Fill only
+        # for resolution, after all saved and refreshed observations were merged.
+        finance = dfr["id"].str.startswith("google_finance_ticker_price__")


Category behavior is hardcoded in four places instead of on the spec. update() decides date-filling from spec.get("fill_missing_dates") (line 240), but _resolve() decides it from the google_finance_ticker_price__ id prefix here. They agree today only because google_finance is the sole spec with the flag. A second spec that sets fill_missing_dates would get a filled baseline in the question bank and an unfilled resolution baseline, so the forecasted and scored events diverge.

The same pattern shows up in the snapshot_names set + assert here, spec["engine"] == "google_finance" in fetch() (line 126), and name == "flight_departure_delay" in fetch() and update() (lines 148, 302). Adding a sixth category means touching all of them.

Suggest deriving these from the spec so the "declarative request specification" in the module docstring holds:

filled_ids = {id for id, (name, spec, _) in configured.items() if spec.get("fill_missing_dates")} snapshot_names = {name for name, spec in QUESTION_SPECS.items() if spec["engine"] in TODAY_ONLY_ENGINES}

If retired categories must keep resolving after their spec is removed (per the comment above), keep a small RETIRED_SNAPSHOT_NAMES constant and union it in, rather than freezing the live set here.

Generated by Claude Code

I got a more helpful comment for this:

update() checks the spec flag to decide whether to forward-fill. Good. Nothing to change

_resolve() does not check the flag. It checks whether the question id starts with the literal string google_finance_ticker_price__.

fetch() checks spec["engine"] == "google_finance" to decide whether to parse a price history.

fetch() and update() check name == "flight_departure_delay" for the flight-specific logic.

_resolve() has a hand-typed set of the three snapshot category names, plus an assert that the set matches the specs.

ok yes, resolution should follow the same configuration as update. Done. I’ve kept the specific finance and flight delay branches because they handle different response structures and recovery rules.

houtanb · 2026-09-29T19:08:19Z

+    return frame if not frame.empty else pd.DataFrame(columns=constants.QUESTION_FILE_COLUMNS)
+
+
+def upload_resolution_files(resolution_files: dict[str, pd.DataFrame]) -> None:


Forked orchestration helpers. This job reimplements question-bank load, per-id history download, and resolution upload rather than using _source_io. I understand why: the shared upload helper slices to [id, date, value] and would drop fetch_datetime, and the shared download swallows non-404 errors. But that makes the divergence a trap. Anyone who later "cleans this up" by switching to _source_io.upload_resolution_files silently loses the timestamps that guard against stale replays.

Two options, either is fine:

Extend _source_io with what serpapi needs (an extra_columns passthrough on upload, a strict download that re-raises anything but 404) and use it here. Other sources would benefit from the strict download too.

Keep the fork but add a short comment on each helper saying which shared-helper behavior it deliberately differs from and why.

Generated by Claude Code

@nikbpetrov is authoratative here, but my take is that these should go into _source_io.py. Snippets from Claude that would clean up src/orchestration/func_serpapi_update/main.py

def upload_resolution_files(source: str, resolution_files: dict[str, pd.DataFrame], *, extra_columns: Iterable[str] = ()) -> None: ... columns = ["id", "date", "value", *extra_columns] df[columns].to_json( ...

and

def load_existing_resolution_files( source: str, ids: Iterable[str] | None = None, *, strict: bool = False, ) -> dict[str, pd.DataFrame]: ... if os.path.exists(local_filename): os.remove(local_filename) # playbook §2.5: never read a stale /tmp copy if strict: try: gcp.storage.download(bucket_name=..., filename=remote_path, local_filename=local_filename) except NotFound: continue else: gcp.storage.download_no_error_message_on_404(...)

yes, I was agonizing about this and thought initially about modifying _source_io.py but felt the branch and PR should stay within the bounds of the serpapi branch. Hence, my decision to place upload_resolution_files in src/orchestration/func_serpapi_update/main.py. But since you don't mind I have moved this into _source_io.py, adding optional extra columns and strict history downloads. SerpAPI retains its timestamps and download-error safeguards, while existing callers retain their defaults.

@nikbpetrov does this change look good to you?

houtanb · 2026-09-29T19:08:26Z

+        keys.get_secret.assert_called_once_with("API_KEY_SERPAPI")
+        assert source.api_key == "test-key"
+        write.assert_called_once_with("serpapi", fetched)


Minor: these assertions pin the private call shape of keys.get_secret and _source_io.write_fetch_output, which AGENTS.md asks us not to do. Switching to keys.API_KEY_SERPAPI (see comment on the entrypoint) would break this test without changing behavior. The source.api_key == "test-key" assertion plus checking what was written (e.g. the frame passed to the upload, or the uploaded file's contents) covers the contract.

Generated by Claude Code

Agreed. Fixed by previous secret-access edit.

nikbpetrov · 2026-09-28T11:23:14Z

    fetch_datetime: Series[str]


+class SerpapiFetchFrame(ResolutionFrame):


FetchFrame inheriting from ResolutionFrame is, to put it mildly, not expected. I would prefer you repeated the cols/col definitions as opposed to this

You are right. I changed SerpapiFetchFrame to inherit directly from pa.DataFrameModel, with explicit column definitions and validation settings.

nikbpetrov · 2026-09-28T11:24:00Z

+                )
+        if failures and failures == requests_attempted:
+            raise RuntimeError("All SerpAPI requests failed; retaining the previous fetch output.")
+        return pd.DataFrame(rows, columns=list(SerpapiFetchFrame.to_schema().columns))


pd.DataFrame(rows) should do the job, as is the case w/ other sources, no need for columns=... stuff

Note that the explicit columns also preserve the fetch schema when no questions are eligible (for example if all configured IDs are nullified). Without them, the empty dataframe fails pandera validation. Note also that kalshi fetch also explicitly supplies column. I have kept this and add a short comment explaining why.

Good catch for kalshi - it's bad. Openeed #319

no need to change this for now then

nikbpetrov · 2026-09-28T11:27:45Z

@@ -0,0 +1,14 @@
+"""SerpAPI metadata and stable question-ID semantics."""


if i understand, the sole purpose of this is more of a path as the core refactor is ongoing; specifically, eventually the naive and dummy baselines job should be able to use the sources directly so importing from helpers/.py won't be done

if that's the intent behind this file, then i'd actually prefer to see this as a separate commit, like tmp(serpapi) or patch(serpapi) or sth so that after/during the refactor it gets reverted/updated but @houtanb might have another pref

Indeed, these are hangovers until the refactor is complete.

No strong feelings on the separate commit, especially if pressed for time given the urgency of merging this code. It will be cleaned up anyway once we move away from all of those helpers/<source> files

Yes, this supports the existing helper imports used by curation and the baseline forecaster. It also contains percentage-threshold functions shared by the baseline and source resolver, so those will need to be relocated when this module is removed. Given @houtanb’s response, I'll keep the current commit structure and leave that migration to the broader refactor.

nikbpetrov · 2026-09-28T14:01:07Z

+                                for day, price in sorted(prices.items())
+                            )
+                            continue
+                    if name == "flight_departure_delay":


ok, changed to elif

nikbpetrov · 2026-09-28T14:07:11Z

+                        raise ValueError("Expected a JSON object from SerpAPI.")
+                    if "error" in data:
+                        raise ValueError(data["error"])
+                    if spec["engine"] == "google_finance":


actuall,y should the engine-specific code even be in fetch? ideally it'd be a lot neater, similar to the other soruces so it's auditable w/o knowledge of serapi-specifics

Ok. I moved the engine-specific parsing into a helper so fetch() is easier to follow.

nikbpetrov · 2026-09-28T15:19:44Z

+        """Append observations to history and refresh the reusable question bank.
+
+        Args:
+            dfq (DataFrame[QuestionFrame]): Existing questions.


just noticed this in other sources too but I dont think the Args' definition should be re-specified in the docstring -- no need to fix as this is a sources-wide issue

Understood. I’ll keep the existing style for this PR.

nikbpetrov · 2026-09-28T15:23:38Z

+    "amazon_minimum_product_price": {
+        "question_template": (
+            "Will the lowest returned listed price in USD for '{product}' on Amazon.com, "
+            "across all conditions (including used), be higher on {resolution_date} "


all product conditions?

ok. changed to “all product conditions” for clarity

nikbpetrov · 2026-09-28T15:26:44Z

+        )
+    result = source.update(dfq, dff, existing_resolution_files=existing)
+    if result.resolution_files:
+        upload_resolution_files(result.resolution_files)


ditto as above - why not:
_source_io.upload_resolution_files(SOURCE, result.resolution_files)

okay i kinda see - it limts df to 3 cols which is not the case for serapi - but i'd recommend patching the source_io func rather than reinventing one here

looks resolved now

Yes, SerpAPI now uses the shared uploader, with fetch_datetime retained

nikbpetrov · 2026-09-30T09:28:12Z

@pythoryn wtf i didnt click submit on my original comments so i am posting them onyl now

will add to some of them as some seem to be addressed

nikbpetrov

.

nikbpetrov

.

Closes forecastingresearch#49

pythoryn requested review from houtanb and nikbpetrov September 28, 2026 03:56

houtanb reviewed Sep 28, 2026

View reviewed changes

houtanb force-pushed the feat/add-serpapi-source branch from 7bc07fa to f0a795e Compare September 28, 2026 11:40

houtanb reviewed Sep 28, 2026

View reviewed changes

Comment thread src/tests/test_types_and_schemas.py Outdated

Comment thread src/sources/serpapi_helpers.py Outdated

pythoryn force-pushed the feat/add-serpapi-source branch from f0a795e to 2975f83 Compare September 29, 2026 02:12

pythoryn force-pushed the feat/add-serpapi-source branch 2 times, most recently from b4ff05b to 42a0e92 Compare September 29, 2026 04:28

houtanb reviewed Sep 29, 2026

View reviewed changes

pythoryn force-pushed the feat/add-serpapi-source branch from 42a0e92 to 83d24d8 Compare September 30, 2026 05:37

nikbpetrov reviewed Sep 30, 2026

View reviewed changes

pythoryn force-pushed the feat/add-serpapi-source branch from 83d24d8 to 03d6a60 Compare October 1, 2026 00:18

nikbpetrov reviewed Oct 1, 2026

View reviewed changes

pythoryn force-pushed the feat/add-serpapi-source branch 5 times, most recently from 6734fc1 to 5f5aa33 Compare October 5, 2026 03:58

feat: add SerpAPI dataset source

196188e

Closes forecastingresearch#49

houtanb force-pushed the feat/add-serpapi-source branch from 698e63f to 196188e Compare October 5, 2026 08:32

houtanb merged commit 93bd525 into forecastingresearch:main Oct 5, 2026
1 check passed

		return frame if not frame.empty else pd.DataFrame(columns=constants.QUESTION_FILE_COLUMNS)


		def upload_resolution_files(resolution_files: dict[str, pd.DataFrame]) -> None:

		fetch_datetime: Series[str]


		class SerpapiFetchFrame(ResolutionFrame):

		@@ -0,0 +1,14 @@
		"""SerpAPI metadata and stable question-ID semantics."""

Conversation

pythoryn commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houtanb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikbpetrov commented Sep 28, 2026

Uh oh!

Uh oh!

Uh oh!

pythoryn commented Sep 29, 2026

Uh oh!

houtanb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythoryn Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythoryn commented Sep 28, 2026 •

edited

Loading

houtanb left a comment •

edited

Loading

pythoryn Sep 30, 2026 •

edited

Loading