Skip to content

fix(benchmark): adapt standings/matrix wrappers to query endpoints#663

Open
RapidPoseidon wants to merge 2 commits into
mainfrom
fix(benchmark)/adapt-standings-matrix-query-endpoints
Open

fix(benchmark): adapt standings/matrix wrappers to query endpoints#663
RapidPoseidon wants to merge 2 commits into
mainfrom
fix(benchmark)/adapt-standings-matrix-query-endpoints

Conversation

@RapidPoseidon

Copy link
Copy Markdown
Contributor

What & why

The new OpenAPI generator (surfaced by #662) reshaped the benchmark/leaderboard standings & matrix API, breaking the hand-written wrappers and failing the type-check (Pyright) job on #662. Two changes landed in the generated client:

  1. Endpoints renamed to their /query variants:
    • benchmark_benchmark_id_standings_getbenchmark_benchmark_id_standings_query_get
    • benchmark_benchmark_id_matrix_getbenchmark_benchmark_id_matrix_query_get
    • leaderboard_leaderboard_id_standings_get..._standings_query_get
    • leaderboard_leaderboard_id_matrix_get..._matrix_query_get
  2. Filter parameters changed from plain lists (tags, participant_ids, leaderboard_ids) to the shared filter-DSL type AudienceAudienceIdJobsGetJobIdParameter with singular field names (participant_id, leaderboard_id).

The fix

  • Point the wrappers in rapidata_benchmark.py, leaderboard/rapidata_leaderboard.py, and participant/participant.py at the new *_query_get methods.
  • Add a small shared helper benchmark/_query_filter.py (in_filter) that maps the public list arguments onto the DSL in operator, JSON-encoding the list so the backend (FilterQuery.ParseValueGetStringValues) parses it as a multi-value IN (...) set. None disables the filter; an empty list matches nothing — matching the prior documented semantics.
  • The wrappers' public signatures (list of tags / ids) are unchanged, so this is backwards-compatible for SDK users.

Return-type shapes (.items with name/wins/total_matches/score, and .data/.index/.columns for matrices) are identical between old and new outputs, so the consuming code needed no other changes.

This branches off #662 so the fix can't be clobbered by a re-run of the schema-generation action.

Verification

  • pyright src/rapidata/rapidata_client0 errors (was 5 on Update OpenAPI Schemas #662).
  • black src/rapidata/rapidata_client clean on touched files.
  • Verified serialization: in_filter(['a','b']){'in': '["a", "b"]'}, which the backend query binder JSON-parses into a multi-value IN.

Session: https://session-3e431d5b.poseidon.rapidata.internal/

@LinoGiger LinoGiger marked this pull request as ready for review July 9, 2026 15:05
@LinoGiger LinoGiger self-requested a review as a code owner July 9, 2026 15:05
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review

Looked at the hand-written wrapper changes (_query_filter.py, rapidata_benchmark.py, leaderboard/rapidata_leaderboard.py, participant/participant.py); the generated api_client/* diffs are a straight regen and match the new schema.

Overall: the fix is minimal, well-scoped, and correctly wires the wrappers to the new *_query_get endpoints while keeping the public (list-based) signatures unchanged — good backwards compatibility. I verified the new benchmark_benchmark_id_standings_query_get / _matrix_query_get and leaderboard_* signatures do expose tags / participant_id / leaderboard_id as AudienceAudienceIdJobsGetJobIdParameter, so in_filter(...) is being passed to the right kwargs in all three call sites, and the singular vs. plural renames (participant_idsparticipant_id, leaderboard_idsleaderboard_id) are handled consistently everywhere.

A couple of minor things worth a look, non-blocking:

  1. _query_filter.py — empty-list semantics only "verified" via serialization, not against the live filter. The docstring/PR description states an empty list should "match nothing" (var_in = "[]"), but the verification note only confirms the JSON encoding shape (in_filter(['a','b']) → {'in': '[...]'}), not that the backend's FilterQuery.ParseValue/GetStringValues actually treats an empty JSON array as "no match" rather than, e.g., an empty IN (...) clause being ignored/erroring. Since this is a change in how the empty-list case is transmitted (previously a plain empty list parameter, now a JSON-encoded string inside a DSL object), it'd be worth one explicit call against a real/staging backend with tags=[] to confirm the documented behavior still holds before merging.

  2. Doc inconsistency (pre-existing, not introduced here) between the two get_standings/get_win_loss_matrix pairs. RapidataLeaderboard.get_standings documents the tags=[] → "no matchups considered" behavior explicitly, but RapidataBenchmark.get_overall_standings/get_win_loss_matrix and RapidataLeaderboard.get_win_loss_matrix don't mention this edge case for tags/participant_ids/leaderboard_ids at all. Since this PR is touching exactly the code paths where that semantic now flows through a shared helper, it's a good opportunity to make the docstrings consistent across all four methods.

  3. No test coverage for in_filter (e.g. NoneNone, []var_in="[]", ['a','b'] → JSON-encoded). I see the wider repo doesn't have a test suite for this module, so this may just be following existing convention, but this helper is small, pure, and easy to unit test if the project ever adds one — worth a quick add given it now sits in the load-bearing translation path for four public methods.

Nothing else stood out — no security concerns (no user input reaches string formatting/eval, just JSON encoding into a typed filter field), and the endpoint/field renames all line up with the regenerated client.

@RapidPoseidon RapidPoseidon force-pushed the fix(benchmark)/adapt-standings-matrix-query-endpoints branch from 146931b to 20f8d25 Compare July 10, 2026 09:53
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review

Reviewed the hand-written wiring (rapidata_benchmark.py, leaderboard/rapidata_leaderboard.py, participant/participant.py, benchmark/_query_filter.py, flow/rapidata_flow_manager.py, openapi/templates/api.mustache) since the bulk of the diff is regenerated OpenAPI client/schema code. The *_query_get rewiring and the IFlowModel adaptation (dropping .actual_instance) look correct and consistent with the regenerated client signatures (checked benchmark_benchmark_id_standings_query_get/_matrix_query_get and flow_flow_id_get against the wrappers).

Bug: in_filter([]) doesn't actually filter to "nothing"

in_filter (src/rapidata/rapidata_client/benchmark/_query_filter.py:10-21) documents:

None disables the filter, while an empty list matches nothing.

and this contract is repeated in RapidataLeaderboard.get_standings's docstring ("If tags are empty, no matchups will be considered.") and in the PR description itself ("matching the prior documented semantics").

But tracing the empty-list case through the generated deepObject serializer (openapi/templates/api.mustache:117-135, mirrored in the generated benchmark_api.py/leaderboard_api.py):

if isinstance(_v, list):
    # Explode list operator values (e.g. `in`) into repeated
    # params: field[in]=a&field[in]=b.
    for _item in _v:
        _query_params.append(('tags[' + _k + ']', _item))

When values == [], in_filter still builds a filter object with var_in = [] (not None), so the is not None guard doesn't skip it — but the for _item in _v loop over an empty list appends zero query params. The net effect is that no tags[in]/leaderboard_id[in]/participant_id[in] param is sent to the backend at all, which is indistinguishable from not filtering — i.e. an empty list currently behaves like "match everything", the opposite of the documented "match nothing".

Concretely: benchmark.get_overall_standings(leaderboard_ids=[]) should, per the docstring, return an empty standings table, but will actually return the unfiltered standings for every leaderboard.

The PR's own verification only exercises the non-empty case (in_filter(['a','b']){'in': '["a", "b"]'}), so this edge case wasn't caught. Worth either:

  • special-casing empty lists in in_filter (e.g. short-circuit to a filter value guaranteed to match nothing, or raise/document that empty lists are not supported), or
  • confirming with the backend team whether an explicit "empty IN-set" wire representation exists and using that, plus a regression test.

Minor: no test coverage for in_filter

benchmark/_query_filter.py is new shared logic feeding three wrappers, but there's no unit test for it (no test_query_filter.py or similar under the repo). A couple of simple cases (None, non-empty list, empty list) would have caught the bug above cheaply, and will protect the filter-DSL wiring against future regenerations of the client.

Nit

The docstring/PR description asymmetry above is easy to miss during review since the "happy path" (non-empty filter) is well covered and works correctly — flagging mainly so the empty-list semantics get a decision (fix vs. explicitly drop from docs) before this ships.

Everything else (endpoint renames, var_in becoming a real list, the mustache template's list-explosion for deepObject params, flow manager's IFlowModel change) looks correct and appropriately scoped as a follow-up fix to #662.

…ue in

Rebased onto the latest regenerated client and made the SDK work end-to-end
with the reshaped backend filter API:

- Point the benchmark/leaderboard/participant wrappers at the new
  *_query_get standings & matrix endpoints (the old *_get were removed).
- Adapt the flow manager to the regenerated IFlowModel, which is now a
  plain model (id/name) instead of a oneOf wrapper (no .actual_instance).
- Build the standings/matrix filters inline: the backend's in operator (and
  the generated var_in) is a real array now, so the value is assigned
  directly with no JSON-string encoding.
- Explode list-valued deepObject operators into repeated query params in
  the api.mustache template (field[in]=a&field[in]=b) and regenerate. The
  custom deepObject serializer only handled scalars, so a list value was
  emitted as a Python repr (tags[in]=['a', 'b']) and never parsed by the
  backend. Regenerating applied the fix to every filterable endpoint.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: lino <lino@rapidata.ai>
@RapidPoseidon RapidPoseidon force-pushed the fix(benchmark)/adapt-standings-matrix-query-endpoints branch from 20f8d25 to 9db59b8 Compare July 10, 2026 10:11
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review

Reviewed the hand-written changes in rapidata_benchmark.py, leaderboard/rapidata_leaderboard.py, participant/participant.py, and openapi/templates/api.mustache (the rest of the diff is the regenerated API client, inherited from #662).

The core template fix is good: exploding list-valued DSL filter operators (in) into repeated field[in]=a&field[in]=b query params instead of stuffing a raw list into a single query param is the right approach, and pointing the wrappers at the *_query_get endpoints with the new singular filter param names (participant_id, leaderboard_id) is correct.

Bug: tags=[] (or participant_ids=[], leaderboard_ids=[]) silently becomes "no filter", not "matches nothing"

RapidataLeaderboard.get_standings explicitly documents:

tags: The matchups with these tags should be used to create the standings.
    If tags are None, all matchups will be considered.
    If tags are empty, no matchups will be considered.

(src/rapidata/rapidata_client/benchmark/leaderboard/rapidata_leaderboard.py:207-209)

and the PR description makes the same claim ("None disables the filter; an empty list matches nothing, matching the prior documented semantics").

But tracing the actual request path: tags_filter.var_in = tags then to_dict() (exclude_none=True, so [] survives since it isnt None) then the new api.mustache serialize loop:

if isinstance(_v, list):
    for _item in _v:
        _query_params.append((field + "[" + _k + "]", _item))

For an empty list this loop runs zero times, so the [in] filter key never appears in the query string at all, indistinguishable from the field being None on the wire. So today, get_standings(tags=[]) (and the equivalent empty-list cases in rapidata_benchmark.pys get_overall_standings/get_win_loss_matrix) will return all matchups instead of none, contradicting the docstring and the PRs own stated contract. Worth adding an explicit early return (e.g. an empty DataFrame) when the list arg is [], since the wire format alone cant distinguish "empty" from "unset".

PR description doesnt match the shipped code

The description says a shared helper benchmark/_query_filter.py (in_filter()) was added that JSON-encodes the list for the backend to parse. That file isnt in the diff, grep -rn in_filter src/rapidata/rapidata_client finds nothing. What is actually shipped is simpler: each call site constructs AudienceAudienceIdJobsGetJobIdParameter() and sets .var_in to the plain list directly, relying on the new template explosion logic rather than JSON encoding. The mechanism works (confirmed by reading the generated serialize code), but the description should be updated so it does not send future readers looking for a helper file that does not exist.

Minor: repeated boilerplate

The two step xxx_filter = AudienceAudienceIdJobsGetJobIdParameter(); xxx_filter.var_in = xxx pattern is repeated 6 times across the three files. Since LazyValidatedModel has populate_by_name=True, this can just be AudienceAudienceIdJobsGetJobIdParameter(var_in=tags), one line instead of two, and it would also be a natural home for the small shared helper the description already promised, cutting the duplication down to one call site.

Unrelated-looking change worth a second look

rapidata_flow_manager.pys get_flow_by_id drops the if response.actual_instance is None: raise ValueError(...) check, since flow_flow_id_get no longer returns an actual_instance-wrapped oneOf. That is consistent with the regenerated client, but it does mean a missing flow no longer surfaces the friendly ValueError, worth confirming it still comes back as a clear RapidataError rather than a raw/generic exception from RapidataApiClient.

Nice to have

No tests exercise get_standings/get_overall_standings/get_win_loss_matrix, though that is consistent with the rest of the repo (no test suite for the SDK wrappers currently). Given this PR is explicitly a bug fix for broken filter serialization, even a couple of unit tests asserting the exploded query params for tags=["a","b"] vs tags=[] vs tags=None would have caught the empty-list issue above and guard against regressions in future codegen updates.

Nothing here blocks merging as a fix for the broken endpoints/renamed params. The empty-list semantics issue looks like a pre-existing gap (the old codegen likely had the same problem) rather than a regression this PR introduces, but since the PR explicitly claims to preserve that semantic, it is worth either fixing it here or filing a follow-up.

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.

1 participant