Skip to content

Split benchmark fields per type: schema 2 rejects fields the type does not use - #405

Closed
ishandhanani wants to merge 1 commit into
idhanani/srt2-14-post-evalfrom
idhanani/srt2-15-benchmark-fields
Closed

ishandhanani wants to merge 1 commit into
idhanani/srt2-14-post-evalfrom
idhanani/srt2-15-benchmark-fields

Conversation

@ishandhanani

Copy link
Copy Markdown
Collaborator

Stacked on #404 (Track 2, the deferred benchmark union from #385). Draft until the stack below it merges.

What

BenchmarkConfig is one flat dataclass with every type's fields on it, so a field the runner never reads (isl on gsm8k, num_shots on sa-bench) loads fine and silently does nothing. Each runner now declares the fields it reads as config_fields, next to a shared set every type may set (client_placement, client_dedicated_node, colocate_with_frontend, sweep, aiperf_package, aiperf_args, export_node_metrics).

  • A schema: 2 recipe that sets a field outside shared + its type's fields is rejected at load, naming the stray fields and the accepted ones.
  • A schema 1 recipe gets a warning and keeps loading, so nothing downstream breaks before it opts in.
  • Adding a field to a runner means adding it to that runner's config_fields; a test asserts every declared field exists on BenchmarkConfig.

Checked against the whole corpus

The rule was run over the 555 historical in-repo recipes and the 481 downstream InferenceMAX recipes, 1179 variants after override expansion, each forced to schema: 2. It rejects exactly one: a manual-type recipe carrying isl/osl/concurrencies. Every other field set per type is accepted. (The same pass surfaced two pre-existing, unrelated load failures in the downstream corpus: 98 recipes set telemetry.provider / default_frequency, which the current TelemetryConfig does not have, and 6 set benchmark.tokenizer_mode. Those fail on main today and are not touched here.)

Validation

  • Full suite green (1847 passed); lint, schema-docs drift check, every example validates.
  • tests/test_benchmark_fields.py: declared fields exist, each type accepts the field sets its recipes use, shared fields work for every type, schema 2 rejects with the expected message, schema 1 warns, defaults never count as set.

…s not use

Every runner now declares the BenchmarkConfig fields it reads as
config_fields, next to a shared set every type may set (client placement,
sweep, aiperf plumbing, post-processing). SrtConfig rejects a schema: 2
recipe that sets a field outside shared + its type's fields, naming the
stray fields and the accepted ones; a schema 1 recipe gets a warning and
keeps loading. Before this, a field the runner never read (isl on gsm8k,
num_shots on sa-bench) was a silent no-op.

Checked against the full recipe corpus: 555 historical in-repo recipes
plus 481 downstream InferenceMAX recipes, 1179 variants after override
expansion. The rule rejects exactly one (a manual-type recipe carrying
isl/osl/concurrencies); every other field set per type is accepted.
@ishandhanani

Copy link
Copy Markdown
Collaborator Author

Consolidated into #407, which carries these same commits as one PR against main (this description is reproduced there as one of its parts). Closing to keep review in one place; the branch stays for per-step reference via stack #398.

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