Skip to content

Key results and details output paths on the model revision - #1370

Open
thisisreyy wants to merge 1 commit into
huggingface:mainfrom
thisisreyy:add-revision-to-output-paths
Open

Key results and details output paths on the model revision#1370
thisisreyy wants to merge 1 commit into
huggingface:mainfrom
thisisreyy:add-revision-to-output-paths

Conversation

@thisisreyy

Copy link
Copy Markdown

Fixes #56.

The sharp edge first: this changes the on-disk output layout

Results move from {output_dir}/results/{model}/ to {output_dir}/results/{model}/{revision}/, and details from {output_dir}/details/{model}/{timestamp}/ to {output_dir}/details/{model}/{revision}/{timestamp}/. That relocation is the point of the issue, but it is a real change for anyone with scripts globbing those directories — they need one extra path segment.

I have kept the one read path inside lighteval working (see Backwards compatibility below), but external tooling that globs the results directory will need updating. Flagging it up front rather than burying it.

What changed

EvaluationTracker gains a _get_model_revision() helper, used by save_results() and _get_details_sub_folder().

The revision is read off the already-logged model config rather than threaded in as new state — GeneralConfigLogger.log_model_info() stores the whole config object, so it was already reachable. No new plumbing.

Why getattr rather than a revision field on ModelConfig

revision = getattr(self.general_config_logger.model_config, "revision", None)

revision is declared on TransformersModelConfig, VLMTransformersModelConfig, VLLMModelConfig and InferenceEndpointModelConfig — all defaulting to "main" — but not on the ModelConfig base. Six backends have no such field: dummy, litellm, inference-providers, tgi, sglang and custom.

Hoisting revision onto the base class would be the tidier-looking fix, and I decided against it deliberately. It would assert that every backend has a meaningful notion of a model revision, which is false — a LiteLLM config points at a hosted API model, a TGI config at a running server. The field would be an inert knob users could set to no effect. And because ModelConfig is a pydantic model, it would join the serialised config in every results file, changing config_general.model_config for those six backends and the reference dicts in test_model_config_property_with_different_model_configs. That is a wider blast radius than this issue warrants.

getattr keeps the dependency one-directional: the tracker asks whether a backend has a revision and degrades gracefully. Happy to switch to a base-class field if you would rather have the uniformity.

Fallback

"main" when the config declares no revision. It is the default already declared by all four configs that have the field, and it is what the issue's own example output shows.

Backwards compatibility

  • results_path_template gains a {revision} placeholder. Existing templates that do not use it are unaffected — str.format() ignores unused keyword arguments — and there is a test pinning that.

  • Details written by an earlier lighteval have no revision segment and would otherwise become unreadable, since _get_details_sub_folder() serves both save and load. When the revision-scoped folder holds no details, loading now falls back to the old layout. Saving always writes the new one.

    The fallback requires the legacy folder to contain details_*.parquet files itself, so a sibling revision directory is never mistaken for a timestamp folder — details/{model}/ is the parent of every revision directory, and a naive fallback would pick up v1.0/ when asked for v2.0. There is a test for exactly that.

    If you consider lighteval pre-1.0 enough that stranding old details is acceptable, I am happy to drop this — it is self-contained in _find_legacy_details_sub_folder() and one branch in load_details_datasets().

Testing

tests/unit/logging/test_evaluation_tracker.py: 8 → 17 passing.

Three existing assertions were updated for the new layout. Nine tests added, covering: two revisions writing to separate directories for both results and details; the "main" fallback; explicit main matching the fallback; templates with and without {revision}; legacy details still loading by explicit timestamp and by "last"; and the sibling-revision guard.

I verified the new tests fail without the source change (5 of 6 path tests, plus the 2 updated existing ones), and that the legacy-fallback tests fail with the fallback method neutered — so they exercise the fallback rather than passing incidentally.

For regressions I ran the full suite on this branch and on clean main and diffed the failure sets: identical, 18 both times, none in logging/. Those 18 are pre-existing local failures from vllm being unavailable on macOS (test_caching, test_reasoning_tags, test_vllm_model, test_metric_requests). tests/test_unit_harness_metrics.py was excluded from both runs — its fixture is a Git LFS pointer I could not fetch locally.

make quality passes.

Docs

cli_args.py help text and docs/source/saving-and-reading-results.mdx updated, since both documented the old paths and the template's available variables.

Results and details were written to a directory keyed only on the model
name, so evaluating several revisions of the same model interleaved every
run in one folder, distinguished only by a timestamp.

Results now go to {output_dir}/results/{model}/{revision}/ and details to
{output_dir}/details/{model}/{revision}/{timestamp}/. The revision is read
off the logged model config, which only the backends that can pin a model
version declare it on (transformers, VLM transformers, vllm and inference
endpoints); everything else falls back to "main", the default those same
backends use.

results_path_template gains a {revision} placeholder. Templates that do not
use it are unaffected, since str.format ignores unused keyword arguments.

Details written by an earlier lighteval, which have no revision segment,
are still readable: when a revision-scoped folder holds no details, loading
falls back to the old layout. The fallback requires the old folder to
contain details files itself, so a sibling revision directory is never
mistaken for a timestamp folder.
@thisisreyy
thisisreyy force-pushed the add-revision-to-output-paths branch from 2e913c4 to 48bbb99 Compare August 30, 2026 04:08
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.

Append revision to filepath in --output_dir?

1 participant