feat: add inclusive language evaluator - #358
Conversation
Add a deterministic InclusiveLanguage evaluator that scans actual_output for non-inclusive terms using case-insensitive word-boundary regex matching. The evaluator ships with a configurable mapping of banned terms to suggested replacements. Users can override the default term list by passing a custom dict. No LLM required - pure regex.
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
area-evaluators
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| name: Optional instance name for identification in reports. | ||
| """ | ||
| super().__init__(name=name) | ||
| self.terms = terms if terms is not None else self.DEFAULT_TERMS |
There was a problem hiding this comment.
Issue (Critical): When terms is None, self.terms is bound directly to the mutable class-level DEFAULT_TERMS dict. Every default-constructed instance shares the same object, so any in-place mutation of self.terms leaks to all other instances and to the class attribute itself. to_dict() also returns that same shared reference. Confirmed:
a = InclusiveLanguage(); b = InclusiveLanguage()
a.terms["newterm"] = "x"
"newterm" in b.terms # True
"newterm" in InclusiveLanguage.DEFAULT_TERMS # TrueSuggestion: Copy the input so each instance owns its dict, e.g.:
self.terms = dict(terms) if terms is not None else dict(self.DEFAULT_TERMS)There was a problem hiding this comment.
Thanks — the default path is fixed (dict(self.DEFAULT_TERMS)), which resolves the cross-instance/class-attribute leak. ✅
One residual, subtler issue in the same spot: the custom terms path still aliases the caller's dict (self.terms = terms), and _compiled_patterns is now snapshotted at construction. So if a caller mutates the dict they passed in after construction, to_dict() reflects the change but evaluate() keeps using the stale compiled patterns — the two drift out of sync. Reproduced:
custom = {"foo": "bar"}
e = InclusiveLanguage(terms=custom)
custom["blacklist"] = "denylist"
"blacklist" in e.to_dict()["terms"] # True
e.evaluate(EvaluationData(input="q", actual_output="the blacklist"))[0].test_pass # True (misses it)Minor edge case, but copying the custom dict too (self.terms = dict(terms) if terms is not None else dict(self.DEFAULT_TERMS)) makes self.terms authoritative and keeps it consistent with _compiled_patterns.
| "slave": "replica", | ||
| "blackday": "blocked day", | ||
| "whiteday": "clear day", | ||
| } |
There was a problem hiding this comment.
Issue (Important – design): The bare tokens master and slave will fire on a lot of legitimate output — "master's degree", "master copy", "master class", quoting/discussing the terms, or referencing an external system literally named that way (e.g. a master git branch). Since a match flips test_pass to False, this default list risks a high false-positive rate for a deterministic guardrail. Additionally, blackday/whiteday aren't standard English words and read as placeholder entries.
Suggestion: Document the tradeoff and the opt-out (terms=...) explicitly in the class docstring, and reconsider whether master/slave/blackday/whiteday belong in the shipped defaults vs. a narrower, higher-precision set. This default term list is a customer-facing decision worth calling out for API review (see general comment).
There was a problem hiding this comment.
The docstring note on false positives clearly documents the tradeoff and the terms=... opt-out — that resolves this. ✅ Keeping the defaults as-is is a reasonable call now that the behavior is documented (this is exactly the kind of default-behavior decision worth confirming in API review).
| text = str(evaluation_case.actual_output).lower() | ||
| found: list[tuple[str, str]] = [] | ||
| for term, suggestion in self.terms.items(): | ||
| if re.search(rf"\b{re.escape(term)}\b", text): |
There was a problem hiding this comment.
Issue (Suggestion): The \b{term}\b patterns are recompiled on every evaluate() call for every term. When running across large suites this repeats work unnecessarily.
Suggestion: Precompile once in __init__ (e.g. store [(re.compile(rf"\b{re.escape(t)}\b", re.IGNORECASE), t, s) for ...]) and reuse. This also lets you drop the .lower() on the whole text in favor of the IGNORECASE flag.
There was a problem hiding this comment.
Precompiling in __init__ looks good. ✅ Minor note: the patterns are compiled without re.IGNORECASE and case-insensitivity relies on .lower()ing the text, so a custom terms dict with uppercase keys would silently never match. Defaults are all lowercase so this is latent, but lowercasing the term when compiling (or using re.IGNORECASE) would make it robust.
|
Issue (Important – process): This PR adds a new public evaluator ( Suggestion: Add the |
| assert "legacy" in results[0].reason | ||
| assert "historical" in results[0].reason | ||
|
|
||
| def test_empty_terms_always_passes(self): |
There was a problem hiding this comment.
Issue (Important – testing): The output here is fully deterministic, and EvaluationOutput is a pydantic BaseModel that supports equality. Most tests assert individual fields (results[0].score, results[0].test_pass, substring checks on reason) across separate cases. Per-field assertions silently miss regressions in fields that aren't checked (e.g. an unexpected label, or a reason/score mismatch).
Suggestion: Assert the whole result list in a single equality check where the output is deterministic, e.g.:
assert results == [EvaluationOutput(score=1.0, test_pass=True, reason="no non-inclusive terms found")]Keep the narrower substring assertions only for the multi-term reason case where ordering/formatting is the specific behavior under test.
There was a problem hiding this comment.
Nicely done — tests now assert the full EvaluationOutput list in a single equality check, which will catch regressions in any field. ✅
|
Assessment: Request Changes Clean, well-tested addition that follows the existing deterministic-evaluator pattern (28 tests pass). The main blocker is a shared-mutable-state bug on the default term list; the rest are design/process items to resolve before merge. Review Categories
Nice to see a purpose-built, LLM-free evaluator with thorough word-boundary and edge-case coverage. |
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Re-review of Assessment: Comment (close to approval) Thanks for the quick turnaround — the substantive code feedback is addressed and all 29 tests pass locally. Status of previous items
Nothing here is blocking on the code side beyond the minor custom-dict copy; the open item is the API-review label/artifacts so a designated reviewer can sign off on the public surface and default term list. |
|
@strandly-the-agent can you review this PR and give a breakdown of it? I am not very familar with eval package. what is our guideline of accepting new features. |
|
@JackYPCOnline here's the breakdown. TL;DR — Code is clean and well-tested; two things matter. (1) There's a real bug: a custom What the PR does, and how evaluators work in this package (for the unfamiliar)An evaluator is a class subclassing This PR adds a third deterministic output check, The real bar for accepting a new feature hereEverything the repo actually documents — no hidden extra gate:
Against that: issue #344 has no maintainer response and no Recommendation: land the primitive, drop the shipped listThe gap is real —
Concrete alternative: same code, reshaped as a generic word-boundary term-list check with required Code finding the earlier reviews missed: uppercase custom terms silently never match
e = InclusiveLanguage(terms={"Blacklist": "denylist"})
e.evaluate(EvaluationData(input="i", actual_output="this is a blacklist"))
# -> score=1.0 test_pass=True reason='no non-inclusive terms found'Fix: compile with Happy to post the generic-vs-specific design question on #344 so the decision is tracked there — just say so. |
|
@max-rattray-aws Do you mind check the bugs that agent mention? |
3 participants
Summary
Adds a deterministic
InclusiveLanguageevaluator that scansactual_outputfor non-inclusive terms using case-insensitive word-boundary regex matching. No LLM required.The evaluator ships with a configurable mapping of banned terms to suggested replacements. Users can override the default term list by passing a custom dict.
Usage
What's tested
to_dict()Related to #344