feat(benchmarks): one runner for four benchmarks, decider made visible - #425
Open
YueAWu wants to merge 22 commits into
Open
feat(benchmarks): one runner for four benchmarks, decider made visible#425YueAWu wants to merge 22 commits into
YueAWu wants to merge 22 commits into
Conversation
Twelve retrieval arms were measured against a decider that answered 404 on every call. The loop caught the error, fell back to a fixed top-3 core with a single round, and returned HTTP 200 with a full episode list -- so the runs reported plausible accuracies and exited zero. Nothing in the result said the decider had never run. Two causes, both fixed here: Twelve `EVEROS_LLMMR_*` module constants were read at import time, so the harness env that was supposed to configure them arrived too late and every run used the defaults. They are now fields on `DeciderSettings`, resolved per search through `_tuning()`, with the legacy env names still honoured. The fallback logged at debug and was indistinguishable from a normal result. It now logs at error with the underlying exception and calls `mark_degraded`, which surfaces on `SearchData.degraded` -- a caller can tell a degraded result from a healthy one without reading a trace file. Degradations reset per search and restore around it, so a long-lived process does not accumulate another request's reasons.
A 400 or 404 from the embedding endpoint was classified the same as a 503 and went through the full retry ladder, turning an unservable model name into minutes of backoff per call and, under concurrency, into an apparent hang. `_classify` now maps 400/401/403/404/413/414/422 to `EmbeddingInputError` -- a domain error the caller can act on -- and leaves 408/429/5xx and transport failures as `EmbeddingServiceError`, which is what retrying is for.
Three defects that only appear under a long benchmark run. An OME strategy with no timeout could hold its slot indefinitely; a stalled extraction then starved every later run behind it. Runs now carry `run_timeout` and raise `TimeoutError`, which the dispatcher records, rather than being cancelled with no trace. The SQLite engine leaked a connection per thread -- the signature was the pool growing from 6 to 58 and the connector wedging. The pool is now explicit and sized, so exhaustion fails loudly instead of degrading. The LLM client had no way to pass provider-specific fields, so the one setting a thinking model needs to answer within a timeout could not be configured at all. `extra` is now plumbed through.
Profile extraction listened on two triggers, one of which fired from the clustering path, so a store built without clusters silently produced no profiles and a store with them produced duplicates. It now listens on `EpisodeExtracted` alone. The clustering path itself is untouched -- `agentic` retrieval needs it, and removing it is what left four rebuilt stores with zero clusters and an `agentic` route that early-returned an empty set without erroring. The profile lock is now per subject rather than global, so two subjects no longer serialise behind each other.
The four benchmarks each had their own driver, so a fix to one never reached
the others and no two numbers were produced by the same code. They now share
`run.py` and differ only in `adapters/<name>.py`, with every knob that decides
a number living in `configs/<dataset>.toml` -- a run is reproducible from its
config, and `reproduce.sh` passes no model overrides.
Defects this closes, each of which produced a complete-looking wrong number:
Profile injection was implemented only in the EverMemBench adapter; the other
three accepted `include_profile` and discarded the profiles. The rendering is
now shared, and returns the memories unchanged when there is no profile so
existing prompts stay byte-identical.
A decider model name with no endpoint went to the extraction endpoint, 404ed
on every call, and fell back silently. `run.py` now probes the decider with a
real call before the first question and refuses to start if it does not
answer; `--decider-base-url` exists so the pair can be set together, and both
are folded into `run_spec.json` so it records what actually ran.
A store whose owners were built under different partition keys returned zero
episodes and scored ~0.5% with no error. SEARCH now asserts the owner exists
in the store first, and names what the store does hold when it does not.
`.env.example` shipped path placeholders that looked configured, which beat
the config defaults and sent runs at `/01/dialogue.json`. Unresolved `${VAR}`
is now detected and reported by variable name.
The README said the decider "runs the same model as extraction, which needs no extra configuration". The configs disagree: `decider_model` defaults to the published `qwen3.6-27B`, and `decider_base_url` deliberately has no default, because no endpoint we could name would serve that model for the reader. So a first run prints a warning the README never mentions -- that the decider has no endpoint, that it is falling back to the `[llm]` model, and that the result is therefore not comparable to the published numbers. Both READMEs now show that banner and say plainly that such a run is valid but is not the published arm. They also described only half the startup check. `run.py` has two paths: an unresolved endpoint degrades with the warning, while a model pointed at an endpoint that does not serve it aborts. Documenting only the abort left the degrade looking like a bug. The English README also carried its Chinese pointer line in Chinese, which `check-cjk` counts against the English-first policy; the file's own mirror is the allowlisted place for that.
The README had grown into prose about why the harness is built the way it is: sections on reproducibility, on what happens when a path is wrong, on managing servers by hand. None of it told a reader what to type. It is now five numbered steps -- install, data, environment, run, output -- plus a flag table, and 110 lines instead of 284. The per-role key table replaces the decider section: the runner already prints what it fell back to and aborts on the endpoint mistake, so the guide does not need to narrate either. `convert_evermembench.py` moves into `adapters/evermembench.py`, which is where it belonged: the adapter already reads the raw release to translate the gold session names, so the release's session-numbering rule was written down twice -- the converter counting forward and rolling back on an all-blank pair, the adapter skipping it up front. Equivalent on the released data (3,570 sessions either way, verified), but that rule decides gold alignment, and if the two ever drift the ids shift by one, gold matches the wrong session, and every stage still reports success. Both now walk `iter_raw_sessions`. Converting the release again after the move reproduces the shipped `evermembench.json` byte for byte. `--smoke` also stops discarding an explicit `--conv`. It assigned `[0, 1]` unconditionally, so `--conv 0 --smoke` ran two conversations and scored 20 questions; the banner printed the conversations it had chosen rather than the ones it was given, so the only way to catch it was to count graded rows.
The four configs named the standard arm, not the published best one: LoCoMo ran
the 27B decider rather than deepseek-v4-flash-0731, and LongMemEval and
EverMemBench answered with gpt-4.1-mini rather than gemini-3.6-flash and
gemini-3-flash-preview. Following the README reproduced a number several points
below the one it cited. Each config now names the arm its published number came
from, and README section 6 states that number next to the decider and answer
model that produced it.
SubtleMemory needed no change: `answer_route` in its adapter already routes each
question between the two answer contracts, so the config was the published arm
already.
`check_protocol.py` pinned LongMemEval's answer model and nothing else, so it
reported "no divergence" for three configs it never looked at -- the two answer
models above changed under it without a word. It now covers all four, and the
values are the expanded ones the loader produces rather than the `${VAR:-...}`
templates the files carry, because that is what a run actually uses.
`unit tests` and `unit tests (3.13)` both failed on this branch while the
same suite passed on every developer machine. The cause is one guard:
shard = Path("/root/v3_longmemeval/store_s0")
if not (shard / ".index" / "lancedb").exists():
pytest.skip("LongMemEval shard not present")
`pathlib` only swallows ENOENT / ENOTDIR / EBADF / ELOOP; EACCES re-raises.
`/root` is mode 0700, the author runs as root and the GitHub runner does not,
so on CI the probe raised `PermissionError: [Errno 13] Permission denied:
'/root/v3_longmemeval/store_s0/.index/lancedb'` and the intended skip became a
failure -- deterministically, which is why both interpreter jobs failed and
neither reproduced locally.
Reproduced in an ubuntu-24.04 container running the suite as uid 1001 with the
runner's environment: 1 failed, 2474 passed before, 2474 passed / 4 skipped
after. For a guard asking "is this store on this machine", a path the process
cannot stat is not present, so `_present()` answers False rather than raising.
The test's subject is still a hardcoded absolute path in a private workspace.
`.gitignore` already unships `tests/unit/test_multiround_per_store.py` for
exactly that reason; whether this one should follow is left to the author.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch rewrote every `source`, `sdist` and `wheels` URL in `uv.lock` from `https://pypi.org/simple` / `files.pythonhosted.org` to `https://mirrors.aliyun.com/pypi/...` -- 1456 references, 0 left pointing at PyPI. That is a local index configuration leaking into a committed lockfile: it makes `make install-deps` in all five CI jobs, and every clone by anyone outside that mirror's region, resolve and download from a third-party regional mirror. The rewrite also dropped the `size` and `upload-time` metadata PyPI serves. Reverted to the lockfile as it stands on `main`. Nothing else changes: both locks pin the same 136 packages at the same 136 versions, and the multiset of artifact hashes is identical, so the installed bytes are unchanged. `uv lock --check` resolves clean against this branch's `pyproject.toml` (which changed no dependency), and `uv sync --frozen` audits the existing environment with nothing to reinstall. To keep a mirror locally without it reaching the lockfile, configure it for installs only and re-lock with the default index, e.g. `UV_DEFAULT_INDEX=https://pypi.org/simple uv lock`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
YueAWu
changed the title
`DeciderSettings.extra` was defined, documented, and exported by the evaluation scripts, but `LLMRoundDecider` called `llm.chat(messages=...)` and nothing else -- so every `enable_thinking=false` was a no-op. A Qwen chat template therefore left thinking ON, an un-finetuned decider spent its whole budget reasoning, and 39.3% of rounds returned nothing inside the 60s deadline. Each of those cost 4 attempts x 3 SDK requests x 60s before falling back to the fixed top-3 core, which the harness recorded as a plausible answer rather than a failure. Two settings join it so the deadline can actually be met: `max_tokens` (512) bounds a reply whose contract is one small JSON object, and `sdk_max_retries` (0) stops the SDK's own retries from multiplying with the decider's. Measured after the fix: 265 -> 10 completion tokens, 10.1s -> 1.7s per decision, and the 27B reference's decider-failure rate 11.9% -> 0.2%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`benchmarks/docs/` holds LaTeX sources and PNGs regenerated from the numbers already in the README, so they are build output rather than source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
YueAWu
force-pushed
the
feat/llm-multiround-retrieval
branch
from
August 31, 2026 13:56
8d8307c to
630add5
Compare
`SearchData.degraded` was empty on every real search, including the ones it
exists to mark. `mark_degraded` rebound a `ContextVar`, and the decider runs
inside `SearchManager.search`'s `asyncio.gather` (manager.py:232). `gather`
wraps each argument in a Task, a Task starts from a *copy* of the context, and
a `set()` in that copy is invisible to the parent that reads it back at
manager.py:245. Measured on the real API before this:
await asyncio.gather(child_that_marks(), other()) -> parent sees ()
await child_that_marks() -> ('decider_fallback',)
So the claim this PR makes -- that a caller can tell a fallback from a healthy
result by looking at the response -- held only for a decider awaited directly,
which is not how search runs it. Every existing test marked and read inside one
task, so the suite agreed with the claim rather than with the code.
The variable now holds a mutable sink and `mark_degraded` appends to it, never
rebinding. A copied context copies the binding, not the object behind it, so
the sink crosses the task boundary, while a fresh sink per `reset_degradations`
keeps one request out of the next one. `mark_degraded` outside a request is a
no-op: installing a sink lazily would put it in whichever task called first,
which is the bug again with extra steps.
Both new tests drive the real `SearchManager.search`, and the reason string
comes from `llm_multiround`'s own fallback rather than from the test -- one
asserts it arrives, the other that the next search is not marked by it. Both
fail on the previous implementation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Corrects a claim made earlier on this branch. `9b63d46` said `sdk_max_retries` (0) "stops the SDK's own retries from multiplying with the decider's". It did not, on either production path, and the settings field it added reached nothing: * `get_decider_llm_client()` built an everalgo `LLMConfig`, whose fields are `model, api_key, base_url, temperature, max_tokens, timeout, extra`. There is no `max_retries`, and `OpenAICompatClient.__init__` passes only api_key, base_url and timeout to `AsyncOpenAI`. Measured on the pre-fix construction: `max_retries == 2`, the SDK default. * With no `[decider].model` the function returned the shared extraction client outright (client.py:138-139), which cannot carry a decider-specific retry count even in principle -- the SDK reads it at construction. * The `max_retries` parameter `9b63d46` added to `OpenAIProvider` was on neither path: only `build_llm_provider` constructs one, it takes `LLMSettings` and passes no `max_retries`, and nothing in `src/` calls it. So the 4 attempts x 3 SDK requests x 60s round that commit describes was still being paid. `resolve_decider_config()` now resolves the effective decider settings once -- model, key and base URL each falling back to `[llm]`, timeout and retries always from `[decider]` -- and the client is built from `OpenAIProvider`, which can carry `max_retries`, satisfies the same `LLMClient` protocol, and returns the same `ChatResponse` type. It is always its own instance, so the inherited configuration gets the decider's deadline and retry count too. `extra` travels with whichever section supplied the endpoint, which is what the previous code did through two different clients: a decider on its own gateway must not inherit a field that gateway rejects, and one inheriting `[llm]` must not lose the field that endpoint requires -- dropping it there would silently turn thinking back on for a Qwen deployment that configured it off once. `sdk_max_retries` gains `ge=0`: the SDK reads it as a count. The tests assert `AsyncOpenAI.max_retries` itself on both paths, and the decider stub now records its call kwargs instead of discarding them through `**_` -- a stub that throws the request away is why `max_tokens` and `extra` could be added, documented, and never sent with the suite green. All of them fail on the previous implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The startup probe gated on `config.decider_model and config.decider_base_url`, so it skipped the shape it exists for. Naming only `[decider].model` and inheriting the endpoint is a valid runtime config -- `get_decider_llm_client()` falls back to `[llm]` field by field -- and when that inherited endpoint does not serve the named model, every decider call fails inside the run instead of before its first question, which is the silent degradation this whole check was added to prevent. Its early return also claimed "the backbone decides, and its own probe covers it". There is no backbone probe: `_assert_decider_answers` and `_assert_owner_in_store` are the only two in this file. `resolve_decider_endpoint()` now applies the same `[decider]` -> `[llm]` fallback as `everos.component.llm.client`, expressed against the fields this harness pushes into the servers it starts (`decider_*` -> `EVEROS_DECIDER__*`, `backbone_*` -> `EVEROS_LLM__*`, then this process's own environment). The gate is what the run does rather than which fields happen to be filled in: a run with no `llm_multiround` method still is not probed, because it makes no decider calls and probing it would fail a hybrid run over an endpoint it never touches. When a decider does run and nothing resolves, it says NOT PROBED rather than returning quietly -- a fail-fast promise that silently declines to check is the same silence it replaces. The probe also sends `[decider].max_tokens` now. Its comment argued for omitting it because "`RoundDecider.__call__` passes messages and nothing else", which `9b63d46` made untrue: the real call is capped, so an uncapped probe vouches for a budget the run never gets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two post-processor metrics that answered a different question than their name.
Neither can crash and neither appears in a run, so both produced numbers of the
right shape indefinitely.
nDCG's ideal ranking was built from the relevance list of the results it had
*retrieved*, so gold the search missed never entered the denominator. The
reported case:
ranked=[["a"]], gold=[{"a", "b"}], k=2 -> recall@2 0.5, ndcg@2 1.0
A perfect ranking score for a ranking with half the answer missing. The ideal is
now `min(len(gold), k)` relevant items, which gives 0.613 there. That change
alone breaks the ratio, because duplicates then earn more gain than the ideal
holds -- `gold={a}` with `ranked=[a, a]` would score 1.63 -- so DCG now credits a
session at its first occurrence only, computed in the same pass as average
precision, which already counted it once. Recall and precision keep their
existing rules: once, and every time, respectively.
`core_sessions_from_trace` keyed by owner and overwrote. That holds only where
one owner asks one question, which is LongMemEval (`longmemeval_<question id>`)
and is why it looked right; LoCoMo puts every question of a conversation under
one owner, where all but the last were dropped and which survived depended on
the order the trace happened to be written in. The key is now
`(owner_id, question_id or question)` -- the search layer cannot fill
`question_id`, since a `/search` request carries only a query and an owner, so
the question text is the stable identifier available. Later rounds for one
question still replace it: the final injection is the state that was scored.
`score()` takes both mappings keyed the same way. A caller still keying gold by
owner now gets `n == 0` with every entry in `skipped_no_gold`, which is loud;
the arrangement it replaces was silent.
The tests write each expectation as the arithmetic that produces it rather than
echoing the implementation, and cover zero gold, full recall, partial recall,
duplicates and `k > len(ranked)`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two assumptions that held on the machine this was written on. `from_toml` appended `.toml` to whatever it was given, while `--config` is documented one screen away as the way to "point at a file outside benchmarks/configs/". So `--config /tmp/custom.toml` opened `/tmp/custom.toml.toml` and reported that as missing -- a path the caller never asked for, which reads as "your file is not there" rather than "I rewrote your argument". A name and a path are now decided apart: a `.toml` suffix or a path separator means a path, used as given; a bare name still resolves under `benchmarks/configs/` with the suffix appended, including the legacy `config.<name>` spelling. The error names the path actually opened either way. `_iter_servers` walked `/proc` unconditionally, so `--list-servers` ended on macOS with `FileNotFoundError: [Errno 2] ... '/proc'`. Process discovery here reads `cmdline` and `environ` out of `/proc` and has no cross-platform equivalent, so the honest answer is that it cannot look: it now says so, names the platform, and points at `--base-url`. Linux behaviour is unchanged, which the tests check by exercising the real walk here rather than only the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_run_pass` already stated the rule -- "A conversation that failed its ingest must not be read: its store is short whatever the failure dropped, and searching it would score the gap as a retrieval miss" -- and then only recorded the outcome. `_run_server_group` ran the read pass over every conversation in the group regardless, so a failed ADD went on to produce searches, answers and judgments against a partial or empty store, and those rows counted in the report's denominator. The exit code was the only signal, and it is the one that does not persist. The report is written before it, is shaped exactly like a clean one, and is what gets read later or picked up by a script. So the read stages now run over the conversations whose ingest succeeded, the skip is announced with the conversation numbers, and the report records `unscorable_conversations` in every method summary. A conversation that failed to ingest contributes no rows at all, so it cannot reach a headline denominator by another route. Tested against `run.py`'s own source for `_run_server_group` rather than a paraphrase of it, so the copy under test cannot drift from the original: failed conversations are dropped from the read pass, clean ones are untouched, an entirely failed group runs no read pass at all, and the skip is printed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guide called EverMemBench's headline "the mean of its nine category columns" -- a macro average. `_collect_method_summary` computes `sum(correct) / sum(total)`, a micro accuracy, and always has. The two differ whenever categories have unequal sizes and unequal accuracy, which is every real run, so the published 66.67 could not be reproduced from the definition next to it. The measured numbers stand. The documentation moves to meet them: the guide now states the definition as arithmetic, says the per-category lines are reported and never averaged in, and the computation itself is labelled MICRO at the point it happens. Two things a reader needs before comparing the figure to anyone else's, so both are written down rather than left to be rediscovered. Five of the six rows in the published EverMemBench comparison table are the macro mean of their own nine columns, so reading our number alongside that column compares two different statistics. And "nine categories" is not something this code can confirm: the adapter names three (`F_SH`, `F_MH`, `F_TP`) and derives the rest from a question-id prefix, so an unlabelled id is reported under its raw key. The test fixture is lopsided on purpose -- 20/100 in one category and 4/4 in another, where micro is 0.2308 and macro is 0.60 -- and asserts both, so a refactor to macro fails with the two numbers side by side rather than reporting that one moved. It also covers an empty category (contributes no rows, so it neither divides by zero nor shifts the headline) and a method with no graded rows at all (no summary, so the ratio is never taken). The guide's wording is asserted too, since the defect was in the guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resume decided what to skip from a row index and an `add.done` marker, with
nothing checking that the rows it kept came from this experiment. Change the
model, the data or the retrieval parameters in a directory that already holds
results and the two runs merge into one report reproducible from neither -- and
`_write_run_spec` then overwrote the old spec, erasing the evidence that they
had differed.
The identity is TWO values, because the halves cost differently. Re-ingesting a
store is hours of LLM calls; re-reading it is minutes.
ingest identity data digest, adapter, extraction backbone
-> mismatch aborts; the stored memories are wrong for this
run and no amount of re-reading fixes them
read identity methods, top_k, include_profile, answer/judge model,
decider model and endpoint
-> mismatch keeps the store and re-runs the read stages
Two things are deliberately NOT in the ingest identity, which is the whole
reason for splitting it. `top_k` caps how many episodes reach the answer prompt
and has no bearing on what was ingested. And the `everalgo` version was measured
across the versions in use here without moving the benchmark, so a bump must not
discard an ingest; it stays in `packages` for provenance without gating resume.
The check runs before `_write_run_spec`, since that call is what destroys the
record it needs. Superseded stage files are renamed, not deleted -- they are the
evidence of what the directory held. An identity the old spec never recorded is
UNKNOWN, not different: specs predating this carry none, and calling their every
field a mismatch would make every historical directory unresumable. Forcing is
possible with --force-reuse-dir and is printed and recorded rather than silent.
`add.done` now carries the identity too, so a marker mirrored in from another run
no longer skips ADD for a store built from other data. It records data and
adapter only: the marker is written inside the per-conversation nested function,
where the startup `serving` list is out of scope, and reaching for a name
assigned further up is what killed two runs after ADD had already finished. The
backbone is still checked at directory level.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SubtleMemory's `data_path` is a directory -- `benchmarks/data/subtlememory/` -- and `_file_digest` only handled files, so its ingest identity came out reading `unavailable:/.../subtlememory`. That is the one dataset whose data changes resume could never have detected, which is the opposite of what the identity is for. Found by running the smoke pass and reading the fingerprint it wrote. Each file now contributes its relative path as well as its bytes, so adding or renaming a file moves the digest even when no byte inside one does. A path that does not exist is still reported as unavailable rather than as a constant, so two missing datasets never compare equal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One runner for four long-term-memory benchmarks (LoCoMo, LongMemEval,
EverMemBench, SubtleMemory), and the multi-round decider's silent failure paths
made visible.
The decider previously fell back to a fixed top-ranked core on any failure and
returned HTTP 200 with a full episode list, so a run whose decider never
answered reported a plausible number and exited zero. It now logs at error,
surfaces on
SearchData.degraded, and the harness sends one real completion tothe decider before the first question and refuses to start if it does not
answer. Twelve sweep arms were measured against that degraded path before this.
Also: each config now names the arm its published number came from,
[answer]and
[judge]models included;--smokeno longer discards an explicit--conv;the EverMemBench converter moved into its adapter so the release's
session-numbering rule is written down once instead of twice.
Area
Verification