Repository navigation
benchmarking/evaluate: run evaluations, with a run_results source - #3551
Merged
Max Smythe (maxsmythe) merged 3 commits intoNov 12, 2026
Merged
Conversation
This was referenced Nov 2, 2026
Haowei Cai (Roy) (roycaihw)
force-pushed
the
run-evaluations-b
branch
2 times, most recently
from
November 3, 2026 17:56
4805228 to
7972199
Compare
Haowei Cai (Roy) (roycaihw)
changed the title
Haowei Cai (Roy) (roycaihw)
force-pushed
the
run-evaluations-b
branch
from
November 4, 2026 03:56
7972199 to
7ed57d5
Compare
Haowei Cai (Roy) (roycaihw)
marked this pull request as ready for review
November 4, 2026 04:00
Contributor
Author
Haowei Cai (Roy) (roycaihw)
force-pushed
the
run-evaluations-b
branch
2 times, most recently
from
November 4, 2026 16:29
d89d6b6 to
5950bb3
Compare
Haowei Cai (Roy) (roycaihw)
force-pushed
the
run-evaluations-b
branch
from
November 9, 2026 01:33
644cb0e to
f064e01
Compare
Contributor
Author
|
Max Smythe (@maxsmythe) I've addressed the comments. PTAL |
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class BenchmarkRunContext: |
Collaborator
There was a problem hiding this comment.
Is this an abstract concept or specific to the RunResults source?
Contributor
Author
There was a problem hiding this comment.
It's an abstract. It's what every source gets about the run. Reworded the docstring.
Collaborator
There was a problem hiding this comment.
What I don't get is why every class gets/needs this information. Wont most/many sources be reading from their own data files that the benchmarking framework likely wont be aware of (even if they live in the artifact directory)?
If the source already knows what files they are looking for, why does it need this data?
Contributor
Author
There was a problem hiding this comment.
Good point. It was shaped around the RunResults source. Now it only says which run it is (test, tag, run_ts, uri) and gives access to the run's stored files. Each source reads what it needs, or looks the run up in its own data by that identity.
| if bad := [k for k in cfg if not isinstance(k, str)]: | ||
| raise ConfigError(f"{where}: source_cfg: non-string keys {sorted(bad, key=repr)}") | ||
| try: | ||
| SOURCES[source].validate(cfg) |
Collaborator
There was a problem hiding this comment.
nice.
We'll need to figure out how to extend this with pluggable sources, but... baby steps
Collaborator
|
Haowei Cai (Roy) (@roycaihw) Thanks! Took another pass. |
Haowei Cai (Roy) (roycaihw)
force-pushed
the
run-evaluations-b
branch
from
November 10, 2026 21:37
f064e01 to
c9310f3
Compare
Benchmark pass/fail logic is spread over one-off tools. With this, a test entry declares `evaluations`, each a set of variables (source queries) and CEL conditions over their values, and the evaluator judges a run into a `Report`: pass, fail or invalid per evaluation. Nothing calls it yet. Conditions compile with `cel-expr-python` when the evaluations load. Sources are classes with three hooks (`sources/base.py`): `validate(cfg)` when the evaluations load, `init(run)` when the run starts, and `query(cfg)` after it. A variable whose source can't judge the run is invalid and is left out of the CEL data, so every condition that reads it is invalid and the evaluation can't pass. `run_results` reads one `stats.jsonl` row. AI use: I wrote this with an AI coding agent from the design, then reviewed the diff and checked the goodput numbers against the tools/goodput prototype on 248 Prow runs. Signed-off-by: Haowei Cai <haoweic@google.com>
Conditions are now plain CEL: integer literals are no longer rewritten, reserved names are left to CEL, and the evaluator no longer tracks which variables a condition reads. An invalid variable is left out of the CEL data and makes its evaluation invalid. The config shape is pydantic models. Signed-off-by: Haowei Cai <haoweic@google.com>
Haowei Cai (Roy) (roycaihw)
force-pushed
the
run-evaluations-b
branch
from
November 10, 2026 21:39
c9310f3 to
3d13e0b
Compare
Contributor
Author
|
Max Smythe (@maxsmythe) I've addressed the comments. PTAL. |
Contributor
Author
|
/retest |
Max Smythe (maxsmythe)
left a comment
Max Smythe (maxsmythe)
left a comment
Collaborator
There was a problem hiding this comment.
One question about the run() object, and then I threw AI at it
BenchmarkRunContext now carries the run's identity and artifact(name) instead of a file list, and the Report reads the identity from it. Each source chooses which artifacts to read, or looks the run up in its own store by identity. source_cfg is typed as JSON so non-JSON values or non-string keys fail at load. RunResults skips blank lines in stats.jsonl and marks the variable invalid on any malformed line. Conditions accept DYN so bare bool fields work, and evaluated_at converts to UTC before formatting. AI use: I wrote this with an AI coding agent from the design, then reviewed the diff. Signed-off-by: Haowei Cai <haoweic@google.com>
Contributor
Author
|
Max Smythe (@maxsmythe) Thanks for reviewing! I addressed the question and the comments. PTAL. |
Collaborator
|
Thanks for all the work! Merging as a test-only change |
Max Smythe (maxsmythe)
enabled auto-merge
November 12, 2026 07:17
Merged
via the queue into
agent-substrate:main
with commit Nov 12, 2026
e472ffb
8 of 9 checks passed
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
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.
Part of #3298 (design). First of four PRs: #3551 (this), #3550, #3552, #3592.
Benchmark pass/fail logic is spread over one-off tools. With this, a test entry declares
evaluations, each a set of variables (source queries) and CEL conditions over their values, and the evaluator judges a run into aReport: pass, fail or invalid per evaluation. Nothing calls it yet. #3552 wires it into the runner and writesevaluation.jsonand thestats.jsonlrows.Conditions compile with
cel-expr-pythonwhen the evaluations load. Sources are classes with three hooks (sources/base.py):validate(cfg)when the evaluations load,init(run)when the run starts, andquery(cfg)after it. A variable whose source can't judge the run is invalid and is left out of the CEL data, so every condition that reads it is invalid and the evaluation can't pass.run_resultsreads onestats.jsonlrow. The config is pydantic models.source_cfg: nullis an error; leave the key out for{}.AI use: I wrote this with an AI coding agent from the design, then reviewed the diff and checked the goodput numbers against the #2526 prototype on 248 Prow runs.