allenai / allenai/asta-autodiscovery

Validate that `n` actually delivers independent samples for our primary models

Đang mở
#73 2 bình luận 0 reaction 0 người được giao Xem trên GitHub
enhancement
Ngôn ngữ chính
Python
Star
10
Fork
2
Merge trung bình
1 ngày 21 giờ
Pull request đã merge (30 ngày)
11

Mô tả

Split out of #68 review, where the `n` parameter came up as the one non-standard thing we ask of every provider. Related: #67, #71 (changes the default model), and a Slack thread with @radamson.

## Why this needs its own investigation

`n` is load-bearing: `--n_belief_samples` (default 5) becomes `n` on a single request, and belief distributions are built from those samples. The framing so far has been *"does the provider support `n`?"* — but measuring it turned up three failure modes that are all quieter and more consequential than outright rejection:

1. A provider can accept `n`, return HTTP 200, and give back **fewer choices than asked** — no error.
2. A provider can return the requested count where the samples are **not independent** — duplicate candidates, so belief variance is understated with nothing to detect it.
3. Our per-provider `n` cap is a hardcoded guess that is **wrong in both directions** depending on the model.

None of these are caught today: `query_llm` never compares `len(response.choices)` to the requested `batch_n`, and the usage record stores the *requested* count (`metadata["n"] = batch_n`) while its own docstring claims it is *"the actual per-request sample count"*. So the one artifact you would check afterwards confirms the wrong number.

## The primary-model list is the blocker

The set of models this should be validated against is in flux:

- **#71** switches all three of `--model` / `--belief_model` / `--vision_model` to `gemini-3.7-flash`.
- The Copilot model set needs input from the collaborator driving that work — Copilot's own `/models` advertises models that are not callable, so "supported" there is account-dependent (see #68 for the detail).

**Action: settle the primary-model list first**, then run the matrix below against it. Everything measured so far is groundwork, not a conclusion.

## Measurements so far

All via `autodiscovery.llm.complete` on this branch (litellm transport), prompt = *"pick a random integer between 1 and 1000000"*, `temperature=1.0` where accepted. `distinct` counts unique values in one response.

### Independence — the finding that matters most

| Model | n=3 | n=5 | n=8 |
| --- | --- | --- | --- |
| `vertex_ai/gemini-3.7-flash` ← **#71's incoming default** | — | 5 → **2 distinct** | 8 → **3 distinct** |
| `vertex_ai/gemini-3.1-pro-preview` ← current default | 3 → **2** | 5 → **3** | 8 → 7 |
| `vertex_ai/gemini-3-flash-preview` | 3 → 3 | 5 → 5 | 8 → 8 |
| `openai/gpt-4o`, `openai/gpt-5`, `openai/gpt-5-mini` | — | 5 → 5 | 10 → 10 |
| `github_copilot/gpt-5.4` | — | 5 → 5 | — |

Collisions in a 1-in-a-million range are not chance. **`gemini-3.7-flash` is the worst of the set** — 2 distinct values out of 5 — and #71 makes it the model for all three roles. `gemini-3-flash-preview` is clean, so this is not "all Gemini"; it is model-specific and needs measuring per candidate default.

Worth confirming with a belief-shaped prompt rather than a random integer before drawing conclusions — the effect may be smaller when the output space is genuinely constrained. That is the first task here.

### Provider `n` ceilings vs. our hardcoded caps

`llm.max_n` currently returns: `vertex_ai` → 5, `openai` + reasoning → 8, everything else → `None`.

| Provider | Real ceiling (measured) | Our cap | Consequence |
| --- | --- | --- | --- |
| `vertex_ai` (3.1-pro, 3-flash, 3.7-flash) | **8** (`n=9` → `BadRequestError`) | 5 | Over-conservative: batches at n=6-8 where one call would do. Correct, just extra round-trips. |
| `openai` | ≥10 | 8 (reasoning) / None | Fine. |
| `github_copilot` | **8** (`Invalid 'n': ... Expected a value <= 8, but got 10`) | `None` | **Broken above 8** — see below. |

### A regression #68 introduced for Copilot

On `main`, Copilot never sent `n`. `copilot_provider` looped client-side:

```python
responses = [_sample()]
if n_samples > 1:
with ThreadPoolExecutor(...) as executor:
responses.extend(executor.map(lambda _: _sample(), range(n_samples - 1)))
```

That worked for any `n_samples`. #68 routes Copilot through litellm with `n=n_samples` in one call, and `max_n` returns `None` for Copilot, so nothing batches:

```
copilot/gpt-5.4 query_llm n=5 → 5 samples, 5 distinct OK
copilot/gpt-5.4 query_llm n=10 → BadRequestError: Invalid 'n': integer above maximum
value. Expected a value <= 8, but got 10 instead.
```

Latent at the default `n_belief_samples=5`. The fix is one line — `max_n` returning 8 for `github_copilot` so existing batching handles it — deliberately left out of #68 to keep that PR scoped to the Gemini defaults, which are unaffected.

### What #68 did *not* break

Worth stating, since swapping Vertex off its OpenAI-compatible endpoint was the plausible regression: `n` still works end to end for the Gemini defaults. litellm's native Vertex path maps `n` → `candidateCount`, and the 5-cap batching still fires.

```
query_llm vertex_ai/gemini-3.1-pro-preview n=5 → 5 samples n=10 → 10 samples (2 calls)
query_llm vertex_ai/gemini-3-flash-preview n=5 → 5 samples n=10 → 10 samples (2 calls)
```

## Proposed scope

1. **Settle the primary-model list** (blocked on #71 and Copilot-collaborator input).
2. Re-run the independence matrix against that list using a **belief-shaped prompt**, not a random integer, to see whether the duplicate-candidate effect is material for the actual workload.
3. Decide whether `n` is the right mechanism at all for models with poor candidate independence, or whether belief sampling should issue separate requests — which is what the pre-#68 Copilot path effectively did.
4. Replace `max_n`'s hardcoded table with measured values, or derive it. Vertex is 5-should-be-8; Copilot is missing entirely.
5. Add a shortfall check: warn (or raise) when `len(choices) < batch_n`, and record the **delivered** count in `metadata["n"]` rather than the requested one. Fix the docstring either way.

## Note for #68 / #71 sequencing

These two PRs conflict semantically, not just textually. #71 sets the defaults to bare `gemini-3.7-flash`; #68 requires litellm-qualified names and rejects unqualified ones at startup. Whichever merges second needs `vertex_ai/gemini-3.7-flash`. They also overlap on `args.py`, `easy.py`, `run.py`, `agents.py`, `scripts/run_job.sh`, `standalone.md`, and `api/metrics/costs.py` (also touched by #70).

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.