Repository navigation
test: require run settings for new results - #676
KennethEnevoldsen merged 11 commits into
Conversation
There was a problem hiding this comment.
Also some changes would be needed after embeddings-benchmark/mteb#5099 (and already existing
results/reduce_large_json_files.py
Line 41 in a331fb4
nomic-ai__nomic-embed-text-v2-moe
Can you also add test that all tasks that subbmitted in PR are present in run_settings.jsonl.
Also test do not cover experiments/ subdirectories
|
UPD embeddings-benchmark/mteb#5099 was merged |
|
Thanks, updated this PR to cover the post-#5099 subsets-style run_settings format and to keep the diff-scoped check tied to the current base branch rather than a stale pull request base SHA. I also added regression coverage for experiments/ paths and for multi-task submissions where every added result needs a matching run_settings entry. The latest CI is green. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
@Samoed Can you take a look at this one? |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Resolve the inconsistent MTEB version threshold before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds a diff-scoped guard requiring new result files to include matching run_settings.jsonl metadata and supported MTEB versions.
Changes:
- Validates result and run-settings matches.
- Adds version and tuple checks.
- Adds regression tests and error reporting.
The implementation enforces >=2.14.0 in three locations, conflicting with the stated >2.0.0 requirement.
| File | Description |
|---|---|
tests/test_new_results_run_settings.py |
Tests validation behavior and edge cases. |
tests/run_settings_check.py |
Implements result, run-settings, tuple, and version validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
KennethEnevoldsen
left a comment
KennethEnevoldsen
left a comment
There was a problem hiding this comment.
I don't know about this PR. For what it is testing I am not sure you need to add 16 tests
I general it seems like more effort to review that it is correct than to implement, at a quick glance simple stuff like version check is incorrect so it would require a lot of effort to check if everything it correct.
|
@Samoed there are some more things which can be fixed/made short, should I take this PR and pushed changes? |
|
I simiplified checks |
| try: | ||
| base_ref = get_base_ref() | ||
| except RuntimeError as e: | ||
| pytest.skip(str(e)) |
There was a problem hiding this comment.
I guess we can remove try-except, so broken checkout fails instead of skipping.
| def load_run_settings( | ||
| path: Path, relative_path: str | ||
| ) -> tuple[set[tuple[str, str, str]], list[str]]: | ||
| if not path.exists(): | ||
| return set(), [f"{relative_path} is missing {RUN_SETTINGS_FILENAME}."] | ||
|
|
||
| covered: set[tuple[str, str, str]] = set() | ||
| errors = [] | ||
| for line_number, line in enumerate(path.read_text().splitlines(), start=1): | ||
| if not line.strip(): | ||
| continue | ||
|
|
||
| source = f"{relative_path} {RUN_SETTINGS_FILENAME} line {line_number}" | ||
| try: | ||
| entry = json.loads(line) | ||
| except json.JSONDecodeError as e: | ||
| errors.append(f"{source} is not valid JSON: {e}") | ||
| continue | ||
| if not isinstance(entry, dict): | ||
| errors.append(f"{source} must be an object.") | ||
| continue | ||
|
|
||
| keys = [key for key, _, _ in _expand_run_settings_entry(entry)] | ||
| if not keys or any(not task for task, _, _ in keys): | ||
| errors.append(f"{source} must define task, split(s) and subset(s).") | ||
| continue | ||
| covered.update(keys) | ||
|
|
||
| if not covered and not errors: | ||
| errors.append(f"{relative_path} {RUN_SETTINGS_FILENAME} has no entries.") | ||
| return covered, errors |
There was a problem hiding this comment.
I guess we can simplify this to:
def load_run_settings(path: Path) -> set[tuple[str, str, str]]:
covered = set()
for line_number, line in enumerate(path.read_text().splitlines(), start=1):
if not line.strip():
continue
try:
entry = json.loads(line)
except json.JSONDecodeError as e:
raise ValueError(f"{path} line {line_number} is not valid JSON: {e}") from e
covered.update(key for key, _, _ in _expand_run_settings_entry(entry))
return covered
Because other errors like no task, split, or that isn't a dict only for which we are adding checks, will not be occurring
| covered, run_settings_errors = load_run_settings( | ||
| result_path.parent / RUN_SETTINGS_FILENAME, relative_path | ||
| ) | ||
| errors += run_settings_errors | ||
| if not covered: | ||
| return errors |
There was a problem hiding this comment.
and instead of this update to:
run_settings_path = result_path.parent / RUN_SETTINGS_FILENAME
if not run_settings_path.exists():
return errors + [f"{relative_path} is missing {RUN_SETTINGS_FILENAME}."]
covered = load_run_settings(run_settings_path)
added few more minor comment, and I guess its good |
There was a problem hiding this comment.
would merge into the other (since it is now so small)
| def validate(result_path: Path) -> list[str]: | ||
| return validate_result_file(result_path, "results/model/revision/DemoTask.json") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "fields", | ||
| [ | ||
| pytest.param({"split": "test", "subset": "en"}, id="legacy-singular"), | ||
| pytest.param( | ||
| {"split": "test", "subsets": ["en", "de"]}, id="collapsed-subsets" | ||
| ), | ||
| pytest.param({"splits": ["test"], "subsets": ["en"]}, id="current-plural"), | ||
| pytest.param({"splits": ["test"], "subset": "en"}, id="mixed"), | ||
| ], | ||
| ) | ||
| def test_accepts_every_run_settings_shape(tmp_path, fields): | ||
| result_path = write_result(tmp_path, subsets=("en",)) | ||
| write_run_settings(tmp_path, **fields) | ||
|
|
||
| assert validate(result_path) == [] |
There was a problem hiding this comment.
is this needed? As I understand it it is test of the test
There was a problem hiding this comment.
(also applied to the below)
There was a problem hiding this comment.
Yes, these are tests for run_settings_check.py. I think they could be helpful, but have test for test is a bit strange, agree
There was a problem hiding this comment.
I am fine either way I think - though probably leaning remove and combine the two scripts
Why
New result submissions can still omit run settings or be produced with older MTEB versions, which makes submitted scores harder to reproduce. This adds a diff-scoped guard for new result files only.
Close embeddings-benchmark/mteb#5031.
What changed
results/.run_settings.jsonlfor each new task result.(task, split, subset)entries and MTEB>2.14.0in result metadata, score blocks, and matching run settings.Validation
python -m pytest tests/test_new_results_run_settings.py- 6 passed, 1 skippedpython -m py_compile tests/test_new_results_run_settings.pygit diff --checkRisk / follow-up
Existing historical results are not migrated or checked. Future result PRs that omit run settings will fail this guard, which is intentional for new submissions; maintainers can relax the threshold if a staged rollout is preferred.