Skip to content

fix(results): read canonical info.type in to_pandas#684

Merged
RapidPoseidon merged 1 commit into
mainfrom
fix(results)/read-canonical-type-field
Jul 17, 2026
Merged

fix(results): read canonical info.type in to_pandas#684
RapidPoseidon merged 1 commit into
mainfrom
fix(results)/read-canonical-type-field

Conversation

@RapidPoseidon

Copy link
Copy Markdown
Contributor

Problem

The result aggregator (rapidata-evaluator) emits the task type as info.type (schema version 4.0.0), and the docs match. But RapidataResults.to_pandas() read info.orderType — a field the server does not emit anywhere in v4 (it exists only in this reader; not in the evaluator, OpenAPI schemas, docs, or any example).

Consequences for every current Compare/Ranking result:

  1. orderType is None → prints the misleading "Results are old and Order type is not specified" warning on brand-new results.
  2. Never routes to the compare-aware path → to_pandas() silently falls through to the generic flattener and loses the A_/B_ asset columns.

Reported from integration testing: SDK 3.16.5, Python 3.12, Compare task.

Fix

Client-side tolerant reader: _task_type() reads canonical type with orderType as a read fallback, so results generated before the rename still route correctly. No server change — this fixes every result already in the wild without re-aggregation.

Verified locally: a v4 (type: Compare) payload now yields A_/B_ columns; an orderType payload still works; the warning fires only when the type is genuinely absent. pyright and black clean.

Scope

Minimal fix so the broken parsing can be merged now. A typed result-model design (typed accessors over the raw dict) is being drafted separately and coordinated with the sibling raw-vs-weighted winner fields work.

🔗 Session: https://node-25c37285.poseidon.rapidata.internal/

The aggregator emits the task type as info.type (v4.0.0), but
RapidataResults.to_pandas() read info.orderType — a name the server
does not emit. Every current Compare/Ranking result therefore hit the
"Results are old" warning and fell through to the generic flattener,
losing the A_/B_ asset columns.

Read info.type as canonical with info.orderType as a read fallback so
both current and older payloads route to the compare-aware path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: lino <68745352+LinoGiger@users.noreply.github.com>
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review

Small, well-scoped fix. The root cause (reading a field the aggregator never emits) is clearly diagnosed in the PR description, and the fallback approach (type canonical, orderType as legacy read fallback) is the right minimal change to unblock this without a server round-trip.

Code quality

  • _task_type() is a clean extraction — good that both call sites (to_pandas warning check and the Compare/Ranking routing) now go through one source of truth instead of duplicating self["info"].get(...).
  • Using .get("info", {}) instead of the old self["info"] is a nice incidental improvement — the old code would KeyError if "info" were ever missing entirely; now it degrades to the "old results" warning instead.
  • Docstring on _task_type() explains the why (rename tolerance) rather than just restating the code — matches the repo's comment conventions.

Potential edge case (minor, not blocking)

  • info.get("type") or info.get("orderType") will fall through to orderType if type is present but falsy (e.g. ""). In practice the aggregator presumably never emits an empty string for this field, so this is unlikely to bite, but info.get("type") if info.get("type") is not None else info.get("orderType") (or info.get("type", info.get("orderType"))) would be more precise if you want to guard against it. Given the stated scope (unblock the known bug), this is fine to leave as-is.

Test coverage

  • No test accompanies this fix, and it doesn't look like rapidata_results.py/to_pandas() has any existing unit tests in tests/ to extend. Given this is fixing a real production bug affecting every Compare/Ranking result, a small regression test (e.g. feeding a synthetic results dict with info.type == "Compare" and one with info.orderType == "Compare" through to_pandas() and asserting A_/B_ columns appear) would be valuable to prevent this from regressing again when the typed result-model work lands. Not a blocker given the "minimal fix to unblock now" framing, but worth a follow-up.

Scope

  • Correctly minimal — no unrelated refactoring, and the PR description clearly flags the larger typed-accessor redesign as separate/follow-up work.

Overall: looks correct and safe to merge. Only suggestion is to add a regression test in a fast follow-up.

@LinoGiger
LinoGiger marked this pull request as ready for review July 17, 2026 14:15
@LinoGiger
LinoGiger self-requested a review as a code owner July 17, 2026 14:15
@RapidPoseidon
RapidPoseidon merged commit fd86544 into main Jul 17, 2026
3 checks passed
@RapidPoseidon
RapidPoseidon deleted the fix(results)/read-canonical-type-field branch July 17, 2026 14:16
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.

2 participants