fix: make capability failures explicit and add fidelity delta report - #18
Conversation
Summary of ChangesHello @wolfiesch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the clarity and utility of benchmark results by making unimplemented adapter capabilities explicit through Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements. It makes adapter capability failures explicit by raising NotImplementedError instead of failing silently, which will make debugging much clearer. It also adds a new failure classification system to distinguish between unimplemented features and incorrect results. The highlight is the new fidelity delta reporting, which automatically generates a markdown report comparing the two most recent benchmark runs, providing a quick signal for regressions or improvements. The changes are well-tested. I have one suggestion to improve memory efficiency when processing the history file.
| entries: list[dict[str, Any]] = [] | ||
| for line in history_path.read_text().splitlines(): | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| parsed = json.loads(line) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if isinstance(parsed, dict): | ||
| entries.append(parsed) | ||
|
|
||
| if len(entries) < 2: | ||
| out_path.write_text("# Fidelity Deltas\n\nNeed at least two runs in history.jsonl.\n") | ||
| return | ||
|
|
||
| previous = entries[-2] | ||
| current = entries[-1] |
There was a problem hiding this comment.
To improve memory efficiency, especially if history.jsonl becomes very large, it's better to read the file line-by-line and only keep track of the last two entries. This avoids loading the entire history file into memory.
| entries: list[dict[str, Any]] = [] | |
| for line in history_path.read_text().splitlines(): | |
| line = line.strip() | |
| if not line: | |
| continue | |
| try: | |
| parsed = json.loads(line) | |
| except json.JSONDecodeError: | |
| continue | |
| if isinstance(parsed, dict): | |
| entries.append(parsed) | |
| if len(entries) < 2: | |
| out_path.write_text("# Fidelity Deltas\n\nNeed at least two runs in history.jsonl.\n") | |
| return | |
| previous = entries[-2] | |
| current = entries[-1] | |
| entries: list[dict[str, Any]] = [] | |
| with history_path.open("r", encoding="utf-8") as f: | |
| for line in f: | |
| line = line.strip() | |
| if not line: | |
| continue | |
| try: | |
| parsed = json.loads(line) | |
| except json.JSONDecodeError: | |
| continue | |
| if isinstance(parsed, dict): | |
| entries.append(parsed) | |
| if len(entries) > 2: | |
| entries.pop(0) | |
| if len(entries) < 2: | |
| out_path.write_text("# Fidelity Deltas\n\nNeed at least two runs in history.jsonl.\n") | |
| return | |
| previous, current = entries |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _failure_note_from_actual(actual: JSONDict) -> str: | ||
| if "error" in actual: | ||
| error_text = str(actual.get("error", "")).lower() | ||
| unsupported_markers = ( | ||
| "notimplemented", | ||
| "not implemented", | ||
| "unsupported", | ||
| "not supported", | ||
| "read-only", | ||
| "write-only", | ||
| ) | ||
| if any(marker in error_text for marker in unsupported_markers): | ||
| return "Not implemented" | ||
| return "Incorrect result" | ||
| return "Incorrect result" |
There was a problem hiding this comment.
_failure_note_from_actual bypassed for Tier 3 exceptions
When Tier 3 base methods (read_named_ranges, add_named_range, read_tables, add_table) raise NotImplementedError, the exception propagates up through the read_*_actual helper and is caught by the generic except Exception as e handler at line 532, which sets notes=f"Exception: {type(e).__name__}" — i.e., "Exception: NotImplementedError".
This means _failure_note_from_actual is never invoked for these cases, and the note will be "Exception: NotImplementedError" rather than "Not implemented". The function is only reached when the adapter call succeeds but returns a dict containing an "error" key.
This may be intentional (the exception handler note is arguably more descriptive), but it's worth confirming this is the desired behavior — since the PR description mentions wanting to "clearly distinguish" not-implemented from incorrect results, you may want the exception handler to also produce a "Not implemented" note for NotImplementedError specifically:
except NotImplementedError as e:
return TestResult(
...
notes="Not implemented",
...
)
except Exception as e:
...
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/excelbench/harness/runner.py
Line: 40:54
Comment:
**`_failure_note_from_actual` bypassed for Tier 3 exceptions**
When Tier 3 base methods (`read_named_ranges`, `add_named_range`, `read_tables`, `add_table`) raise `NotImplementedError`, the exception propagates up through the `read_*_actual` helper and is caught by the generic `except Exception as e` handler at line 532, which sets `notes=f"Exception: {type(e).__name__}"` — i.e., `"Exception: NotImplementedError"`.
This means `_failure_note_from_actual` is never invoked for these cases, and the note will be `"Exception: NotImplementedError"` rather than `"Not implemented"`. The function is only reached when the adapter call succeeds but returns a dict containing an `"error"` key.
This may be intentional (the exception handler note is arguably more descriptive), but it's worth confirming this is the desired behavior — since the PR description mentions wanting to "clearly distinguish" not-implemented from incorrect results, you may want the exception handler to also produce a `"Not implemented"` note for `NotImplementedError` specifically:
```
except NotImplementedError as e:
return TestResult(
...
notes="Not implemented",
...
)
except Exception as e:
...
```
How can I resolve this? If you propose a fix, please make it concise.| for mode in ("read", "write"): | ||
| prev_value = prev_feature.get(mode) | ||
| curr_value = curr_feature.get(mode) | ||
| if prev_value is None or curr_value is None: |
There was a problem hiding this comment.
New features/modes silently ignored in deltas
When one run has a library/feature/mode entry and the other doesn't (i.e., prev_value is None or curr_value is None), the delta is silently skipped. This means if a library adds support for a new feature between runs (going from no score to a score), or drops one entirely, it won't appear in the delta report. Depending on intent, this could mask meaningful regressions or improvements — particularly if a library is newly added or removed between runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/excelbench/results/renderer.py
Line: 803:803
Comment:
**New features/modes silently ignored in deltas**
When one run has a library/feature/mode entry and the other doesn't (i.e., `prev_value is None or curr_value is None`), the delta is silently skipped. This means if a library adds support for a new feature between runs (going from no score to a score), or drops one entirely, it won't appear in the delta report. Depending on intent, this could mask meaningful regressions or improvements — particularly if a library is newly added or removed between runs.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR makes adapter capability reporting more explicit by having Tier-3 methods raise NotImplementedError instead of silently returning empty results, adds failure classification to distinguish "Not implemented" from "Incorrect result", and introduces automated fidelity delta reporting between benchmark runs.
Changes:
- Tier-3 adapter methods (read_named_ranges, add_named_range, read_tables, add_table) now raise NotImplementedError by default instead of returning empty lists or None
- Added
_failure_note_from_actualfunction to classify test failures as either "Not implemented" or "Incorrect result" based on error text markers - Added automatic fidelity delta reporting that compares the two most recent runs and generates a FIDELITY_DELTAS.md file with regressions, improvements, and net score changes
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/excelbench/harness/adapters/base.py | Updated Tier-3 methods to raise NotImplementedError with descriptive messages instead of returning empty/no-op defaults |
| src/excelbench/harness/runner.py | Added _failure_note_from_actual function to classify failures and integrated it into test_read_case to automatically set TestResult notes |
| src/excelbench/results/renderer.py | Added _render_fidelity_deltas and _compute_fidelity_deltas functions to generate delta reports, integrated into render_results workflow |
| tests/test_adapter_base.py | Added test verifying Tier-3 methods raise NotImplementedError by default |
| tests/test_named_ranges.py | Updated tests to expect NotImplementedError instead of empty results |
| tests/test_tables.py | Updated tests to expect NotImplementedError instead of no-op behavior |
| tests/test_runner_utils.py | Added tests for failure note classification logic |
| tests/test_renderer_utils.py | Added tests for fidelity delta computation and rendering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ca63bfda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| previous = entries[-2] | ||
| current = entries[-1] | ||
| deltas = _compute_fidelity_deltas(previous, current) |
There was a problem hiding this comment.
_render_fidelity_deltas always compares the last two history entries, but it does not check their profile fields before computing deltas. I verified excelbench benchmark allows writing both xlsx and xls runs to the same output directory (default results), so a normal profile switch can produce regression/improvement rows that are just format differences rather than real fidelity changes; this makes the new report unreliable for tracking regressions.
Useful? React with 👍 / 👎.
| passed=passed, | ||
| expected=expected, | ||
| actual=actual, | ||
| notes=None if passed else _failure_note_from_actual(actual), |
There was a problem hiding this comment.
The new note mapping only runs in the non-exception return path, so capability failures raised by the new Tier-3 defaults (read_named_ranges/read_tables now raise NotImplementedError) still land in the generic exception handler and get notes="Exception: NotImplementedError" instead of "Not implemented". That means the change still does not consistently distinguish unsupported features from incorrect results in emitted test results.
Useful? React with 👍 / 👎.
Motivation
Description
read_named_ranges,add_named_range,read_tables, andadd_tableraiseNotImplementedErrorinstead of silently returning empty/no‑ops (file:src/excelbench/harness/adapters/base.py)._failure_note_from_actualto classify failing reads/writes and setTestResult.notesto eitherNot implemented(for unsupported/not implemented errors) orIncorrect result(for mismatches/other failures), and wire it intotest_read_caseso notes are attached automatically (file:src/excelbench/harness/runner.py).FIDELITY_DELTAS.md(fromhistory.jsonl) comparing the two most recent runs and uses_compute_fidelity_deltasto produce a per-library/feature/mode delta table and a summary of regressions/improvements (file:src/excelbench/results/renderer.py).tests/*_utils.py,tests/test_adapter_base.py,tests/test_named_ranges.py,tests/test_renderer_utils.py,tests/test_tables.py).Testing
uv run ruff check— passed.uv run mypy— passed.uv run pytest -qinitially failed due to repositoryaddoptsincluding--cov=excelbench(coverage plugin unavailable in this environment), so tests were run with the override-o addopts='';uv run pytest -q -o addopts=''completed successfully with1089 passed, 53 skipped, 6 xfailed.Codex Task
Greptile Overview
Greptile Summary
This PR makes two focused improvements to the benchmark harness: (1) Tier 3 adapter base methods (
read_named_ranges,add_named_range,read_tables,add_table) now raiseNotImplementedErrorinstead of silently returning empty/no-op values, making missing adapter capabilities explicit in test output; and (2) a new fidelity delta report (FIDELITY_DELTAS.md) is automatically generated fromhistory.jsonlto highlight per-library/feature score regressions and improvements between runs.base.pynow raiseNotImplementedErrorwith descriptive messages. This is a breaking change for any downstream adapter that relied on the silent no-op behavior, though the existing exception handler intest_read_casecatches these gracefully._failure_note_from_actualfunction classifies failing test results as either "Not implemented" or "Incorrect result" based on error text markers. Note: for the Tier 3NotImplementedErrorexceptions specifically, this function is bypassed — exceptions are caught by the generic handler which setsnotes="Exception: NotImplementedError"instead._render_fidelity_deltasand_compute_fidelity_deltascompare the two most recent history entries and produce a markdown table of changed scores with regression/improvement counts. Edge cases (no history, single run, no changes) are handled correctly.NotImplementedError, and new tests cover the failure-note mapping and fidelity delta computation/rendering.Confidence Score: 4/5
_failure_note_from_actualfunction doesn't actually get invoked for the main Tier 3 NotImplementedError scenario it was designed to support (those exceptions are caught by the generic handler). This is functional but may produce less specific notes than intended.src/excelbench/harness/runner.py— the interaction between_failure_note_from_actualand the generic exception handler deserves a second look to confirm the desired note text for NotImplementedError cases.Important Files Changed
_failure_note_from_actualfor classifying failure notes on test results. The function works for dict-based error returns but Tier 3 NotImplementedError exceptions bypass it via the generic exception handler._render_fidelity_deltasand_compute_fidelity_deltas. Well-structured with proper edge case handling for missing/insufficient history._failure_note_from_actualfunction covering NotImplementedError, unsupported, and generic error cases.Flowchart
flowchart TD A[test_read_case called] --> B{Feature type?} B -->|named_ranges / tables| C[Call read_*_actual helper] C --> D[Call adapter.read_named_ranges / read_tables] D --> E{Adapter implements method?} E -->|Yes - concrete adapter| F[Returns data] F --> G[Compare expected vs actual] G --> H{Passed?} H -->|Yes| I[TestResult: passed=True, notes=None] H -->|No| J[_failure_note_from_actual] J --> K[TestResult: notes='Incorrect result'] E -->|No - base class| L[Raises NotImplementedError] L --> M[Caught by except Exception handler] M --> N["TestResult: notes='Exception: NotImplementedError'"] O[render_results] --> P[_append_history] P --> Q[_render_fidelity_deltas] Q --> R{history.jsonl has 2+ entries?} R -->|Yes| S[_compute_fidelity_deltas] S --> T[Write FIDELITY_DELTAS.md] R -->|No| U[Write placeholder message]Last reviewed commit: 54ca63b